From patchwork Tue Aug 30 06:49:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 74940 Delivered-To: patch@linaro.org Received: by 10.140.29.52 with SMTP id a49csp1989720qga; Mon, 29 Aug 2016 23:49:56 -0700 (PDT) X-Received: by 10.200.46.25 with SMTP id r25mr2258109qta.114.1472539796179; Mon, 29 Aug 2016 23:49:56 -0700 (PDT) Return-Path: Received: from lists.linaro.org (lists.linaro.org. [54.225.227.206]) by mx.google.com with ESMTP id 7si14122266qki.72.2016.08.29.23.49.54; Mon, 29 Aug 2016 23:49:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linaro-uefi-bounces@lists.linaro.org designates 54.225.227.206 as permitted sender) client-ip=54.225.227.206; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linaro-uefi-bounces@lists.linaro.org designates 54.225.227.206 as permitted sender) smtp.mailfrom=linaro-uefi-bounces@lists.linaro.org; dmarc=pass (p=NONE dis=NONE) header.from=linaro.org Received: by lists.linaro.org (Postfix, from userid 109) id 1B37E60EEE; Tue, 30 Aug 2016 06:49:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on ip-10-142-244-252 X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=disabled version=3.4.0 Received: from [127.0.0.1] (localhost [127.0.0.1]) by lists.linaro.org (Postfix) with ESMTP id E18B160E65; Tue, 30 Aug 2016 06:49:47 +0000 (UTC) X-Original-To: linaro-uefi@lists.linaro.org Delivered-To: linaro-uefi@lists.linaro.org Received: by lists.linaro.org (Postfix, from userid 109) id 4D16960E7A; Tue, 30 Aug 2016 06:49:46 +0000 (UTC) Received: from mail-it0-f45.google.com (mail-it0-f45.google.com [209.85.214.45]) by lists.linaro.org (Postfix) with ESMTPS id 1193560E2A for ; Tue, 30 Aug 2016 06:49:45 +0000 (UTC) Received: by mail-it0-f45.google.com with SMTP id e63so146892976ith.1 for ; Mon, 29 Aug 2016 23:49:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=VfJS7E1rn9S5VLS81JP+UQkV5jcgrXfUWcO2FsvDXL8=; b=im2yhkO5gs4BqJCfMBrtAUrzz9l4ra7LprZQaAIakVdTCs9ArSHJEU5VDAt5M1M2Cf dHdduYrnMnBpMz8V56i2kWa5OehCnc7xR0KWHR5ulFaHvxhf3xO1qV+1MUKmsaxIZMtb QZiqTzQwipWhwIwrTUF0fZTSFYSDXENjH7Y0gDwdCfTx84su8jaVaFa7MlBpoVHDDUTD uj9AZTDsbh/zseChHvQhDX9mFg3orVBR7JS6gJ+c4sIFS8SeQkphM9xNj+W5OtOQ0l0w 0AWFgnw0hVKKrpNMKCVH86rsWJTiPLW4KEjauiWxoRkPd1FX5+S8hsDnZK3gUtPyP4fZ Vmdg== X-Gm-Message-State: AE9vXwMEIIME/g2ZEeijllHjyQ6jJLW288HNBPxgOwiI0TWsQUUm5no3IZk6ncPGP4O7s3dAUsIQJwCOvXjxAgi48RA= X-Received: by 10.36.57.215 with SMTP id l206mr23090351ita.5.1472539784478; Mon, 29 Aug 2016 23:49:44 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 29 Aug 2016 23:49:43 -0700 (PDT) In-Reply-To: <57C4F68E.5020600@softiron.co.uk> References: <1471132113-9684-1-git-send-email-alan@softiron.co.uk> <1472497359-23612-1-git-send-email-alan@softiron.co.uk> <1472497359-23612-2-git-send-email-alan@softiron.co.uk> <57C4F68E.5020600@softiron.co.uk> From: Ard Biesheuvel Date: Tue, 30 Aug 2016 07:49:43 +0100 Message-ID: To: Alan Ott Cc: Linaro UEFI Mailman List Subject: Re: [Linaro-uefi] [PATCH v6 1/5] Drivers/Net/MarvellYukon: Don't re-use DMA buffers X-BeenThere: linaro-uefi@lists.linaro.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linaro-uefi-bounces@lists.linaro.org Sender: "Linaro-uefi" On 30 August 2016 at 03:59, Alan Ott wrote: > On 08/29/2016 03:34 PM, Ard Biesheuvel wrote: >> >> On 29 August 2016 at 20:02, Alan Ott wrote: >>> >>> Change the receive buffers to be single-use only. This involves a few >>> changes that work together: >>> 1. Change msk_newbuf() to not attempt to re-use or free a buffer, >>> 2. Change msk_rxeof() free the buffer when it is done with it, >>> 3. Store a temporary copy of the received data for passing to >>> the Receive Queue, >>> 4. 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: >>> 1. The optimization failed to work for 64-bit DMA transfers; >>> 2. 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 >>> --- >>> 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? Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi 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 -#define MAX_SUPPORTED_PACKET_SIZE EFI_PAGE_SIZE +#define MAX_SUPPORTED_PACKET_SIZE (1500) EFI_STATUS mskc_probe (EFI_PCI_IO_PROTOCOL *PciIo); _______________________________________________