From: "Ard Biesheuvel" <ardb@kernel.org>
To: Pedro Falcato <pedro.falcato@gmail.com>
Cc: devel@edk2.groups.io, wangyzhaoz@163.com,
Leif Lindholm <quic_llindhol@quicinc.com>,
Ran Wang <wangran@bosc.ac.cn>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH V1] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
Date: Sat, 29 Jul 2023 09:06:17 +0200 [thread overview]
Message-ID: <CAMj1kXEwtVFuQ1H4Ms9d8mw_XVp9eSbYBw-KwhpBCqai7zcptA@mail.gmail.com> (raw)
In-Reply-To: <CAKbZUD2jsGTYrAOh7Q+HWZmNUYuqWvfPtX0io5uTBZEe4gxZRg@mail.gmail.com>
On Fri, 28 Jul 2023 at 21:40, Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Fri, Jul 28, 2023 at 5:00 AM wangy <wangyzhaoz@163.com> wrote:
> >
> > From: Yang Wang <wangyzhaoz@163.com>
> >
> > If IrqStat is NULL, the interrupt status will not be
> > read from the device.When the interrupt status is read,
> > it will also be cleared.
>
> Let's improve the commit message a bit, something like:
>
> The EFI spec (see UEFI 2.10, 24.1.12) requires
> EFI_SIMPLE_NETWORK.GetStatus() to handle NULL InterruptStatus pointers
> by not reading nor clearing the interrupt status from the device.
>
> However, EmacGetDmaStatus (part of the DwEmacSnpDxe GetStatus()
> implementation) did not correctly handle NULL IrqStat, despite already
> being tagged as an OPTIONAL argument. This made calling GetStatus()
> with a NULL pointer (for example, the call in MnpRecycleTxBuf) either
> corrupt memory or straight-up crash.
>
> Make it EFI spec compliant, by adding proper NULL pointer checks
> around RI_SET_MSK and TI_SET_MSK retrieval/clearing.
>
> --
>
> In any case, take my:
>
> Acked-by: Pedro Falcato <pedro.falcato@gmail.com>
>
Thanks,
I've pushed this as 4f316893c9ed..cbab3c40f76e with the suggested
update for the commit message.
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Ran Wang <wangran@bosc.ac.cn>
> >
> > Signed-off-by: Yang Wang <wangyzhaoz@163.com>
> > ---
> > .../Drivers/DwEmacSnpDxe/EmacDxeUtil.c | 22 ++++++++++++-------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
> > index 3b982ce984..26d3ff6138 100755
> > --- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
> > +++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
> > @@ -500,24 +500,30 @@ 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;
> > --
> > 2.25.1
> >
> >
> >
> > ------------
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#107311): https://edk2.groups.io/g/devel/message/107311
> > Mute This Topic: https://groups.io/mt/100404855/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 (#107377): https://edk2.groups.io/g/devel/message/107377
Mute This Topic: https://groups.io/mt/100404855/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-07-29 7:06 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-28 3:59 [edk2-devel] [edk2-platforms][PATCH V1] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus wangy
2023-07-28 19:40 ` Pedro Falcato
2023-07-29 7:06 ` Ard Biesheuvel [this message]
2023-07-31 3:24 ` wangy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMj1kXEwtVFuQ1H4Ms9d8mw_XVp9eSbYBw-KwhpBCqai7zcptA@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox