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

[Woojung.Huh]

Hi Nicola

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

Best regards,
Woojung