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

[Chris Babroski]

> [Severity: High]
> What happens if this channel reset also times out?
> 
> The commit message says the soft reset is issued "and wait for it before
> anything is freed". However, if lan743x_dmac_tx_reset() returns -
> ETIMEDOUT,
> the code only logs it, and lan743x_tx_close() goes on with the normal
> teardown:
> 
> lan743x_tx_close()
>     ...
>     lan743x_tx_release_all_descriptors(tx);  /* unmap and free skbs */
>     ...
>     lan743x_tx_ring_cleanup(tx);  /* dma_free_coherent() ring and head */
> 
> The commit message describes a wedged channel that may not honour
> the stop
> request. Could that channel ignore the soft reset as well? It would then
> keep fetching descriptors from, or writing the head index into, memory
> that
> has already been freed and reused.
> 
> lan743x_rx_close() has the same pattern. After a timed-out RX reset,
> lan743x_rx_ring_cleanup() unmaps and frees every RX skb, even though
> their
> descriptors still have OWN set. It then frees the descriptor ring and the
> head write-back buffer. On RX, the device could write packet data into
> freed pages.
> 

As discussed in another thread for this patch: for V2 we will remove the
lan743x_dmac_tx_reset() from lan743x_tx_close() because it is unnecessary
for recovery. This will make the code appear effectively the same as it was before.
The recovery happens in the following lan743x_tx_open() which
does FIFO and DMAC reset. We have seen that this recovery mechanism is
working as expected.

Changing the order of lan743x_tx_close() is out of scope for this patch series.

> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch. The old
> code also discarded the return value of lan743x_csr_wait_for_bit(), and
> this change only adds a log message.
> 
> After a failed reset, lan743x_tx_open() still programs TX_BASE_ADDR,
> TX_HEAD_WRITEBACK_ADDR, TX_CFG_A/B/C and TX_TAIL. It then writes
> DMAC_CMD_START_T_ and returns 0.
>  ...
> lan743x_rx_open() handles the reset the same way.
> ...
> Should the open paths return an error when the channel reset times out?

As discussed in another thread for this patch: in V2 we will return an error
after cleaning up the previous initialization steps that occurred before time out.

Thanks,
Chris

pw-bot: cr