At 2023-07-27 21:45:46, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:

>On Thu, Jul 27, 2023 at 8:12 AM wangy <wangyzhaoz@163.com> wrote:
>>
>> Hi Pedro Falcato,
>>
>> At 2023-07-27 08:26:44, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:
>> >On Wed, Jul 26, 2023 at 4:07 AM wangy <wangyzhaoz@163.com> wrote:
>> >>
>> >> 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.
>> >>
>> >
>> >The EFI_SIMPLE_NETWORK protocol, that SnpGetStatus implements, says:
>> >
>> >> A pointer to the bit mask of the currently active interrupts (see “Related Definitions”). If this is NULL, the interrupt status will not be read from the device.
>> >> If this is not NULL, the interrupt status will be read from the device. When the interrupt status is read, it will also be cleared.
>> >> Clearing the transmit interrupt does not empty the recycled transmit buffer array.
>> >
>> >So it seems to me that there's some missing logic in EmacGetDmaStatus.
>> >
>> >I don't know this hardware, but I'll assume that writes to
>> >DW_EMAC_DMAGRP_STATUS_OFST clear/ACK the corresponding interrupt
>> >status bit.
>> >
>> >I would suggest this (apologies for gmail line wrapping, hopefully you
>> >get the idea):
>> >
>> >diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> >b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> >index 3b982ce98411..df4ce53e9e0b 100755
>> >--- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> >+++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> >@@ -500,24 +500,29 @@ 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;
>> >+      }
>>
>> How about  it be more appropriate to change it to this way?
>> if "if (DmaStatus & DW_EMAC_DMAGRP_STATUS_RI_SET_MSK) " is true, set Mask |=DW_EMAC_DMAGRP_STATUS_RI_SET_MSK
>> if  "if (IrqStat != NULL)" is true, set  *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>>    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;
>> +      if (IrqStat != NULL) {
>> +        *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> +      }
>>        Mask |= DW_EMAC_DMAGRP_STATUS_RI_SET_MSK;
>> -    } else {
>> -      *IrqStat &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>>      }
>> +
>>      // Tx interrupt
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_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;
>> >+      }
>>
>> How about it be more appropriate to change it to this way?
>> if "if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK)" is true , set Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> if " if (IrqStat != NULL)" is true , set *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>>      // Tx interrupt
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK) {
>> -      *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> +      if (IrqStat != NULL) {
>> +        *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> +      }
>>        Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> -    } else {
>> -      *IrqStat &= ~EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>>      }
>> +
>>      // Tx Buffer
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){
>>        Mask |= DW_EMAC_DMAGRP_STATUS_TU_SET_MSK;
>>
>
>This is wrong. Again, quoting the EFI spec...
>
>> A pointer to the bit mask of the currently active interrupts (see “Related Definitions”). If this is NULL, the interrupt status will not be read from the device.
>> If this is not NULL, the interrupt status will be read from the device. When the interrupt status is read, it will also be cleared.
>> Clearing the transmit interrupt does not empty the recycled transmit buffer array.
>
>So you must only clear the status if you read the status. If IrqStat
>is NULL, you don't read the interrupt status, so you must also not
>clear it. As far as I can see, my patch is correct. Could you please
>test it?

I have tested the patch you suggested, and it can also work. 
I will second V2 of the patch based on your suggestion.
Thank you very much for your suggestion.

Regards,

Yang

>
>-- 
>Pedro