On 08/26/2016 02:36 PM, Daniil Egranov wrote:
On 08/26/2016 12:49 PM, Alan Ott wrote:
On 08/26/2016 12:34 PM, Daniil Egranov wrote:
Hi Alan,
On 08/25/2016 05:39 PM, Alan Ott 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 | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 7979bf3..573dea5 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; }
- m = rxd->rx_m; 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_ERROR, "Marvell Yukon: failed to Flush DMA\n"));This is network related error message, please change the debug level to EFI_D_NET.
}
- Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
- if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Unmap DMA\n"));This is network related error message, please change the debug level to EFI_D_NET.
These are PCI/DMA-related. Other places in the driver, errors of this nature are reported as EFI_D_ERROR. It's up to you guys though.
See also the error messages: "failed to allocate DMA'able", and "failed to map DMA'able" in if_msk.c, which are EFI_D_ERROR. I just followed what was already there, which I still think is right.
Let me know.
I am trying to keep EFI_D_ERROR for the initialization code and for general sanity checking (i may missed some places). This code will be called constantly during the network operations so in case of error, it will spam console terminal with errors. That will be interesting for people who is debugging network functionality but not for others so i would like to limit it to EFI_D_NET so this type of messages have to be explicitly enabled.
Ok. No problem. Be on the lookout for v4.
Alan.