On Fri, 28 Jul 2023 at 21:40, Pedro Falcato <pedro.falc...@gmail.com> wrote:
>
> On Fri, Jul 28, 2023 at 5:00 AM wangy <wangyzh...@163.com> wrote:
> >
> > From: Yang Wang <wangyzh...@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.falc...@gmail.com>
>
Thanks,
I've pushed this as 4f316893c9ed..cbab3c40f76e with the suggested
update for the commit message.
> > Cc: Leif Lindholm <quic_llind...@quicinc.com>
> > Cc: Ard Biesheuvel <a...@kernel.org>
> > Cc: Ran Wang <wang...@bosc.ac.cn>
> >
> > Signed-off-by: Yang Wang <wangyzh...@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+ow...@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [pedro.falc...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
