From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 1E141AC0473 for ; Thu, 27 Jul 2023 00:26:58 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=/VXudo98Wk/raxySUTvwj3IPfx32LzENtWLCxKemmGs=; c=relaxed/simple; d=groups.io; h=X-Received:X-Received:X-Received:X-Gm-Message-State:X-Google-Smtp-Source:X-Received:MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1690417617; v=1; b=hgZgS1kf2EZMtno8GjpNkqn5MeXnhVl8CbTP6xHNFswIDrD3Uq+8Z4hWXjIRa3nSUZPGPZiz 9NdUDf6sirjDAn3pUCNn62Na4XxMZdx032dyDJlBSk6AiOoiS4Nw8xrY4Oqsy4gcy4sA2xrf9s+ cE/vATNVBAbT0kC4R9/GFCJs= X-Received: by 127.0.0.2 with SMTP id 6oALYY7687511xjgMJtAQxUc; Wed, 26 Jul 2023 17:26:57 -0700 X-Received: from mail-vk1-f172.google.com (mail-vk1-f172.google.com [209.85.221.172]) by mx.groups.io with SMTP id smtpd.web10.2525.1690417617063275673 for ; Wed, 26 Jul 2023 17:26:57 -0700 X-Received: by mail-vk1-f172.google.com with SMTP id 71dfb90a1353d-4863f6ffed5so177495e0c.0 for ; Wed, 26 Jul 2023 17:26:56 -0700 (PDT) X-Gm-Message-State: nZ8yPDdn5J5jqkk7Gy4gRpqSx7686176AA= X-Google-Smtp-Source: APBJJlEtg6TZr1YIwbt2N57Yk7xlqzg3dTZQRqaRx3qnBGqj8s5GRy02K51y5hr2N9QtNfUznHHuRizPPt7977rtbzY= X-Received: by 2002:a1f:c10d:0:b0:471:7996:228f with SMTP id r13-20020a1fc10d000000b004717996228fmr517292vkf.7.1690417615843; Wed, 26 Jul 2023 17:26:55 -0700 (PDT) MIME-Version: 1.0 References: <20230725011037.2229-1-wangyzhaoz@163.com> <12891562.24d4.189902ac15b.Coremail.wangyzhaoz@163.com> In-Reply-To: <12891562.24d4.189902ac15b.Coremail.wangyzhaoz@163.com> From: "Pedro Falcato" Date: Thu, 27 Jul 2023 01:26:44 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus To: devel@edk2.groups.io, wangyzhaoz@163.com Cc: Leif Lindholm , Ard Biesheuvel Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pedro.falcato@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=hgZgS1kf; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Wed, Jul 26, 2023 at 4:07=E2=80=AFAM wangy wrote: > > Hi Pedro Falcato, > > At 2023-07-25 16:45:01, "Pedro Falcato" wrote: > > >On Tue, Jul 25, 2023 at 2:10=E2=80=AFAM 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/DwEmacSn= pDxe.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 =3D 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/EmacDxeU= til.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 =3D 0; > >> + UINT32 DmaStatus; > >> + UINT32 ErrorBit; > >> + UINT32 Mask =3D 0; > >> + EFI_STATUS Status =3D EFI_SUCCESS; > >> + > >> + if (IrqStat =3D=3D NULL) { > >> + Status =3D 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 co= de of 'Status=3DSnp ->GetStatus (Snp, NULL, (VOID * *)&TxBuf);'. > then in EmacGetDmaStatus(), it will not check this pointer and use direct= ly, 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 =E2=80= =9CRelated Definitions=E2=80=9D). If this is NULL, the interrupt status wil= l not be read from the device. > If this is not NULL, the interrupt status will be read from the device. W= hen the interrupt status is read, it will also be cleared. > Clearing the transmit interrupt does not empty the recycled transmit buff= er 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 =3D 0; + if (IrqStat !=3D NULL) { + *IrqStat =3D 0; + } + DmaStatus =3D MmioRead32 (MacBaseAddress + DW_EMAC_DMAGRP_STATUS_OFST); if (DmaStatus & DW_EMAC_DMAGRP_STATUS_NIS_SET_MSK) { Mask |=3D DW_EMAC_DMAGRP_STATUS_NIS_SET_MSK; // Rx interrupt if (DmaStatus & DW_EMAC_DMAGRP_STATUS_RI_SET_MSK) { - *IrqStat |=3D EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; - Mask |=3D DW_EMAC_DMAGRP_STATUS_RI_SET_MSK; - } else { - *IrqStat &=3D ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; + if (IrqStat !=3D NULL) { + *IrqStat |=3D EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; + Mask |=3D DW_EMAC_DMAGRP_STATUS_RI_SET_MSK; + } } // Tx interrupt if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK) { - *IrqStat |=3D EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; - Mask |=3D DW_EMAC_DMAGRP_STATUS_TI_SET_MSK; - } else { - *IrqStat &=3D ~EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; + if (IrqStat !=3D NULL) { + *IrqStat |=3D EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; + Mask |=3D DW_EMAC_DMAGRP_STATUS_TI_SET_MSK; + } } + // Tx Buffer if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){ Mask |=3D 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/Maintainer= s.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? 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). --=20 Pedro -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-