Message-ID: <4a25cee610a8d54e491b863b1b6b6ff117fa9b0d.camel@calian.com>
Date:   Mon, 10 Apr 2023 17:05:17 +0000
From:   Robert Hancock <robert.hancock@...ian.com>
To:     "ingo.rohloff@...terbach.com" <ingo.rohloff@...terbach.com>
CC:     "Nicolas.Ferre@...rochip.com" <Nicolas.Ferre@...rochip.com>,
        "claudiu.beznea@...rochip.com" <claudiu.beznea@...rochip.com>,
        "tomas.melin@...sala.com" <tomas.melin@...sala.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "harini.katakam@...inx.com" <harini.katakam@...inx.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 0/1] Alternative, restart tx after tx used bit read

On Fri, 2023-04-07 at 23:33 +0200, Ingo Rohloff wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you recognize the sender
> and know the content is safe.
> 
> 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")
> 
> 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.
> 
> 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.

I would have a similar concern that a race condition like that could
happen. I suspect in order to fix this properly we would need to know
more about how this prefetch mechanism works and how software was
supposed to cope with it. If it works as simplistically as you
described, it seems like it would inevitably cause a bunch of hard to
handle race conditions and it may be preferable to disable it in the
core if possible.

> 
> 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
> 

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Powered by blists - more mailing lists