From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web11.21514.1594738146972561085 for ; Tue, 14 Jul 2020 07:49:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=CW1AnhyI; spf=pass (domain: nuviainc.com, ip: 209.85.221.65, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f65.google.com with SMTP id j4so22175233wrp.10 for ; Tue, 14 Jul 2020 07:49:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=j8SYhvaenw/9XCpbYFsN7c80S+j9K/jeSBc0WNCgfis=; b=CW1AnhyIvKwy8puH7O4B1TuiaC58dDQrv/qAnjrcI0fjv2dSBwAQ/OtTa0wgc/bwF9 XljVelzTgiLo6YEEhcI/ZsdDnhpIw8KveZ3Xu0ViavVCCb6qs12YQZhBeisOSaJ5/zs9 rHa5nKK1OQbIS/bM0DaXoiyWz9gd/0BGJ11OGqfDUZZ4xAj4Rzaxn+xpxslZGrHEEq5E y29rbLVisIN9Wau2oUCSl2DO0oPJxQJMyfgRVEOcnGVfTiOPo+G0e3rkqHrqtAQxMokU 4urp9/pG39wrujhx5+9uZJQXja1S5pe6QF+ePoMsjTnNVqnSzXUBxwKdtf35VwMnwGmF uJMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=j8SYhvaenw/9XCpbYFsN7c80S+j9K/jeSBc0WNCgfis=; b=HOUL5L/ygEmO9RwAa24Wjiv6Hbcl9TTmQi7NwSqLNbMiZfQYbX19LCquTrftsqYa1n dQVJc50SvgeugrH9HW6hgaZXDMqZU0/QX774/q3Ywq4r/b/Ho+PYZ3Jpe9t3gB6CH80u WBoVqs3wYNac9PmJbUPI4EKvzUIfhbYdziGhIhBKSGCNXQKrWcxEHYC4ddDZ+2IPkp9z OK2ZfEKhn6Wo7WOrYSzFX+lfAymzQRaVl1p1kn4IXjpzF11DxO20UnEUD7ov1IXiB+JJ 4IYSWank6RsWXHYU1LUSW1vpRvJUxRxYjw3Y9Q4oKXWhU56HG5deJS1cdXC9XDVXhafK eXeA== X-Gm-Message-State: AOAM533jqUGXubtVTtmk+MwPkRBwdS83MU72A3vIQM4vQ5/OOTYPtiIu s7GhEDDByQ/Lz6wsQUewOg30ng== X-Google-Smtp-Source: ABdhPJzdM3B9gcTDraQ1S5MiZopk0WGW+0u9Q/XofxnHRQoFNQUuBsoRHhDfHg+1OfldN/d06pxcog== X-Received: by 2002:a5d:684f:: with SMTP id o15mr6264163wrw.148.1594738145552; Tue, 14 Jul 2020 07:49:05 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id p14sm31683118wrj.14.2020.07.14.07.49.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jul 2020 07:49:05 -0700 (PDT) Date: Tue, 14 Jul 2020 15:49:03 +0100 From: "Leif Lindholm" To: Pete Batard Cc: Andrei Warkentin , devel@edk2.groups.io, ard.biesheuvel@arm.com, philmd@redhat.com Subject: Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied Message-ID: <20200714144903.GZ12303@vanye> References: <20200712042850.117715-1-andrey.warkentin@gmail.com> <20200714133608.GX12303@vanye> <91646163-e88f-bd10-6c18-339cd06b4575@akeo.ie> MIME-Version: 1.0 In-Reply-To: <91646163-e88f-bd10-6c18-339cd06b4575@akeo.ie> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jul 14, 2020 at 14:51:46 +0100, Pete Batard wrote: > On 2020.07.14 14:36, Leif Lindholm wrote: > > On Mon, Jul 13, 2020 at 12:25:18 +0100, Pete Batard wrote: > > > One very minor formatting issue inline (that can be sorted during > > > integration): > > > > I would also like to prepend "Silicon/" to the subject line. > > Please do. It should have been there in the first place. Thanks. With those minor tweaks: Reviewed-by: Leif Lindholm Pushed as 2242b990248c. > Regards, > > /Pete > > > If I can do those two things: > > Reviewed-by: Leif Lindholm > > > > > On 2020.07.12 05:28, Andrei Warkentin wrote: > > > > This was originally a bit sloppy, and could hypothetically under heavy > > > > load result in a buffer being overwritten by hardware before the received > > > > buffer is copied. > > > > > > > > Signed-off-by: Andrei Warkentin > > > > --- > > > > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h | 15 +++++ > > > > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c | 59 +++++++++++++++----- > > > > Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c | 26 ++++++--- > > > > 3 files changed, 77 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h > > > > index 1a117b25..b39a1326 100644 > > > > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h > > > > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/BcmGenetDxe.h > > > > @@ -358,6 +358,16 @@ GenetTxIntr ( > > > > OUT VOID **TxBuf > > > > ); > > > > +UINT32 > > > > +GenetRxPending ( > > > > + IN GENET_PRIVATE_DATA *Genet > > > > + ); > > > > + > > > > +UINT32 > > > > +GenetTxPending ( > > > > + IN GENET_PRIVATE_DATA *Genet > > > > + ); > > > > + > > > > EFI_STATUS > > > > GenetRxIntr ( > > > > IN GENET_PRIVATE_DATA *Genet, > > > > @@ -365,4 +375,9 @@ GenetRxIntr ( > > > > OUT UINTN *FrameLength > > > > ); > > > > +VOID > > > > +GenetRxComplete ( > > > > + IN GENET_PRIVATE_DATA *Genet > > > > + ); > > > > + > > > > #endif /* GENET_UTIL_H__ */ > > > > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c > > > > index 1c4c8527..a0097b0d 100644 > > > > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c > > > > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c > > > > @@ -661,6 +661,7 @@ GenetDmaMapRxDescriptor ( > > > > Genet->RxBufferMap[DescIndex].PhysAddress & 0xFFFFFFFF); > > > > GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex), > > > > (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0xFFFFFFFF); > > > > + GenetMmioWrite (Genet, GENET_RX_DESC_STATUS (DescIndex), 0); > > > > return EFI_SUCCESS; > > > > } > > > > @@ -753,12 +754,9 @@ GenetTxIntr ( > > > > OUT VOID **TxBuf > > > > ) > > > > { > > > > - UINT32 ConsIndex, Total; > > > > + UINT32 Total; > > > > - ConsIndex = GenetMmioRead (Genet, > > > > - GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > > > > - > > > > - Total = (ConsIndex - Genet->TxConsIndex) & 0xFFFF; > > > > + Total = GenetTxPending (Genet); > > > > if (Genet->TxQueued > 0 && Total > 0) { > > > > DmaUnmap (Genet->TxBufferMap[Genet->TxNext]); > > > > *TxBuf = Genet->TxBuffer[Genet->TxNext]; > > > > @@ -770,6 +768,46 @@ GenetTxIntr ( > > > > } > > > > } > > > > +UINT32 > > > > +GenetRxPending ( > > > > + IN GENET_PRIVATE_DATA *Genet > > > > + ) > > > > +{ > > > > + UINT32 ProdIndex; > > > > + UINT32 ConsIndex; > > > > + > > > > + ConsIndex = GenetMmioRead (Genet, > > > > + GENET_RX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > > > > + ASSERT (ConsIndex == Genet->RxConsIndex); > > > > + > > > > + ProdIndex = GenetMmioRead (Genet, > > > > + GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > > > > + return (ProdIndex - Genet->RxConsIndex) & 0xFFFF; > > > > +} > > > > + > > > > +UINT32 > > > > +GenetTxPending ( > > > > + IN GENET_PRIVATE_DATA *Genet > > > > + ) > > > > +{ > > > > + UINT32 ConsIndex; > > > > + > > > > + ConsIndex = GenetMmioRead (Genet, > > > > + GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > > > > > > If we want to be pedantic about EDK2 formatting, this should be indented to > > > start after the "Ge" of GenetMmioRead above, like the previous calls. > > > > > > > + > > > > + return (ConsIndex - Genet->TxConsIndex) & 0xFFFF; > > > > +} > > > > + > > > > +VOID > > > > +GenetRxComplete ( > > > > + IN GENET_PRIVATE_DATA *Genet > > > > + ) > > > > +{ > > > > + Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0xFFFF; > > > > + GenetMmioWrite (Genet, GENET_RX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE), > > > > + Genet->RxConsIndex); > > > > +} > > > > + > > > > /** > > > > Simulate an "RX interrupt", returning the index of a completed RX buffer and > > > > corresponding frame length. > > > > @@ -790,21 +828,14 @@ GenetRxIntr ( > > > > ) > > > > { > > > > EFI_STATUS Status; > > > > - UINT32 ProdIndex, Total; > > > > + UINT32 Total; > > > > UINT32 DescStatus; > > > > - ProdIndex = GenetMmioRead (Genet, > > > > - GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0xFFFF; > > > > - > > > > - Total = (ProdIndex - Genet->RxConsIndex) & 0xFFFF; > > > > + Total = GenetRxPending (Genet); > > > > if (Total > 0) { > > > > *DescIndex = Genet->RxConsIndex % GENET_DMA_DESC_COUNT; > > > > DescStatus = GenetMmioRead (Genet, GENET_RX_DESC_STATUS (*DescIndex)); > > > > *FrameLength = SHIFTOUT (DescStatus, GENET_RX_DESC_STATUS_BUFLEN); > > > > - > > > > - Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0xFFFF; > > > > - GenetMmioWrite (Genet, GENET_RX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE), > > > > - Genet->RxConsIndex); > > > > Status = EFI_SUCCESS; > > > > } else { > > > > Status = EFI_NOT_READY; > > > > diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > > > > index 371216ca..1bda18f1 100644 > > > > --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > > > > +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c > > > > @@ -502,9 +502,19 @@ GenetSimpleNetworkGetStatus ( > > > > Genet->SnpMode.MediaPresent = FALSE; > > > > } else { > > > > Genet->SnpMode.MediaPresent = TRUE; > > > > + } > > > > + > > > > + if (TxBuf != NULL) { > > > > + GenetTxIntr (Genet, TxBuf); > > > > + } > > > > - if (TxBuf != NULL) { > > > > - GenetTxIntr (Genet, TxBuf); > > > > + if (InterruptStatus != NULL) { > > > > + *InterruptStatus = 0; > > > > + if (GenetRxPending (Genet) > 0) { > > > > + *InterruptStatus |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; > > > > + } > > > > + if (GenetTxPending (Genet) > 0) { > > > > + *InterruptStatus |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; > > > > } > > > > } > > > > @@ -741,13 +751,8 @@ GenetSimpleNetworkReceive ( > > > > DEBUG ((DEBUG_ERROR, > > > > "%a: Buffer size (0x%X) is too small for frame (0x%X)\n", > > > > __FUNCTION__, *BufferSize, FrameLength)); > > > > - Status = GenetDmaMapRxDescriptor (Genet, DescIndex); > > > > - if (EFI_ERROR (Status)) { > > > > - DEBUG ((DEBUG_ERROR, "%a: Failed to remap RX descriptor!\n", > > > > - __FUNCTION__)); > > > > - } > > > > - EfiReleaseLock (&Genet->Lock); > > > > - return EFI_BUFFER_TOO_SMALL; > > > > + Status = EFI_BUFFER_TOO_SMALL; > > > > + goto out; > > > > } > > > > if (DestAddr != NULL) { > > > > @@ -773,11 +778,14 @@ GenetSimpleNetworkReceive ( > > > > Status = EFI_NOT_READY; > > > > } > > > > +out: > > > > Status = GenetDmaMapRxDescriptor (Genet, DescIndex); > > > > if (EFI_ERROR (Status)) { > > > > DEBUG ((DEBUG_ERROR, "%a: Failed to remap RX descriptor!\n", __FUNCTION__)); > > > > } > > > > + GenetRxComplete (Genet); > > > > + > > > > EfiReleaseLock (&Genet->Lock); > > > > return Status; > > > > } > > > > > > > > > > Reviewed-by: Pete Batard >