* [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
@ 2020-07-12 4:28 Andrei Warkentin
2020-07-13 11:25 ` Pete Batard
0 siblings, 1 reply; 6+ messages in thread
From: Andrei Warkentin @ 2020-07-12 4:28 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif, pete, philmd
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;
+
+ 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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
2020-07-12 4:28 [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied Andrei Warkentin
@ 2020-07-13 11:25 ` Pete Batard
2020-07-14 12:03 ` Leif Lindholm
2020-07-14 13:36 ` Leif Lindholm
0 siblings, 2 replies; 6+ messages in thread
From: Pete Batard @ 2020-07-13 11:25 UTC (permalink / raw)
To: Andrei Warkentin, devel; +Cc: ard.biesheuvel, leif, philmd
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 <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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
2020-07-13 11:25 ` Pete Batard
@ 2020-07-14 12:03 ` Leif Lindholm
2020-07-14 13:36 ` Leif Lindholm
1 sibling, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2020-07-14 12:03 UTC (permalink / raw)
To: Pete Batard; +Cc: Andrei Warkentin, devel, ard.biesheuvel, philmd
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 <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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
2020-07-13 11:25 ` Pete Batard
2020-07-14 12:03 ` Leif Lindholm
@ 2020-07-14 13:36 ` Leif Lindholm
2020-07-14 13:51 ` Pete Batard
1 sibling, 1 reply; 6+ messages in thread
From: Leif Lindholm @ 2020-07-14 13:36 UTC (permalink / raw)
To: Pete Batard, Andrei Warkentin; +Cc: devel, ard.biesheuvel, philmd
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
2020-07-14 13:36 ` Leif Lindholm
@ 2020-07-14 13:51 ` Pete Batard
2020-07-14 14:49 ` Leif Lindholm
0 siblings, 1 reply; 6+ messages in thread
From: Pete Batard @ 2020-07-14 13:51 UTC (permalink / raw)
To: Leif Lindholm, Andrei Warkentin; +Cc: devel, ard.biesheuvel, philmd
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
2020-07-14 13:51 ` Pete Batard
@ 2020-07-14 14:49 ` Leif Lindholm
0 siblings, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2020-07-14 14:49 UTC (permalink / raw)
To: Pete Batard; +Cc: Andrei Warkentin, devel, ard.biesheuvel, philmd
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 <leif@nuviainc.com>
Pushed as 2242b990248c.
> 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>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-14 14:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-12 4:28 [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied Andrei Warkentin
2020-07-13 11:25 ` Pete Batard
2020-07-14 12:03 ` Leif Lindholm
2020-07-14 13:36 ` Leif Lindholm
2020-07-14 13:51 ` Pete Batard
2020-07-14 14:49 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox