From: "Leif Lindholm" <leif@nuviainc.com>
To: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin <andrey.warkentin@gmail.com>,
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
Date: Tue, 14 Jul 2020 15:49:03 +0100 [thread overview]
Message-ID: <20200714144903.GZ12303@vanye> (raw)
In-Reply-To: <91646163-e88f-bd10-6c18-339cd06b4575@akeo.ie>
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>
>
prev parent reply other threads:[~2020-07-14 14:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200714144903.GZ12303@vanye \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox