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

[Chris Babroski]

Hi Woojung and Nicolai,

> > > @@ -2271,11 +2300,11 @@ static int lan743x_tx_open(struct lan743x_tx
> > > *tx)
> > >                         FCT_TX_CTL_EN_(tx->channel_number));
> > >
> > >       /* reset tx channel */
> > > -     lan743x_csr_write(adapter, DMAC_CMD,
> > > -                       DMAC_CMD_TX_SWR_(tx->channel_number));
> > > -     lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
> > > -                              DMAC_CMD_TX_SWR_(tx->channel_number),
> > > -                              0, 1000, 20000, 100);
> > > +     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);
> > 
> > Not a bug, but what's the benefit of keeping going here?
> > 
> > If the channel never came out of reset, lan743x_tx_open() and
> > lan743x_rx_open() could return the error instead of starting it anyway.
> > 
> > With the reset work from 4/5 a failed reopen then ends in dev_close(),
> > which seems like the better place to end up. It would also settle the
> > open question from the cover letter.
> > 
> Agree with Nicolai's comment that returning error at lan743x_tx/rx_open() to
> make lan743x_netdev_open() returns with error.

Sounds good. I'll update V2 to handle the timeout error by cleaning up
previously initialized steps and then return the error.

One thing worth noting is that every call to lan743x_csr_wait_for_bit()
could technically time out and most cases are unhandled by the driver
today. My goal was to avoid making too many unrelated changes as part of
this patch series, but more error handling could potentially be added as
part of a future series.

Thanks,
Chris