Re: [PATCH net-next v3 08/13] net: lan966x: add shutdown callback to stop FDMA on reboot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `napi_disable()` on an uninitialized NAPI context during shutdown causes a kernel panic if the switch is probed without active ports.
- [High] Network queues are not stopped and locks are not acquired during shutdown, allowing a concurrent `ndo_start_xmit` to reactivate the TX hardware channel.
- [Medium] Incorrect teardown ordering in `lan966x_shutdown()` allows concurrent NAPI polling to race with hardware channel disablement.
--

commit cad51ac4b346e31972387c9d5600ef4d31ee8c03
Author: Daniel Machon <daniel.machon@microchip.com>
net: lan966x: add shutdown callback to stop FDMA on reboot

This commit adds a shutdown callback to mask FDMA interrupts and disable
the RX and TX channels so that the PCIe INTx does not assert before the
driver has re-probed after a reboot.

Note: The format of this review has been altered to use a free-form summary
rather than inline patch quoting due to recitation restrictions.

Does the new shutdown routine properly handle devices probed with no active
ports?

The shutdown callback checks if the fdma flag is set and then proceeds to
call napi_disable() unconditionally. However, if the device tree specifies
no active ports, the NAPI structure might not be initialized since
lan966x_fdma_netdev_init() runs inside a port iteration loop, even though
the fdma flag is set to true early in the probe sequence.

Calling napi_disable() on an uninitialized NAPI context will cause it to
dereference a NULL dev pointer to acquire the netdev lock, leading to a
panic on shutdown. Should the code verify NAPI initialization, perhaps by
checking lan966x->fdma_ndev, before attempting to disable NAPI?

Could a concurrent transmission reactivate the TX hardware channel during
shutdown?

The shutdown routine calls lan966x_fdma_tx_disable(), which stops the
channel and sets tx->activated to false. This happens without stopping the
network transmit queues or acquiring the transmit lock.

If a concurrent transmission occurs, the network stack could invoke
lan966x_fdma_xmit(), acquire the lock, and call lan966x_fdma_tx_start().
Because tx->activated was just set to false, the transmit start routine
might write to the hardware to restart the channel. This would leave the
DMA channel active across the reset, defeating the purpose of the shutdown
callback.

Would it be safer to detach the device (e.g., using netif_device_detach)
or acquire the appropriate locks before disabling the TX channel?

Is the teardown ordering in the shutdown callback susceptible to a race
condition with the NAPI polling thread?

Currently, the hardware channels are disabled before NAPI is disabled and
the hardware interrupts are masked. Because interrupts and NAPI are still
active, a hardware interrupt could fire while the RX channel is being
disabled, scheduling NAPI.

This would cause the NAPI poll routine to execute concurrently with the
teardown process, potentially accessing descriptors or writing to reload
registers while the channel is discarding databases and stopping.

Should the shutdown callback mask hardware interrupts and call
napi_disable() to cleanly stop the software polling thread before writing
to the hardware to stop the DMA channels?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504-lan966x-pci-fdma-v3-0-a56f5740d870@microchip.com?part=8