Re: [Linaro-uefi] [PATCH] OpenPlatformPkg/Lan91xDxe: Fixed driver crash and TX recycle buffer

Hi Daniil,

On 9 June 2016 at 00:54, Daniil Egranov daniil.egranov@arm.com wrote:

Hi Ryan,

Thanks for your comments.

On 06/07/2016 08:51 AM, Ryan Harkin wrote:

...

Hi Daniil,

I think the contents of this patch are generally OK, but there are a few issues.

On 7 June 2016 at 01:25, Daniil Egranov daniil.egranov@arm.com wrote:

...

Fixed driver crash on ARP packets. Fixed TX recycle buffer overflow. Added prototype for SnpWaitForPacketNotify SNP call and notify event.

It seems that this patch is doing three separate things, so I'd expect three patches.

Can you explain how you reproduce the crash and buffer overflow and explain the purpose of the SNP event?

The crash can be reproduced from the UEFI shell command line. Any command which triggers ARP broadcast will rise the exception (for ex. ping).

OK, so the crash isn't really a crash, but a failure: i.e. the model doesn't lock up, it continues to run. And it seems the failure and buffer overflow fix are the same thing. That's good.

The original Tx Queue depth is very shallow (16). It possibly works for the real hardware but I am getting "LAN91x: SnpTransmit(): TxQueue insert failure." under our platform model almost immediately after starting a network communication (like "ping") . The call rate of SnpTransmit() which is filling up the queue with used buffers and SnpGetStatus() which is giving it back to MNP may not be the same under the model and can be effected by performance of the model. Instead of tuning up the custom queue, I think it's better to use a linked list from the edk2 base library which should be good for both models and the real hardware.

Thanks, I can reproduce this now. Initially, ping works for me. I can run "ping 192.168.1.1" many times in a row and without problems. But it does fail occasionally, even if it is hard to reproduce. I see the same error message during PXEboot every time, so that's a more reliable way to reproduce the problem for me.

Once I add your patch, the problem goes away. So I'm happy that the buffer overflow fix is worth having.

SNP event: this is the attempt to use the SNP event for receiving a data. The current code is a template for implementing the even call. It does not return anything yet. Some of the UEFI applications are using loops where they directly call MNP receive function until all data received. With such loops, I have some issues with the platform model performance. It will be more appropriate to wait for the data event instead of forcing the controller poll for a data.

Thanks for clarifying. In that case, this change shouldn't be part of this patch.

This patch does not affect the driver functionality but adds the initial structure.

And as it doesn't do anything, then I don't think you should be submitting it even as a separate patch.

Cheers, Ryan.

...

...

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

Drivers/Net/Lan91xDxe/Lan91xDxe.c | 179 +++++++++++++++++++++++++------------- 1 file changed, 118 insertions(+), 61 deletions(-)

diff --git a/Drivers/Net/Lan91xDxe/Lan91xDxe.c b/Drivers/Net/Lan91xDxe/Lan91xDxe.c index 3f857a5..45b330f 100644 --- a/Drivers/Net/Lan91xDxe/Lan91xDxe.c +++ b/Drivers/Net/Lan91xDxe/Lan91xDxe.c @@ -65,10 +65,7 @@ typedef struct _LAN91X_DRIVER { EFI_NETWORK_STATISTICS Stats;

// Transmit Buffer recycle queue

-#define TX_QUEUE_DEPTH 16

• VOID *TxQueue[TX_QUEUE_DEPTH];
• UINTN TxQueHead;
• UINTN TxQueTail;

• LIST_ENTRY TransmitQueueHead;

  // Register access variables UINTN IoBase; // I/O Base Address

@@ -137,47 +134,18 @@ STATIC CHAR16 CONST * CONST ChipIds[ 16 ] = { NULL, NULL, NULL };

+typedef struct {

• VOID *Buf;
• UINTN Length;

+} MSK_SYSTEM_BUF;

-/* ------------------ TxBuffer Queue functions ------------------- */

-#define TxQueNext(off) ((((off) + 1) >= TX_QUEUE_DEPTH) ? 0 : ((off) + 1))

-STATIC -BOOLEAN -TxQueInsert (

• IN LAN91X_DRIVER *LanDriver,
• IN VOID *Buffer
• )

-{

• if (TxQueNext (LanDriver->TxQueTail) == LanDriver->TxQueHead) {
• return FALSE;
• }
• LanDriver->TxQueue[LanDriver->TxQueTail] = Buffer;
• LanDriver->TxQueTail = TxQueNext (LanDriver->TxQueTail);
• return TRUE;

-}

-STATIC -VOID -*TxQueRemove (

• IN LAN91X_DRIVER *LanDriver
• )

-{

• VOID *Buffer;
• if (LanDriver->TxQueTail == LanDriver->TxQueHead) {
• return NULL;
• }
• Buffer = LanDriver->TxQueue[LanDriver->TxQueHead];
• LanDriver->TxQueue[LanDriver->TxQueHead] = NULL;
• LanDriver->TxQueHead = TxQueNext (LanDriver->TxQueHead);

+typedef struct {

• UINTN Signature;
• LIST_ENTRY Link;
• MSK_SYSTEM_BUF SystemBuf;

+} MSK_LINKED_SYSTEM_BUF;

• return Buffer;

-} +#define TX_MBUF_SIGNATURE SIGNATURE_32 ('t','x','m','b')

/* ------------------ MAC Address Hash Calculations ------------------- */

@@ -1105,6 +1073,42 @@ exit_unlock: return Status; }

