Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus

["wangy" <wangyzhaoz@163.com>]

[-- Attachment #1: Type: text/plain, Size: 4453 bytes --]

Hi Pedro Falcato,


At 2023-07-25 16:45:01, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:
>On Tue, Jul 25, 2023 at 2:10 AM <wangyzhaoz@163.com> wrote:
>>
>> From: Yang Wang <wangyzhaoz@163.com>
>>
>> Check EmacGetDmaStatus input parameters
>> IrqStat may be a null pointer.
>>
>> Signed-off-by: Yang Wang <wangyzhaoz@163.com>
>> ---
>>  .../Drivers/DwEmacSnpDxe/DwEmacSnpDxe.c          |  7 +++++--
>>  .../Drivers/DwEmacSnpDxe/EmacDxeUtil.c           | 16 ++++++++++++----
>>  .../Drivers/DwEmacSnpDxe/EmacDxeUtil.h           |  2 +-
>>  3 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/DwEmacSnpDxe.c b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/DwEmacSnpDxe.c
>> index 4cb3371d79..6805511a1d 100755
>> --- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/DwEmacSnpDxe.c
>> +++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/DwEmacSnpDxe.c
>> @@ -847,9 +847,12 @@ SnpGetStatus (
>>    }
>>
>>    // Check DMA Irq status
>> -  EmacGetDmaStatus (IrqStat, Snp->MacBase);
>> +  Status = EmacGetDmaStatus (IrqStat, Snp->MacBase);
>> +  if (EFI_ERROR(Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a: error  Status: %r\n", __func__, Status));
>> +  }
>>
>> -  return EFI_SUCCESS;
>> +  return Status;
>>  }
>>
>>
>> diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> index 3b982ce984..45b5a05f51 100755
>> --- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> +++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> @@ -489,16 +489,22 @@ EmacDmaStart (
>>  }
>>
>>
>> -VOID
>> +EFI_STATUS
>>  EFIAPI
>>  EmacGetDmaStatus (
>>    OUT  UINT32   *IrqStat  OPTIONAL,
>>    IN   UINTN    MacBaseAddress
>>    )
>>  {
>> -  UINT32  DmaStatus;
>> -  UINT32  ErrorBit;
>> -  UINT32  Mask = 0;
>> +  UINT32        DmaStatus;
>> +  UINT32        ErrorBit;
>> +  UINT32        Mask = 0;
>> +  EFI_STATUS    Status = EFI_SUCCESS;
>> +
>> +  if (IrqStat == NULL) {
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto EXIT;
>> +  }
>
>This patch looks bogus to me. IrqStat is marked OPTIONAL, how can you

>error out if it isn't provided?


I foud in the MnpRecycleTxBuf(), it will pass NULL in 2nd parameter at code of 'Status=Snp ->GetStatus (Snp, NULL, (VOID * *)&TxBuf);'.
then in EmacGetDmaStatus(), it will not check this pointer and use directly, causig this problem (system hang). That's why I add above check.


>Also, please CC maintainers next time.


Thanks for remding.


However, I found the maintainer mailbox info in file platforms/Maintainers.txt, is not complete match to what you listed, I am a little bit confused

R: Ard Biesheuvel <ardb+tianocore@kernel.org> 
M: Leif Lindholm <quic_llindhol@quicinc.com>
May I ask if the maintainer information is obtained here?


Regards,


Yang


>
>>
>>    DmaStatus = MmioRead32 (MacBaseAddress +
>>                             DW_EMAC_DMAGRP_STATUS_OFST);
>> @@ -602,6 +608,8 @@ EmacGetDmaStatus (
>>    MmioOr32 (MacBaseAddress +
>>              DW_EMAC_DMAGRP_STATUS_OFST,
>>              Mask);
>> +EXIT:
>> +  return Status;
>>  }
>>
>>
>> diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.h b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.h
>> index c4c3653dc7..60f30ecd16 100755
>> --- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.h
>> +++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.h
>> @@ -339,7 +339,7 @@ EmacDmaStart (
>>    );
>>
>>
>> -VOID
>> +EFI_STATUS
>>  EFIAPI
>>  EmacGetDmaStatus (
>>    OUT UINT32                  *IrqStat  OPTIONAL,
>> --
>> 2.25.1
>>
>>
>>
>> ------------
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#107195): https://edk2.groups.io/g/devel/message/107195
>> Mute This Topic: https://groups.io/mt/100342205/5946980
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [pedro.falcato@gmail.com]
>> ------------
>>
>>
>
>
>-- 
>Pedro
>
>
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107260): https://edk2.groups.io/g/devel/message/107260
Mute This Topic: https://groups.io/mt/100342205/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6098 bytes --]