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