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 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 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+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] -=-=-=-=-=-=-=-=-=-=-=-