Hi Pedro Falcato, At 2023-07-27 08:26:44, "Pedro Falcato" <pedro.falc...@gmail.com> wrote: >On Wed, Jul 26, 2023 at 4:07 AM wangy <wangyzh...@163.com> wrote: >> >> Hi Pedro Falcato, >> >> At 2023-07-25 16:45:01, "Pedro Falcato" <pedro.falc...@gmail.com> wrote: >> >> >On Tue, Jul 25, 2023 at 2:10 AM <wangyzh...@163.com> wrote: >> >> >> >> From: Yang Wang <wangyzh...@163.com> >> >> >> >> Check EmacGetDmaStatus input parameters >> >> IrqStat may be a null pointer. >> >> >> >> Signed-off-by: Yang Wang <wangyzh...@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+tianoc...@kernel.org> >> M: Leif Lindholm <quic_llind...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-