RE: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop

[Woojung.Huh]

Hi Nicolai & Chris,

> > > lan743x_tx_close() and lan743x_rx_close() ask the DMA controller to
> > > stop
> > > the channel and wait for it, but ignore the result. If the channel is
> > > still stop-pending when the wait gives up, the descriptor ring, the
> > > head
> > > write-back buffer and the packet buffers are unmapped and freed while
> > > the channel may still be fetching from or writing to them.
> > >
> > > A TX channel has been seen to stop making progress, with its head index
> > > no longer advancing while DMAC_CMD still reports it as started. A
> > > channel in that state may not honour the stop request either, and
> > > closing the interface would then free memory the engine can still
> > > access.
> > >
> > > If the channel is still stop-pending after the wait, or still reads as
> > > started, issue a channel soft reset and wait for it before anything is
> > > freed, and log when that happens.
> > >
> > > Use the same helpers for the channel resets that lan743x_tx_open() and
> > > lan743x_rx_open() already open-code, and log there too if the reset
> > > times out.
> > >
> > > Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x
> > > driver")
> > > Assisted-by: LLM
> > > Signed-off-by: Chris Babroski <cbabroski@xxxxxxxxxx>
> > > ---
> > >  drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++----
> > >  1 file changed, 52 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > > b/drivers/net/ethernet/microchip/lan743x_main.c
> > > index 82d3ec20ba25..9e1276cf313c 100644
> > > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> >
> > > [...]
> >
> > > @@ -2221,11 +2239,22 @@ static int lan743x_tx_ring_init(struct
> > > lan743x_tx *tx)
> > >  static void lan743x_tx_close(struct lan743x_tx *tx)
> > >  {
> > >       struct lan743x_adapter *adapter = tx->adapter;
> > > +     int ret;
> > >
> > >       lan743x_csr_write(adapter,
> > >                         DMAC_CMD,
> > >                         DMAC_CMD_STOP_T_(tx->channel_number));
> > > -     lan743x_dmac_tx_wait_till_stopped(adapter, tx->channel_number);
> > > +     ret = lan743x_dmac_tx_wait_till_stopped(adapter, tx-
> >channel_number);
> > > +     if (ret < 0 || ret == DMAC_CHANNEL_STATE_STARTED) {
> > > +             netdev_warn(adapter->netdev,
> > > +                         "TX channel %d failed to stop, resetting it\n",
> > > +                         tx->channel_number);
> > > +             ret = lan743x_dmac_tx_reset(adapter, tx->channel_number);
> > > +             if (ret)
> > > +                     netdev_err(adapter->netdev,
> > > +                                "TX channel %d reset timed out\n",
> > > +                                tx->channel_number);
> > > +     }
> >
> > While looking something up in the registers for the other thread I
> > stumbled over this in AN2948 about TX_SWR_x:
> >
> > "This is an immediate reset and will cause any, in process, PCIe request
> > to [be] aborted. This may likely result in a corrupt PCIe packet."
> >
> > A channel that did not stop is the one case where a request is still in
> > flight, so this resets right when the datasheet says not to. Is there a
> > sanctioned way to recover a channel that ignores STOP_T?
> >
> > Woojung, maybe you can say?
> >
> 
> Understood your question.
> 
> Talking to hardware engineer whether there is a way beside hitting TX_SWR_x
> in this situation. Will update when getting answered.
> 

Recommended way of setting TX_SWR_x is after stopping and flushing FCT to avoid
in-flight frames during TX DMA reset.

lan743x_dmac_tx_reset() is already called lan743x_tx_open() with proper 
sequence of initializing FCT at lan743x_netdev_open().

lan743x_reset_work() in the patch 4/5 is doing lan743x_netdev_close(), ... and
lan743x_netdev_open(), I think no additional lan743x_dmac_tx_reset() may be
needed at failure of lan743x_dmac_tx_wait_till_stopped() in lan743x_tx_close().

Hi Chris,
Did you see calling lan743x_dmac_tx_reset() at lan743x_tx_close() is a necessary step
in addition to lan743x_dmac_tx_reset() at lan743x_tx_open() when lockup happened?

Best regards,
Woojung