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.
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?
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 receive buffer 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 DMA buffer\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?
EFI_STATUS mskc_probe (EFI_PCI_IO_PROTOCOL *PciIo);
-- 2.7.4
Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi