public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
@ 2023-07-18  7:07 wangyzhaoz
  0 siblings, 0 replies; 8+ messages in thread
From: wangyzhaoz @ 2023-07-18  7:07 UTC (permalink / raw)
  To: devel; +Cc: Yang Wang, Michael D Kinney, Loh Tien Hock, Ooi Tzy Way, Ran Wang

From: Yang Wang <wangyzhaoz@163.com>

Check EmacGetDmaStatus input parameters
IrqStat may be a null pointer.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Loh Tien Hock <tien.hock.loh@intel.com>
Cc: Ooi Tzy Way <tzy.way.ooi@intel.com>
Cc: Ran Wang <wangran@bosc.ac.cn>

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;
+  }
 
   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 (#106986): https://edk2.groups.io/g/devel/message/106986
Mute This Topic: https://groups.io/mt/100212047/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
@ 2023-07-25  1:10 wangyzhaoz
  2023-07-25  8:45 ` Pedro Falcato
  0 siblings, 1 reply; 8+ messages in thread
From: wangyzhaoz @ 2023-07-25  1:10 UTC (permalink / raw)
  To: devel; +Cc: Yang Wang

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;
+  }
 
   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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
  2023-07-25  1:10 [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus wangyzhaoz
@ 2023-07-25  8:45 ` Pedro Falcato
  2023-07-26  3:07   ` wangy
  0 siblings, 1 reply; 8+ messages in thread
From: Pedro Falcato @ 2023-07-25  8:45 UTC (permalink / raw)
  To: devel, wangyzhaoz; +Cc: Leif Lindholm, Ard Biesheuvel

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?
Also, please CC maintainers next time.

>
>    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 (#107226): https://edk2.groups.io/g/devel/message/107226
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
  2023-07-25  8:45 ` Pedro Falcato
@ 2023-07-26  3:07   ` wangy
  2023-07-27  0:26     ` Pedro Falcato
  0 siblings, 1 reply; 8+ messages in thread
From: wangy @ 2023-07-26  3:07 UTC (permalink / raw)
  To: devel, pedro.falcato; +Cc: Leif Lindholm, Ard Biesheuvel

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
  2023-07-26  3:07   ` wangy
@ 2023-07-27  0:26     ` Pedro Falcato
  2023-07-27  7:11       ` wangy
  0 siblings, 1 reply; 8+ messages in thread
From: Pedro Falcato @ 2023-07-27  0:26 UTC (permalink / raw)
  To: devel, wangyzhaoz; +Cc: Leif Lindholm, Ard Biesheuvel

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;
+      }
    }
    // 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;


It seems like this would preserve the original EFI protocol behavior
of *only* clearing the interrupt status for RX and TX if and only if
IrqStat was passed, and hence, read.


> >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?

Yes, usually you would use GetMaintainers.py from edk2 to get them.
It's not a complete match because I hand-typed their addresses into my
CC list and it autocompleted to another one of Ard's addresses (that
he also uses in the mailing list).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107277): https://edk2.groups.io/g/devel/message/107277
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
  2023-07-27  0:26     ` Pedro Falcato
@ 2023-07-27  7:11       ` wangy
  2023-07-27 13:45         ` Pedro Falcato
  0 siblings, 1 reply; 8+ messages in thread
From: wangy @ 2023-07-27  7:11 UTC (permalink / raw)
  To: Pedro Falcato; +Cc: devel, Leif Lindholm, Ard Biesheuvel, wangran@bosc.ac.cn

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

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;


>    }
>+
>    // Tx Buffer
>    if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){
>      Mask |= DW_EMAC_DMAGRP_STATUS_TU_SET_MSK;
>
>
>It seems like this would preserve the original EFI protocol behavior
>of *only* clearing the interrupt status for RX and TX if and only if
>IrqStat was passed, and hence, read.
>
>
>> >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?
>
>Yes, usually you would use GetMaintainers.py from edk2 to get them.
>It's not a complete match because I hand-typed their addresses into my
>CC list and it autocompleted to another one of Ard's addresses (that

>he also uses in the mailing list).


Thanks,I get it .


Regards,


Yang
> >-- >Pedro

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107297): https://edk2.groups.io/g/devel/message/107297
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: 10493 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
  2023-07-27  7:11       ` wangy
@ 2023-07-27 13:45         ` Pedro Falcato
  2023-07-28  3:55           ` wangy
  0 siblings, 1 reply; 8+ messages in thread
From: Pedro Falcato @ 2023-07-27 13:45 UTC (permalink / raw)
  To: devel, wangyzhaoz; +Cc: Leif Lindholm, Ard Biesheuvel, wangran@bosc.ac.cn

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?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107301): https://edk2.groups.io/g/devel/message/107301
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]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
  2023-07-27 13:45         ` Pedro Falcato
@ 2023-07-28  3:55           ` wangy
  0 siblings, 0 replies; 8+ messages in thread
From: wangy @ 2023-07-28  3:55 UTC (permalink / raw)
  To: Pedro Falcato; +Cc: devel, Leif Lindholm, Ard Biesheuvel, wangran@bosc.ac.cn

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

Hi Pedro Falcato,



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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107310): https://edk2.groups.io/g/devel/message/107310
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: 11855 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-07-28  3:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-25  1:10 [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus wangyzhaoz
2023-07-25  8:45 ` Pedro Falcato
2023-07-26  3:07   ` wangy
2023-07-27  0:26     ` Pedro Falcato
2023-07-27  7:11       ` wangy
2023-07-27 13:45         ` Pedro Falcato
2023-07-28  3:55           ` wangy
  -- strict thread matches above, loose matches on Subject: below --
2023-07-18  7:07 wangyzhaoz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox