[edk2-devel] [edk2][PATCH 1/1] BcmGenetDxe: don't consume RX buffer until it's actually copied
Pete Batard
pete at akeo.ie
Tue Jul 14 13:51:46 UTC 2020
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 at 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 at 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 at akeo.ie>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#62502): https://edk2.groups.io/g/devel/message/62502
Mute This Topic: https://groups.io/mt/75452213/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list