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;
+ }
}
// 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;
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).
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107277): https://edk2.groups.io/g/devel/message/107277
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]
-=-=-=-=-=-=-=-=-=-=-=-
