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

2020-07-14 Thread Leif Lindholm 
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 & 0x);
> > > >  GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex),
> > > >(Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0x);
> > > > +  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)) & 
> > > > 0x;
> > > > -
> > > > -  Total = (ConsIndex - Genet->TxConsIndex) & 0x;
> > > > +  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)) & 
> > > > 0x;
> > > > +  ASSERT (ConsIndex == Genet->RxConsIndex);
> > > > +
> > > > +  ProdIndex = GenetMmioRead (Genet,
> > > > +GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 
> > > > 0x;
> > > > +  return (ProdIndex - Genet->RxConsIndex) & 0x;
> > > > +}
> > > > +
> > > > +UINT32
> > > > +GenetTxPending (
> > > > +  IN  GENET_PRIVATE_DATA *Genet
> > > > +  )
> > > > +{
> > > > +  UINT32 ConsIndex;
> > > > +
> > > > +  ConsIndex = GenetMmioRead (Genet,
> > > > + GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
> > > 
> > > 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) & 0x;
> > > > +}
> > > > +
> > > > +VOID
> > > > +GenetRxComplete (
> > > > +  IN GENET_PRIVATE_DATA *Genet
> > > > +  )
> > > > +{
> > > > +  Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0x;
> > > > +  GenetMmioWrite (Genet, GENET_RX_DMA_CONS_INDEX 
> > > > (GENET_DMA_DEFAULT_QUEUE),
> > > > +

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

2020-07-14 Thread Pete Batard 

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 


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 & 0x);
 GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex),
   (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0x);
+  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)) & 0x;
-
-  Total = (ConsIndex - Genet->TxConsIndex) & 0x;
+  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)) & 0x;
+  ASSERT (ConsIndex == Genet->RxConsIndex);
+
+  ProdIndex = GenetMmioRead (Genet,
+GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
+  return (ProdIndex - Genet->RxConsIndex) & 0x;
+}
+
+UINT32
+GenetTxPending (
+  IN  GENET_PRIVATE_DATA *Genet
+  )
+{
+  UINT32 ConsIndex;
+
+  ConsIndex = GenetMmioRead (Genet,
+ GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;


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) & 0x;
+}
+
+VOID
+GenetRxComplete (
+  IN GENET_PRIVATE_DATA *Genet
+  )
+{
+  Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0x;
+  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_STATUSStatus;
-  UINT32ProdIndex, Total;
+  UINT32Total;
 UINT32DescStatus;
-  ProdIndex = GenetMmioRead (Genet,
-GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
-
-  Total = (ProdIndex - Genet->RxConsIndex) & 0x;
+  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) & 0x;
-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

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

2020-07-14 Thread Leif Lindholm 
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 

> 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 & 0x);
> > GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex),
> >   (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0x);
> > +  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)) & 
> > 0x;
> > -
> > -  Total = (ConsIndex - Genet->TxConsIndex) & 0x;
> > +  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)) & 
> > 0x;
> > +  ASSERT (ConsIndex == Genet->RxConsIndex);
> > +
> > +  ProdIndex = GenetMmioRead (Genet,
> > +GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 
> > 0x;
> > +  return (ProdIndex - Genet->RxConsIndex) & 0x;
> > +}
> > +
> > +UINT32
> > +GenetTxPending (
> > +  IN  GENET_PRIVATE_DATA *Genet
> > +  )
> > +{
> > +  UINT32 ConsIndex;
> > +
> > +  ConsIndex = GenetMmioRead (Genet,
> > + GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
> 
> 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) & 0x;
> > +}
> > +
> > +VOID
> > +GenetRxComplete (
> > +  IN GENET_PRIVATE_DATA *Genet
> > +  )
> > +{
> > +  Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0x;
> > +  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_STATUSStatus;
> > -  UINT32ProdIndex, Total;
> > +  UINT32Total;
> > UINT32DescStatus;
> > -  ProdIndex = GenetMmioRead (Genet,
> > -GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 
> > 0x;
> > -
> > -  Total = (ProdIndex - Genet->RxConsIndex) & 0x;
> > +  Total = GenetRxPending (Genet);
> > if (Total > 0) {
> >   *DescIndex = Genet->RxConsIndex % GENET_DMA_DESC_COUNT;
> >   DescStatus = GenetMmioRead (Genet, GENET_RX_DESC_STATUS (*DescIndex));
> >   *FrameLength = SHIFTOUT (DescStatus,

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

2020-07-14 Thread Leif Lindholm 
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):

Will do, thanks for highlighting (and review).

/
Leif

> 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 & 0x);
> > GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex),
> >   (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0x);
> > +  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)) & 
> > 0x;
> > -
> > -  Total = (ConsIndex - Genet->TxConsIndex) & 0x;
> > +  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)) & 
> > 0x;
> > +  ASSERT (ConsIndex == Genet->RxConsIndex);
> > +
> > +  ProdIndex = GenetMmioRead (Genet,
> > +GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 
> > 0x;
> > +  return (ProdIndex - Genet->RxConsIndex) & 0x;
> > +}
> > +
> > +UINT32
> > +GenetTxPending (
> > +  IN  GENET_PRIVATE_DATA *Genet
> > +  )
> > +{
> > +  UINT32 ConsIndex;
> > +
> > +  ConsIndex = GenetMmioRead (Genet,
> > + GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
> 
> 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) & 0x;
> > +}
> > +
> > +VOID
> > +GenetRxComplete (
> > +  IN GENET_PRIVATE_DATA *Genet
> > +  )
> > +{
> > +  Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0x;
> > +  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_STATUSStatus;
> > -  UINT32ProdIndex, Total;
> > +  UINT32Total;
> > UINT32DescStatus;
> > -  ProdIndex = GenetMmioRead (Genet,
> > -GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 
> > 0x;
> > -
> > -  Total = (ProdIndex - Genet->RxConsIndex) & 0x;
> > +  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 =

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

2020-07-13 Thread Pete Batard 
One very minor formatting issue inline (that can be sorted during 
integration):


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 & 0x);
GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex),
  (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0x);
