Date:   Wed, 12 Apr 2023 09:27:19 +0300
From:   Tomas Melin <tomas.melin@...sala.com>
To:     Ingo Rohloff <ingo.rohloff@...terbach.com>,
        robert.hancock@...ian.com
Cc:     Nicolas.Ferre@...rochip.com, claudiu.beznea@...rochip.com,
        davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 0/1] Alternative, restart tx after tx used bit read

Hi,
On 08/04/2023 00:33, Ingo Rohloff wrote:
> [You don't often get email from ingo.rohloff@...terbach.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> I am sorry; this is a long E-Mail.
> 
> I am referring to this problem:
> 
> Robert Hancock wrote:
>> On Wed, 2022-03-23 at 08:43 -0700, Jakub Kicinski wrote:
>>> On Wed, 23 Mar 2022 10:08:20 +0200 Tomas Melin wrote:
>>>>> From: Claudiu Beznea <claudiu.beznea@...rochip.com>
>>>>>
>>>>> On some platforms (currently detected only on SAMA5D4) TX might stuck
>>>>> even the pachets are still present in DMA memories and TX start was
>>>>> issued for them.
>>>>> ...
>>>> On Xilinx Zynq the above change can cause infinite interrupt loop
>>>> leading to CPU stall.  Seems timing/load needs to be appropriate for
>>>> this to happen, and currently with 1G ethernet this can be triggered
>>>> normally within minutes when running stress tests on the network
>>>> interface.
>>>> ...
>>> Which kernel version are you using?  Robert has been working on macb +
>>> Zynq recently, adding him to CC.
>> ...
>> I haven't looked at the TX ring descriptor and register setup on this core
>> in that much detail, but the fact the controller gets into this "TX used
>> bit read" state in the first place seems unusual.  I'm wondering if
>> something is being done in the wrong order or if we are missing a memory
>> barrier etc?
> 
> I am developing on a ZynqMP (Ultrascale+) SoC from AMD/Xilinx.
> I have seen the same issue before commit 4298388574dae6168 ("net: macb:
> restart tx after tx used bit read")

Since you mention before that commit this triggered, have you been able
to reproduce the problem with that commit applied?

> 
> The scenario which sometimes triggers it for me:
> 
> I have an application running on the PC.
> The application sends a short command (via TCP) to the ZynqMP.
> The ZynqMP answers with a long stream of bytes via TCP
> (around 230KiB).
> The PC knows the amount of data and waits to receive the data completely.
> The PC gets stuck, because the last TCP segment of the transfer gets
> stuck in the ZynqMP and is not transmitted.
> You can re-trigger the TX Ring by pinging the ZynqMP:
> The Ping answer will re-trigger the TX ring, which in turn will also
> then send the stuck IP/TCP packet.
> 
> Unfortunately triggering this problem seems to be hard; at least I am
> not able to reproduce it easily.
> 
> So: If anyone has a more reliable way to trigger the problem,
> please tell me.
> This is to check if my proposed alternative works under all circumstances.
> 
> I have an alternate implementation, which does not require to turn on
> the "TX USED BIT READ" (TUBR) interrupt.
> The reason why I think this alternative might be better is, because I
> believe the TUBR interrupt happens at the wrong time; so I am not sure
> that the current implementation works reliably.
> 
> Analysis:
> Commit 404cd086f29e867f ("net: macb: Allocate valid memory for TX and RX BD
> prefetch") mentions:
> 
>     GEM version in ZynqMP and most versions greater than r1p07 supports
>     TX and RX BD prefetch. The number of BDs that can be prefetched is a
>     HW configurable parameter. For ZynqMP, this parameter is 4.
> 
> I think what happens is this:
> Example Scenario (SW == linux kernel, HW == cadence ethernet IP).
> 1) SW has written TX descriptors 0..7
> 2) HW is currently transmitting TX descriptor 6.
>    HW has already prefetched TX descriptors 6,7,8,9.
> 3) SW writes TX descriptor 8 (clearing TX_USED)
> 4) SW writes the TSTART bit.
>    HW ignores this, because it is still transmitting.
> 5) HW transmits TX descriptor 7.
> 6) HW reaches descriptor 8; because this descriptor
>    has already been prefetched, HW sees a non-active
>    descriptor (TX_USED set) and stops transmitting.
> 
> From debugging the code it seems that the TUBR interrupt happens, when
> a descriptor is prefetched, which has a TX_USED bit set, which is before
> it is processed by the rest of the hardware:
> When looking at the end of a transfer it seems I get a TUBR interrupt,
> followed by some more TX COMPLETE interrupts.
> 
I recall that the documentation for the TUBR is rather sparse, so to be
sure about the semantics how this is supposed to work, internal
documentation would indeed be valuable.

Thanks,
Tomas


> Additionally that means at the time the TUBR interrupt happens, it
> is too early to write the TSTART bit again, because the hardware is
> still actively transmitting.
> 
> The alternative I implemented is to check in macb_tx_complete() if
> 
> 1) The TX Queue is non-empty (there are pending TX descriptors)
> 2) The hardware indicates that it is not transmitting any more
> 
> If this situation is detected, the TSTART bit will be written to
> restart the TX ring.
> 
> I know for sure, that I hit the code path, which restarts the
> transmission in macb_tx_complete(); that's why I believe the
> "Example Scenario" I described above is correct.
> 
> I am still not sure if what I implemented is enough:
> macb_tx_complete() should at least see all completed TX descriptors.
> I still believe there is a (very short) time window in which there
> might be a race:
> 1) HW completes TX descriptor 7 and sets the TX_USED bit
>    in TX descriptor 7.
>    TX descriptor 8 was prefetched with a set TX_USED bit.
> 2) SW sees that TX descriptor 7 is completed
>    (TX_USED bit now is set).
> 3) SW sees that there still is a pending TX descriptor 8.
> 4) SW checks if the TGO bit is still set, which it is.
>    So the SW does nothing at this point.
> 5) HW processes the prefetched,set TX_USED bit in
>    TX descriptor 8 and stops transmission (clearing the TGO bit).
> 
> I am not sure if it is guaranteed that 5) cannot happen after 4).  If 5)
> happens after 4) as described above, then the controller still gets stuck.
> The only idea I can come up with, is to re-check the TGO bit
> a second time a little bit later, but I am not sure how to
> implement this.
> 
> Is there anyone who has access to hardware documentation, which
> sheds some light onto the way the descriptor prefetching works?
> 
> so long
>   Ingo
> 
> 
> Ingo Rohloff (1):
>   net: macb: A different way to restart a stuck TX descriptor ring.
> 
>  drivers/net/ethernet/cadence/macb.h      |  1 -
>  drivers/net/ethernet/cadence/macb_main.c | 67 +++++++++---------------
>  2 files changed, 24 insertions(+), 44 deletions(-)
> 
> --
> 2.17.1
> 

Powered by blists - more mailing lists