Re: [edk2] [PATCH 1/1] EmbeddedPkg/Lan9118Dxe: Do not return uninitialised TxBuff

2016-05-12 Thread Laszlo Ersek 
On 05/11/16 23:50, Michael Brown wrote:
> Conform to the specification for GetStatus(), which states that "if
> there are no transmit buffers to recycle and TxBuf is not NULL, *TxBuf
> will be set to NULL".
> 
> Cc: Leif Lindholm 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Brown 
> ---
>  EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c 
> b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> index 8af23df..aabaf60 100644
> --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> @@ -1055,6 +1055,8 @@ SnpGetStatus (
>LanDriver->Stats.TxTotalFrames += 1;
>*TxBuff = LanDriver->TxRing[PacketTag % LAN9118_TX_RING_NUM_ENTRIES];
>  }
> +  } else if (TxBuff != NULL) {
> +*TxBuff = NULL;
>}
>  
>// Check for a TX Error interrupt
> 

(I've also seen your iPXE commit 6164741f81fb. (BTW the arm/aarch64
enablement in iPXE is impressive to the point of stunning.))

I think we might need a bigger knife here. The current implementation
not only breaks the following sentence from the spec:

If there are no transmit buffers to recycle and TxBuf is not NULL,
*TxBuf will be set to NULL.

it also breaks this:

  If this [i.e., TxBuf] is NULL, then the transmit buffer status will
  not be read.

Currently this function reads the LAN9118_TX_FIFO_INF register
unconditionally, and if the (masked) result is positive, it reads --
consumes? -- the LAN9118_TX_STATUS register regardless of TxBuf. To me
that seems like a direct violation of the language above.

So, instead of this patch, I would propose:

  if (TxBuf != NULL) {
NumTxStatusEntries = ...;
if (NumTxStatusEntries == 0) {
  *TxBuf = NULL;
} else {
  TxStatus = ...;
  PacketTag = ...;
  ...
}
  }

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/1] EmbeddedPkg/Lan9118Dxe: Do not return uninitialised TxBuff

2016-05-12 Thread Ard Biesheuvel 
On 11 May 2016 at 23:50, Michael Brown  wrote:
> Conform to the specification for GetStatus(), which states that "if
> there are no transmit buffers to recycle and TxBuf is not NULL, *TxBuf
> will be set to NULL".
>
> Cc: Leif Lindholm 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Brown 

Reviewed-by: Ard Biesheuvel 

Pushed as e117c894fdf1

Thanks!


> ---
>  EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c 
> b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> index 8af23df..aabaf60 100644
> --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
> @@ -1055,6 +1055,8 @@ SnpGetStatus (
>LanDriver->Stats.TxTotalFrames += 1;
>*TxBuff = LanDriver->TxRing[PacketTag % LAN9118_TX_RING_NUM_ENTRIES];
>  }
> +  } else if (TxBuff != NULL) {
> +*TxBuff = NULL;
>}
>
>// Check for a TX Error interrupt
> --
> 2.3.8
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 1/1] EmbeddedPkg/Lan9118Dxe: Do not return uninitialised TxBuff

2016-05-11 Thread Michael Brown 
Conform to the specification for GetStatus(), which states that "if
there are no transmit buffers to recycle and TxBuf is not NULL, *TxBuf
will be set to NULL".

Cc: Leif Lindholm 
Cc: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Brown 
---
 EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c 
b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
index 8af23df..aabaf60 100644
--- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
+++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c
@@ -1055,6 +1055,8 @@ SnpGetStatus (
   LanDriver->Stats.TxTotalFrames += 1;
   *TxBuff = LanDriver->TxRing[PacketTag % LAN9118_TX_RING_NUM_ENTRIES];
 }
+  } else if (TxBuff != NULL) {
+*TxBuff = NULL;
   }
 
   // Check for a TX Error interrupt
-- 
2.3.8

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel