On 24 September 2016 at 02:00, Daniil Egranov daniil.egranov@arm.com wrote:
On 09/23/2016 03:10 AM, Ard Biesheuvel wrote:
(+ Alan)
On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:
- Corrected Ethernet frame size to 1538.
- Performance optimization. Removed extra CopyMem() call.
Could you split this in 2 patches please?
Will do.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com
NOTE: The driver requires [edk2] [PATCH 0/2] Juno PCI fixes
Drivers/Net/MarvellYukonDxe/if_msk.c | 22 +++++++++------------- Drivers/Net/MarvellYukonDxe/if_msk.h | 2 +- 2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 6fb8af4..8e9b726 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1876,7 +1876,6 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;
MSK_DMA_BUF m;
DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n"));
@@ -1906,16 +1905,6 @@ msk_rxeof ( rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; #endif
- gBS->CopyMem (&m, &rxd->rx_m, sizeof(m));
- Status = msk_newbuf (sc_if, cons);
- if (EFI_ERROR (Status)) {
// This is a dropped packet, but we aren't counting drops// Reuse old buffermsk_discard_rxbuf (sc_if, cons);What happened to dropping the packet in this case?
I am going to review this part again. I guess we have an assumption that the driver will drop received messages in case of memory allocation issue but keep going.
No, it allocates a new buffer (and hands it to the device) before handling the contents of the old buffer. The old buffer is simply reused by the device, and not processed any further, if the packet needs to be dropped for any reason.
If you want to get rid of the gBS->CopyMem(), why not simply assign 'rxd->rx_m' to 'm' field by field? That should help the compiler optimize away the in-memory version of 'm' entirely
Will copping the receive buffer pointer to the queue buffer pointer be a more optimal way?
No, you need 'm' to store the values between the time that you allocate a new buffer for the device and process the contents of the old one.
break;- }
Status = mPciIo->Flush (mPciIo); if (EFI_ERROR (Status)) { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n"));@@ -1932,14 +1921,21 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE;
m_link->SystemBuf.Buf = m.Buf;
m_link->SystemBuf.Buf = rxd->rx_m.Buf; m_link->SystemBuf.Length = len; InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receivebuffer link. Dropping Frame\n"));
gBS->FreePool (m.Buf);
gBS->FreePool (rxd->rx_m.Buf); }- Status = msk_newbuf (sc_if, cons);
- if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMAbuffer\n"));
break;}
} while (0);
MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index 66513d3..64ddeac 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.h +++ b/Drivers/Net/MarvellYukonDxe/if_msk.h @@ -15,7 +15,7 @@
#include <Uefi.h>
-#define MAX_SUPPORTED_PACKET_SIZE (1500) +#define MAX_SUPPORTED_PACKET_SIZE (1538)
This looks reasonable, but could you still explain why you need to make this change?
With 1500 size tftp transfer gets messed up. I missed UDP part here. The driver receives Layer 2 Ethernet frame with 18 (Ethernet) + 20 (IPv4) + 8 (UDP) + 1500 (max payload) = 1546. If driver will support jumbo frames, it should be changed accordingly. Do we care about frame size for IPv6 case?
IPv6 yes. Jumbo frames no, since, looking at the original driver, this is not trivial to implement.