Re: [edk2-devel] [edk2-platforms][PATCH V3] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
Hi Pedro Falcato,
At 2023-07-31 17:15:20, "Pedro Falcato" wrote:
>On Mon, Jul 31, 2023 at 4:25 AM wangy wrote:
>>
>> From: Yang Wang
>>
>> 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.
>>
>> Cc: Leif Lindholm
>> Cc: Ard Biesheuvel
>>
>> Signed-off-by: Yang Wang
>> Acked-by: Pedro Falcato
>> Reviewed-by: Ran Wang
>> ---
>> .../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
>
>Hi,
>
>No need for a v3, Ard has already applied the patch
>(https://github.com/tianocore/edk2-platforms/commit/cbab3c40f76ee913621a9f4afe3398b217b0d086).
Thank you very much, I see.
Regards,
Yang
> >-- >Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107416): https://edk2.groups.io/g/devel/message/107416
Mute This Topic: https://groups.io/mt/100455239/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH V1] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
Hi Pedro Falcato,
At 2023-07-29 03:40:15, "Pedro Falcato" wrote:
>On Fri, Jul 28, 2023 at 5:00 AM wangy wrote:
>>
>> From: Yang Wang
>>
>> 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
Ok，Thanks. I will send an improved V3 PATCH.
Regards,
Yang
> >> Cc: Leif Lindholm >> Cc: Ard Biesheuvel
> >> >> Cc: Ran Wang >> >>
> >> Signed-off-by: Yang Wang >> --- >>
> >> .../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+ow...@edk2.groups.io >> Unsubscribe:
> >> https://edk2.groups.io/g/devel/unsub [pedro.falc...@gmail.com] >>
> >> >> >> > > >-- >Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107392): https://edk2.groups.io/g/devel/message/107392
Mute This Topic: https://groups.io/mt/100404855/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [edk2-platforms][PATCH V3] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
From: Yang Wang
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.
Cc: Leif Lindholm
Cc: Ard Biesheuvel
Signed-off-by: Yang Wang
Acked-by: Pedro Falcato
Reviewed-by: Ran Wang
---
.../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 (#107391): https://edk2.groups.io/g/devel/message/107391
Mute This Topic: https://groups.io/mt/100455239/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [edk2-platforms][PATCH V1] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
From: Yang Wang
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.
Cc: Leif Lindholm
Cc: Ard Biesheuvel
Cc: Ran Wang
Signed-off-by: Yang Wang
---
.../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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
Hi Pedro Falcato,
At 2023-07-27 21:45:46, "Pedro Falcato" wrote:
>On Thu, Jul 27, 2023 at 8:12 AM wangy wrote:
>>
>> Hi Pedro Falcato,
>>
>> At 2023-07-27 08:26:44, "Pedro Falcato" wrote:
>> >On Wed, Jul 26, 2023 at 4:07 AM wangy wrote:
>> >>
>> >> Hi Pedro Falcato,
>> >>
>> >> At 2023-07-25 16:45:01, "Pedro Falcato" wrote:
>> >>
>> >> >On Tue, Jul 25, 2023 at 2:10 AM wrote:
>> >> >>
>> >> >> From: Yang Wang
>> >> >>
>> >> >> Check EmacGetDmaStatus input parameters
>> >> >> IrqStat may be a null pointer.
>> >> >>
>> >> >> Signed-off-by: Yang Wang
>> >> >> ---
>> >> >> .../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 UINTNMacBaseAddress
>> >> >>)
>> >> >> {
>> >> >> - UINT32 DmaStatus;
>> >> >> - UINT32 ErrorBit;
>> >> >> - UINT32 Mask = 0;
>> >> >> + UINT32DmaStatus;
>> >> >> + UINT32ErrorBit;
>> >> >> + UINT32Mask = 0;
>> >> >> + EFI_STATUSStatus = 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.
&
Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
Hi Pedro Falcato,
At 2023-07-27 08:26:44, "Pedro Falcato" wrote:
>On Wed, Jul 26, 2023 at 4:07 AM wangy wrote:
>>
>> Hi Pedro Falcato,
>>
>> At 2023-07-25 16:45:01, "Pedro Falcato" wrote:
>>
>> >On Tue, Jul 25, 2023 at 2:10 AM wrote:
>> >>
>> >> From: Yang Wang
>> >>
>> >> Check EmacGetDmaStatus input parameters
>> >> IrqStat may be a null pointer.
>> >>
>> >> Signed-off-by: Yang Wang
>> >> ---
>> >> .../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 UINTNMacBaseAddress
>> >>)
>> >> {
>> >> - UINT32 DmaStatus;
>> >> - UINT32 ErrorBit;
>> >> - UINT32 Mask = 0;
>> >> + UINT32DmaStatus;
>> >> + UINT32ErrorBit;
>> >> + UINT32Mask = 0;
>> >> + EFI_STATUSStatus = 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
Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
Hi Pedro Falcato,
At 2023-07-25 16:45:01, "Pedro Falcato" wrote:
>On Tue, Jul 25, 2023 at 2:10 AM wrote:
>>
>> From: Yang Wang
>>
>> Check EmacGetDmaStatus input parameters
>> IrqStat may be a null pointer.
>>
>> Signed-off-by: Yang Wang
>> ---
>> .../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 UINTNMacBaseAddress
>>)
>> {
>> - UINT32 DmaStatus;
>> - UINT32 ErrorBit;
>> - UINT32 Mask = 0;
>> + UINT32DmaStatus;
>> + UINT32ErrorBit;
>> + UINT32Mask = 0;
>> + EFI_STATUSStatus = 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
M: Leif Lindholm
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+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [pedro.falc...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
