<div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div>Hi Pedro Falcato,</div><div><br></div><p style="margin: 0;"><span style="font-family: arial; white-space: pre-wrap;">At 2023-07-27 21:45:46, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:</span></p><pre>>On Thu, Jul 27, 2023 at 8:12 AM wangy <wangyzhaoz@163.com> wrote:
>>
>> Hi Pedro Falcato,
>>
>> At 2023-07-27 08:26:44, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:
>> >On Wed, Jul 26, 2023 at 4:07 AM wangy <wangyzhaoz@163.com> wrote:
>> >>
>> >> Hi Pedro Falcato,
>> >>
>> >> At 2023-07-25 16:45:01, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:
>> >>
>> >> >On Tue, Jul 25, 2023 at 2:10 AM <wangyzhaoz@163.com> wrote:
>> >> >>
>> >> >> From: Yang Wang <wangyzhaoz@163.com>
>> >> >>
>> >> >> Check EmacGetDmaStatus input parameters
>> >> >> IrqStat may be a null pointer.
>> >> >>
>> >> >> Signed-off-by: Yang Wang <wangyzhaoz@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
<div>>test it?</div><div><br></div><div>I have tested the patch you suggested, and it can also work.
I will second V2 of the patch based on your suggestion.</div><div>Thank you very much for your suggestion.</div><div><br></div><div><pre style="width: 1602.64px; word-break: break-word !important;"><div style="white-space: unset !important; word-break: break-word !important;">Regards,</div><div style="white-space: unset !important; word-break: break-word !important;"><br style="white-space: unset !important; word-break: break-word !important;"></div><div style="white-space: unset !important; word-break: break-word !important;">Yang</div></pre></div><div><br></div>>
>--
>Pedro
</pre></div>
<div width="1" style="color:white;clear:both">_._,_._,_</div>
<hr>
Groups.io Links:<p>
You receive all messages sent to this group.
<p>
<a target="_blank" href="https://edk2.groups.io/g/devel/message/107310">View/Reply Online (#107310)</a> |
|
<a target="_blank" href="https://groups.io/mt/100342205/1813853">Mute This Topic</a>
| <a href="https://edk2.groups.io/g/devel/post">New Topic</a>
<br>
<a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> |
<a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> |
<a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a>
[edk2-devel-archive@redhat.com]<br>
<div width="1" style="color:white;clear:both">_._,_._,_</div>