Hi Pedro Falcato,
At 2023-07-27 08:26:44, "Pedro Falcato" <pedro.falc...@gmail.com> wrote:
>On Wed, Jul 26, 2023 at 4:07 AM wangy <wangyzh...@163.com> wrote:
>>
>> Hi Pedro Falcato,
>>
>> At 2023-07-25 16:45:01, "Pedro Falcato" <pedro.falc...@gmail.com> wrote:
>>
>> >On Tue, Jul 25, 2023 at 2:10 AM <wangyzh...@163.com> wrote:
>> >>
>> >> From: Yang Wang <wangyzh...@163.com>
>> >>
>> >> Check EmacGetDmaStatus input parameters
>> >> IrqStat may be a null pointer.
>> >>
>> >> Signed-off-by: Yang Wang <wangyzh...@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;
> }
>+
> // Tx Buffer
> if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){
> Mask |= 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/Maintainers.txt, is not complete match to what you listed, I am a
>> little bit confused
>> R: Ard Biesheuvel <ardb+tianoc...@kernel.org>
>> M: Leif Lindholm <quic_llind...@quicinc.com>
>> 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).
Thanks,I get it .
Regards,
Yang
> >-- >Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107297): https://edk2.groups.io/g/devel/message/107297
Mute This Topic: https://groups.io/mt/100342205/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
