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

["Pete Batard" <pete@akeo.ie>]

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.

Regards,

/Pete

> 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>