+  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)) & 0x;
-
-  Total = (ConsIndex - Genet->TxConsIndex) & 0x;
+  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)) & 0x;
+  ASSERT (ConsIndex == Genet->RxConsIndex);
+
+  ProdIndex = GenetMmioRead (Genet,
+GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
+  return (ProdIndex - Genet->RxConsIndex) & 0x;
+}
+
+UINT32
+GenetTxPending (
+  IN  GENET_PRIVATE_DATA *Genet
+  )
+{
+  UINT32 ConsIndex;
+
+  ConsIndex = GenetMmioRead (Genet,
+ GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;


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) & 0x;
+}
+
+VOID
+GenetRxComplete (
+  IN GENET_PRIVATE_DATA *Genet
+  )
+{
+  Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0x;
+  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_STATUSStatus;
-  UINT32ProdIndex, Total;
+  UINT32Total;
UINT32DescStatus;
  
-  ProdIndex = GenetMmioRead (Genet,

-GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
-
-  Total = (ProdIndex - Genet->RxConsIndex) & 0x;
+  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) & 0x;
-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;
+  }
+
+

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

2020-07-11 Thread Andrei Warkentin 
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 & 0x);
   GenetMmioWrite (Genet, GENET_RX_DESC_ADDRESS_HI (DescIndex),
 (Genet->RxBufferMap[DescIndex].PhysAddress >> 32) & 0x);
+  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)) & 0x;
-
-  Total = (ConsIndex - Genet->TxConsIndex) & 0x;
+  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)) & 0x;
+  ASSERT (ConsIndex == Genet->RxConsIndex);
+
+  ProdIndex = GenetMmioRead (Genet,
+GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
+  return (ProdIndex - Genet->RxConsIndex) & 0x;
+}
+
+UINT32
+GenetTxPending (
+  IN  GENET_PRIVATE_DATA *Genet
+  )
+{
+  UINT32 ConsIndex;
+
+  ConsIndex = GenetMmioRead (Genet,
+ GENET_TX_DMA_CONS_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
+
+  return (ConsIndex - Genet->TxConsIndex) & 0x;
+}
+
+VOID
+GenetRxComplete (
+  IN GENET_PRIVATE_DATA *Genet
+  )
+{
+  Genet->RxConsIndex = (Genet->RxConsIndex + 1) & 0x;
+  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_STATUSStatus;
-  UINT32ProdIndex, Total;
+  UINT32Total;
   UINT32DescStatus;
 
-  ProdIndex = GenetMmioRead (Genet,
-GENET_RX_DMA_PROD_INDEX (GENET_DMA_DEFAULT_QUEUE)) & 0x;
-
-  Total = (ProdIndex - Genet->RxConsIndex) & 0x;
+  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) & 0x;
-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) {