On 30 August 2016 at 03:59, Alan Ott alan@softiron.co.uk wrote:
On 08/29/2016 03:34 PM, Ard Biesheuvel wrote:
On 29 August 2016 at 20:02, Alan Ott alan@softiron.co.uk wrote:
Change the receive buffers to be single-use only. This involves a few changes that work together:
- Change msk_newbuf() to not attempt to re-use or free a buffer,
- Change msk_rxeof() free the buffer when it is done with it,
- Store a temporary copy of the received data for passing to the Receive Queue,
- Call the required Flush() and Unmap() on the DMA buffer.
In addition this means failure to allocate a new buffer is a failure of msk_rxeof().
Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:
- The optimization failed to work for 64-bit DMA transfers;
- The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.
Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk
Drivers/Net/MarvellYukonDxe/if_msk.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..ad7a1c2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (
rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
- if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
- return EFI_ALREADY_STARTED;
- } else if (rxd->rx_m.Buf != NULL) {
- mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
- mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length),
rxd->rx_m.Buf);
- }
- Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages,
EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;
MSK_DMA_BUF m;
DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n"));
@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }
- rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
- m = rxd->rx_m;
Struct assignment is not allowed in EDK2 (but I haven't checked the existing code).
I had a look in the EDKII C Coding Standards 2.1 Draft PDF here: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications and didn't see it, but maybe I'm looking at the wrong thing.
I will change this to
gBS->CopyMem (&m, &rxd->rx_m, sizeof(MSK_DMA_BUF));
when merging the patch, unless there is a pressing reason not to.
Feel free, but: gBS->CopyMem (&m, &rxd->rx_m, sizeof(m)); is a little safer (sizeof(m) instead of sizeof(MSK_DMA_BUF)), but of course the struct assignment method is the safest (with compile-time checks for equivalent types and automatic determination of size (which already helped me once today)).
Whatever you feel needs to be done is fine.
OK
Status = msk_newbuf (sc_if, cons); if (EFI_ERROR (Status)) {
// This is a dropped packet, but we aren't counting drops // Reuse old buffer msk_discard_rxbuf (sc_if, cons);break;- }
- Status = mPciIo->Flush (mPciIo);
- if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n")); }- Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
- if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Unmap DMA\n"));- }
Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);
rxd = &sc_if->msk_cdata.msk_rxdesc[cons];m_link->Signature = RX_MBUF_SIGNATURE; Status = gBS->AllocatePool (EfiBootServicesData, len, (VOID**) &m_link->DmaBuf.Buf); if(!EFI_ERROR (Status)) {gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); m_link->DmaBuf.Length = len;
m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping;
m_link->DmaBuf.DmaMapping = m.DmaMapping;Do we still need DmaMapping after we unmapped the buffer?
Certainly not, but the consumer of that queue doesn't do anything with the DMA mapping, which is why it didn't cause an issue, even before my patches.
The Receive Queue should use a MSK_LINKED_SYSTEM_BUF instead of a MSK_LINKED_DMA_BUF. I made a patch for this, which I will send tomorrow (Tuesday) as part of v7.
OK, but before you do that, could we get rid of the PciIo->AllocateBuffer() and the redundant copy as well? Something like below, but in a way that it doesn't leak memory?
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9095ccd1744d..bf0f6d091ccb 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,7 +581,8 @@ msk_newbuf (
rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
- Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); + Status = gBS->AllocatePool (EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), + &Buffer); if (EFI_ERROR (Status)) { return Status; } @@ -590,7 +591,7 @@ msk_newbuf ( Status = mPciIo->Map (mPciIo, EfiPciIoOperationBusMasterWrite, Buffer, &Length, &PhysAddr, &Mapping); if (EFI_ERROR (Status)) { Length = MAX_SUPPORTED_PACKET_SIZE; - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (Length), Buffer); + gBS->FreePool (Buffer); return Status; }
@@ -1608,7 +1609,7 @@ msk_txrx_dma_free ( mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); // Free Rx buffers as we own these if(rxd->rx_m.Buf != NULL) { - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf); + gBS->FreePool (rxd->rx_m.Buf); rxd->rx_m.Buf = NULL; } gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); @@ -1891,19 +1892,11 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE; - Status = gBS->AllocatePool (EfiBootServicesData, - len, - (VOID**) &m_link->DmaBuf.Buf); - if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); - m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = m.DmaMapping; - - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); + m_link->DmaBuf.Buf = m.Buf; + m_link->DmaBuf.Length = len; + m_link->DmaBuf.DmaMapping = m.DmaMapping;
- InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); - } else { - DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA buffer. Dropping Frame\n")); + InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n")); diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index a5025a39ecf3..66513d3c54c1 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 EFI_PAGE_SIZE +#define MAX_SUPPORTED_PACKET_SIZE (1500)
EFI_STATUS mskc_probe (EFI_PCI_IO_PROTOCOL *PciIo);