Hi Pedro Falcato,
At 2023-07-29 03:40:15, "Pedro Falcato" <pedro.falc...@gmail.com> wrote:
>On Fri, Jul 28, 2023 at 5:00 AM wangy <wangyzh...@163.com> wrote:
>>
>> From: Yang Wang <wangyzh...@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.falc...@gmail.com>
Ok，Thanks. I will send an improved V3 PATCH.
Regards,
Yang
> >> Cc: Leif Lindholm <quic_llind...@quicinc.com> >> Cc: Ard Biesheuvel
> >> <a...@kernel.org> >> Cc: Ran Wang <wang...@bosc.ac.cn> >> >>
> >> Signed-off-by: Yang Wang <wangyzh...@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+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]
-=-=-=-=-=-=-=-=-=-=-=-
