[edk2-devel] [PATCH] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus
wangy
wangyzhaoz at 163.com
Fri Jul 28 03:55:16 UTC 2023
Hi Pedro Falcato,
At 2023-07-27 21:45:46, "Pedro Falcato" <pedro.falcato at gmail.com> wrote:
>On Thu, Jul 27, 2023 at 8:12 AM wangy <wangyzhaoz at 163.com> wrote:
>>
>> Hi Pedro Falcato,
>>
>> At 2023-07-27 08:26:44, "Pedro Falcato" <pedro.falcato at gmail.com> wrote:
>> >On Wed, Jul 26, 2023 at 4:07 AM wangy <wangyzhaoz at 163.com> wrote:
>> >>
>> >> Hi Pedro Falcato,
>> >>
>> >> At 2023-07-25 16:45:01, "Pedro Falcato" <pedro.falcato at gmail.com> wrote:
>> >>
>> >> >On Tue, Jul 25, 2023 at 2:10 AM <wangyzhaoz at 163.com> wrote:
>> >> >>
>> >> >> From: Yang Wang <wangyzhaoz at 163.com>
>> >> >>
>> >> >> Check EmacGetDmaStatus input parameters
>> >> >> IrqStat may be a null pointer.
>> >> >>
>> >> >> Signed-off-by: Yang Wang <wangyzhaoz at 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;
>> >+ }
>>
>> 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 |=DW_EMAC_DMAGRP_STATUS_RI_SET_MSK
>> if "if (IrqStat != NULL)" is true, set *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> 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;
>> + if (IrqStat != NULL) {
>> + *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> + }
>> Mask |= DW_EMAC_DMAGRP_STATUS_RI_SET_MSK;
>> - } else {
>> - *IrqStat &= ~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 |= 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;
>> >+ }
>>
>> 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 |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> if " if (IrqStat != NULL)" is true , set *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> // Tx interrupt
>> if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK) {
>> - *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> + if (IrqStat != NULL) {
>> + *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> + }
>> Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> - } else {
>> - *IrqStat &= ~EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> }
>> +
>> // Tx Buffer
>> if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){
>> Mask |= 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 “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 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?
I have tested the patch you suggested, and it can also work.
I will second V2 of the patch based on your suggestion.
Thank you very much for your suggestion.
Regards,
Yang
> >-- >Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107310): https://edk2.groups.io/g/devel/message/107310
Mute This Topic: https://groups.io/mt/100342205/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20230728/bb10c30a/attachment-0001.htm>
More information about the edk2-devel-archive
mailing list