Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied

["Leif Lindholm" <leif@nuviainc.com>]

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.
If I can do those two things:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> 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 <andrey.warkentin@gmail.com>
> > ---
> >   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 <pete@akeo.ie>