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 <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Brown <[email protected]>
> ---
> 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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