+/**

• Notification call back function for WaitForPacket event.
• <at> param Event EFI Event.
• <at> param SnpPtr Pointer to YUKON_DRIVER structure.

+**/ +VOID +EFIAPI +SnpWaitForPacketNotify (

• EFI_EVENT Event,
• VOID *SnpPtr
• )

+{

• DEBUG ((EFI_D_NET, "LAN91x: SnpWaitForPacketNotify()\n"));
• //
• // Do nothing if either parameter is a NULL pointer.
• //
• if (Event == NULL || SnpPtr == NULL) {
• return ;
• }
• //
• // Do nothing if the SNP interface is not initialized.
• //
• switch (((LAN91X_DRIVER *) SnpPtr)->SnpMode.State) {
• case EfiSimpleNetworkInitialized:

•  break;
  

• case EfiSimpleNetworkStopped:
• case EfiSimpleNetworkStarted:
• default:

•  return ;
  

• }

+}

This function does nothing, it just returns. Is it supposed to be doing something? If not, but yet it's needed, why doesn't it just return? No parameter checking or state checking is needed. Explaining the purpose of the event seems more important when it seems that it does nothing.

Please see the explanation above.

...

...

/* ** UEFI Initialize() function ** @@ -1148,10 +1152,25 @@ SnpInitialize ( // Find the LanDriver structure LanDriver = INSTANCE_FROM_SNP_THIS(Snp);

• Status = gBS->CreateEvent (

•    EVT_NOTIFY_WAIT,
  

•    TPL_NOTIFY,
  

•    &SnpWaitForPacketNotify,
  

•    LanDriver,
  

•    &LanDriver->Snp.WaitForPacket
  

•    );
  

• if (EFI_ERROR (Status)) {
• LanDriver->Snp.WaitForPacket = NULL;
• Status = EFI_DEVICE_ERROR;
• ReturnUnlock (EFI_DEVICE_ERROR);
• }
• // Initiate a software reset Status = SoftReset (LanDriver); if (EFI_ERROR(Status)) { DEBUG((EFI_D_WARN, "LAN91x: Soft reset failed\n"));
• gBS->CloseEvent (LanDriver->Snp.WaitForPacket); ReturnUnlock (EFI_DEVICE_ERROR); }

@@ -1159,6 +1178,7 @@ SnpInitialize ( if (PhySoftReset (LanDriver) < 0) { Snp->Mode->State = EfiSimpleNetworkStopped; DEBUG((EFI_D_WARN, "LAN91x: PHY soft reset timeout\n"));

• gBS->CloseEvent (LanDriver->Snp.WaitForPacket); ReturnUnlock (EFI_NOT_STARTED); }

@@ -1643,11 +1663,12 @@ SnpGetStatus ( OUT VOID **TxBuff OPTIONAL ) {

• LAN91X_DRIVER *LanDriver;
• EFI_TPL SavedTpl;
• EFI_STATUS Status;
• BOOLEAN MediaPresent;
• UINT8 IstReg;

• LAN91X_DRIVER *LanDriver;
• EFI_TPL SavedTpl;
• EFI_STATUS Status;
• BOOLEAN MediaPresent;
• UINT8 IstReg;
• MSK_LINKED_SYSTEM_BUF *m_head;

That variable name is not in keeping with the style in this file. How about something like LinkedSystemBuf (as used elsewhere)?

I'll fix the name.

...

...

// Check preliminaries
if (Snp == NULL) {

@@ -1689,8 +1710,21 @@ SnpGetStatus ( }

// Pass back the completed buffer address

• // The transmit buffer status is not read when TxBuf is NULL if (TxBuff != NULL) {

• *TxBuff = TxQueRemove (LanDriver);

• *((UINT8 **) TxBuff) = (UINT8 *) 0;
• if( !IsListEmpty (&LanDriver->TransmitQueueHead))
• {

•  m_head = CR (GetFirstNode (&LanDriver->TransmitQueueHead),
  

MSK_LINKED_SYSTEM_BUF, Link, TX_MBUF_SIGNATURE);

•  if(m_head != NULL) {
  

•    *TxBuff = m_head->SystemBuf.Buf;
  

•    RemoveEntryList (&m_head->Link);
  

•    FreePool (m_head);
  

Looking at this code, at first I thought you were freeing the entire linked list, not just the first element. So I think a rename of m_head would indeed be beneficial here.

Ok.

...

...

•  }
  

• }
• // if (TxBuff != NULL) {
• // *TxBuff = TxQueRemove (LanDriver);

You shouldn't add commented out code.

Missed it. It will be removed.

...

...

}

// Update the media status

@@ -1733,6 +1767,8 @@ SnpTransmit ( UINTN Retries; UINT16 Proto; UINT8 PktNum;

• MSK_LINKED_SYSTEM_BUF *LinkedSystemBuf;

• // Check preliminaries if ((Snp == NULL) || (BufAddr == NULL)) {

@@ -1818,12 +1854,18 @@ SnpTransmit ( PktNum &= ARR_PACKET;

// Check for the nature of the frame

• if (DstAddr->Addr[0] == 0xFF) {
• LanDriver->Stats.TxBroadcastFrames += 1;
• } else if ((DstAddr->Addr[0] & 0x1) == 1) {
• LanDriver->Stats.TxMulticastFrames += 1;

• // If no destination address, it's ARP broadcast
• if(DstAddr != NULL)
• {
• if (DstAddr->Addr[0] == 0xFF) {

•  LanDriver->Stats.TxBroadcastFrames += 1;
  

• } else if ((DstAddr->Addr[0] & 0x1) == 1) {

•  LanDriver->Stats.TxMulticastFrames += 1;
  

• } else {

•  LanDriver->Stats.TxUnicastFrames += 1;
  

• } } else {

• LanDriver->Stats.TxUnicastFrames += 1;

• LanDriver->Stats.TxBroadcastFrames += 1; }

  // Set the Packet Number and Pointer registers

@@ -1877,14 +1919,27 @@ SnpTransmit ( ReturnUnlock (EFI_DEVICE_ERROR); }

• // Update the Rx statistics

• // Update the Tx statistics LanDriver->Stats.TxTotalBytes += BufSize; LanDriver->Stats.TxGoodFrames += 1;

  // Update the Tx Buffer cache

• if (!TxQueInsert (LanDriver, BufAddr)) {
• DEBUG((EFI_D_WARN, "LAN91x: SnpTransmit(): TxQueue insert

failure.\n"));

• }

• LinkedSystemBuf = AllocateZeroPool (sizeof (MSK_LINKED_SYSTEM_BUF));
• if (LinkedSystemBuf == NULL) {
• return EFI_OUT_OF_RESOURCES;
• }
• LinkedSystemBuf->Signature = TX_MBUF_SIGNATURE;
• //
• // Add the passed Buffer to the transmit queue. Don't copy.
• //
• LinkedSystemBuf->SystemBuf.Buf = BufAddr;
• LinkedSystemBuf->SystemBuf.Length = BufSize;
• InsertTailList (&LanDriver->TransmitQueueHead,

&LinkedSystemBuf->Link);

• // if (!TxQueInsert (LanDriver, BufAddr)) {
• // DEBUG((EFI_D_WARN, "LAN91x: SnpTransmit(): TxQueue insert

failure.\n"));

• // }

• Status = EFI_SUCCESS;

  // Dump the packet header

@@ -1941,7 +1996,7 @@ SnpReceive ( // Serialize access to data and registers SavedTpl = gBS->RaiseTPL (LAN91X_TPL);

• // Check that driver was started and initialised

• // Check that driver was started and initialized

Changing spelling from English to US English seems a bit unnecessary :-O The file is already littered with both variants. Maybe you should create a separate patch to make them all consistent if it bothers you?

Ah ... i did not realize that there is a nice mix of both spellings :) . It does not bother me at all. I'll switch it back.

...

...

switch (Snp->Mode->State) {
case EfiSimpleNetworkInitialized:
  break;

@@ -2104,7 +2159,6 @@ exit_unlock: return Status; }

• /*------------------ Driver Execution Environment main entry point

------------------*/

/* @@ -2155,9 +2209,12 @@ Lan91xDxeEntry ( PrintPhyRegisters (LanDriver); #endif

• // Initialize transmit queue
• InitializeListHead (&LanDriver->TransmitQueueHead);
• // Assign fields and func pointers Snp->Revision = EFI_SIMPLE_NETWORK_PROTOCOL_REVISION;

• Snp->WaitForPacket = NULL;

• Snp->WaitForPacket = SnpWaitForPacketNotify; Snp->Initialize = SnpInitialize; Snp->Start = SnpStart; Snp->Stop = SnpStop;

-- 2.7.4

---

Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi

---

Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi