Hi Pedro Falcato,

At 2023-07-29 03:40:15, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:
>On Fri, Jul 28, 2023 at 5:00 AM wangy <wangyzhaoz@163.com> wrote:
>>
>> From: Yang Wang <wangyzhaoz@163.com>
>>
>> If IrqStat is NULL, the interrupt status will not be
>> read from the device.When the interrupt status is read,
>> it will also be cleared.
>
>Let's improve the commit message a bit, something like:
>
>The EFI spec (see UEFI 2.10, 24.1.12) requires
>EFI_SIMPLE_NETWORK.GetStatus() to handle NULL InterruptStatus pointers
>by not reading nor clearing the interrupt status from the device.
>
>However, EmacGetDmaStatus (part of the DwEmacSnpDxe GetStatus()
>implementation) did not correctly handle NULL IrqStat, despite already
>being tagged as an OPTIONAL argument. This made calling GetStatus()
>with a NULL pointer (for example, the call in MnpRecycleTxBuf) either
>corrupt memory or straight-up crash.
>
>Make it EFI spec compliant, by adding proper NULL pointer checks
>around RI_SET_MSK and TI_SET_MSK retrieval/clearing.
>
>--
>
>In any case, take my:
>
>Acked-by: Pedro Falcato <pedro.falcato@gmail.com>

Ok，Thanks. I will send an improved V3 PATCH.

Regards,

Yang

>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: Ran Wang <wangran@bosc.ac.cn>
>>
>> Signed-off-by: Yang Wang <wangyzhaoz@163.com>
>> ---
>>  .../Drivers/DwEmacSnpDxe/EmacDxeUtil.c        | 22 ++++++++++++-------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> index 3b982ce984..26d3ff6138 100755
>> --- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> +++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> @@ -500,24 +500,30 @@ EmacGetDmaStatus (
>>    UINT32  ErrorBit;
>>    UINT32  Mask = 0;
>>
>> +  if (IrqStat != NULL) {
>> +    *IrqStat = 0;
>> +  }
>> +
>>    DmaStatus = MmioRead32 (MacBaseAddress +
>>                             DW_EMAC_DMAGRP_STATUS_OFST);
>>    if (DmaStatus & DW_EMAC_DMAGRP_STATUS_NIS_SET_MSK) {
>>      Mask |= DW_EMAC_DMAGRP_STATUS_NIS_SET_MSK;
>>      // Rx interrupt
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_RI_SET_MSK) {
>> -      *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> -      Mask |= DW_EMAC_DMAGRP_STATUS_RI_SET_MSK;
>> -    } else {
>> -      *IrqStat &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> +      if (IrqStat != NULL) {
>> +        *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> +        Mask |= DW_EMAC_DMAGRP_STATUS_RI_SET_MSK;
>> +      }
>>      }
>> +
>>      // Tx interrupt
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK) {
>> -      *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> -      Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> -    } else {
>> -      *IrqStat &= ~EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> +      if (IrqStat != NULL) {
>> +        *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> +        Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> +      }
>>      }
>> +
>>      // Tx Buffer
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){
>>        Mask |= DW_EMAC_DMAGRP_STATUS_TU_SET_MSK;
>> --
>> 2.25.1
>>
>>
>>
>> ------------
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#107311): https://edk2.groups.io/g/devel/message/107311
>> Mute This Topic: https://groups.io/mt/100404855/5946980
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [pedro.falcato@gmail.com]
>> ------------
>>
>>
>
>
>-- 
>Pedro