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 8115B740035 for ; Thu, 27 Jul 2023 13:46:01 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=37wa4KmShGMFQU+BUXwy/udW/tY07qsikSQOgoA9Fho=; 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=1690465560; v=1; b=pSKStJPqtbKsqfPp68GwxSBXb5lc1e5lKgl+/SvLyAD98lmMwrsjzo+IioR7c0Ip1gK7C2TW 4I51AdygvqaMBqvmHJKhMF8u9+LNfXtuiT9PxG8ae8afJMvMMIFrCXpo6oSBsIW1EBc1RTZums9 htDxm7ko93Wp5TSyiIvhy7JA= X-Received: by 127.0.0.2 with SMTP id srkGYY7687511xYlAnuAZQ54; Thu, 27 Jul 2023 06:46:00 -0700 X-Received: from mail-vk1-f169.google.com (mail-vk1-f169.google.com [209.85.221.169]) by mx.groups.io with SMTP id smtpd.web10.7749.1690465559396321420 for ; Thu, 27 Jul 2023 06:45:59 -0700 X-Received: by mail-vk1-f169.google.com with SMTP id 71dfb90a1353d-4863c756812so376337e0c.1 for ; Thu, 27 Jul 2023 06:45:59 -0700 (PDT) X-Gm-Message-State: 218GWIjvVhbFz1lUjtE8kTDux7686176AA= X-Google-Smtp-Source: APBJJlEBD/8hu3asJgowgQKaaubeOtzheCT5bdqFCCRpiUKkj+bbMExXXyuAQPoXOG/CkKC0hiJN9C3zDznXEwUNjVM= X-Received: by 2002:a1f:414c:0:b0:471:19d6:1ce1 with SMTP id o73-20020a1f414c000000b0047119d61ce1mr1383237vka.11.1690465558048; Thu, 27 Jul 2023 06:45:58 -0700 (PDT) MIME-Version: 1.0 References: <20230725011037.2229-1-wangyzhaoz@163.com> <12891562.24d4.189902ac15b.Coremail.wangyzhaoz@163.com> <1dce3e26.221f.1899630890d.Coremail.wangyzhaoz@163.com> In-Reply-To: <1dce3e26.221f.1899630890d.Coremail.wangyzhaoz@163.com> From: "Pedro Falcato" Date: Thu, 27 Jul 2023 14:45:46 +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 , "wangran@bosc.ac.cn" 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=pSKStJPq; 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 Thu, Jul 27, 2023 at 8:12=E2=80=AFAM wangy wrote: > > Hi Pedro Falcato, > > At 2023-07-27 08:26:44, "Pedro Falcato" wrote: > >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" wrot= e: > >> > >> >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/DwEma= cSnpDxe.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__, Stat= us)); > >> >> + } > >> >> > >> >> - return EFI_SUCCESS; > >> >> + return Status; > >> >> } > >> >> > >> >> > >> >> diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacD= xeUtil.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= code of 'Status=3DSnp ->GetStatus (Snp, NULL, (VOID * *)&TxBuf);'. > >> then in EmacGetDmaStatus(), it will not check this pointer and use dir= ectly, 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 = 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 b= uffer 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/EmacDxeUti= l.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; > >+ } > > 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= |=3DDW_EMAC_DMAGRP_STATUS_RI_SET_MSK > if "if (IrqStat !=3D NULL)" is true, set *IrqStat |=3D EFI_SIMPLE_NETWO= RK_RECEIVE_INTERRUPT; > 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; > + if (IrqStat !=3D NULL) { > + *IrqStat |=3D EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; > + } > Mask |=3D DW_EMAC_DMAGRP_STATUS_RI_SET_MSK; > - } else { > - *IrqStat &=3D ~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 |=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; > >+ } > > 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= |=3D DW_EMAC_DMAGRP_STATUS_TI_SET_MSK; > if " if (IrqStat !=3D NULL)" is true , set *IrqStat |=3D EFI_SIMPLE_NETWO= RK_TRANSMIT_INTERRUPT; > // Tx interrupt > if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK) { > - *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; > - } else { > - *IrqStat &=3D ~EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT; > } > + > // Tx Buffer > if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){ > Mask |=3D DW_EMAC_DMAGRP_STATUS_TU_SET_MSK; > This is wrong. Again, quoting the EFI spec... > 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 you must only clear the status if you read the status. If IrqStat is NULL, you don't read the interrupt status, so you must also not clear it. As far as I can see, my patch is correct. Could you please test it? --=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 (#107301): https://edk2.groups.io/g/devel/message/107301 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-