The hardware can report an out-of-range tpd_cons (seen as 0xffff)
while the PCIe link/MAC is resetting. An out-of-range value can
never be reached and the loop below would spin forever. To avoid
a soft lockup treat it as "nothing new to clean" instead.

Signed-off-by: Gajdos Tamás <tamas@rimpianto.com>
---
 drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 7efa3fc257..e58f1d2c26 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1602,6 +1602,9 @@ static int atl1c_clean_tx(struct napi_struct *napi, int budget)
 	AT_READ_REGW(&adapter->hw, atl1c_qregs[tpd_ring->num].tpd_cons,
 		     &hw_next_to_clean);
 
+	if (unlikely(hw_next_to_clean >= tpd_ring->count))
+		hw_next_to_clean = next_to_clean;
+
 	while (next_to_clean != hw_next_to_clean) {
 		buffer_info = &tpd_ring->buffer_info[next_to_clean];
 		if (buffer_info->skb) {
-- 
2.53.0

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: atl1c: fix soft lockup on out-of-range tpd_cons read

This adds a bounds check on the TX consumer index read from the hardware
in atl1c_clean_tx(), so that an out-of-range value (reported as 0xffff)
is treated as "nothing to clean" rather than driving the cleanup loop
forever.

> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 7efa3fc257b39..e58f1d2c26bdf 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1602,6 +1602,9 @@ static int atl1c_clean_tx(struct napi_struct *napi, int budget)
>  	AT_READ_REGW(&adapter->hw, atl1c_qregs[tpd_ring->num].tpd_cons,
>  		     &hw_next_to_clean);
>  

[Severity: Medium]
Should this carry a Fixes: tag and a Cc: stable@vger.kernel.org?

git blame puts the unbounded loop in atl1c_clean_tx() at commit
43250ddd75a35d ("atl1c: Atheros L1C Gigabit Ethernet driver"), and the
per-queue register form of the read at commit 057f4af2b1716f, so all
stable trees appear to be affected:

Fixes: 43250ddd75a35d ("atl1c: Atheros L1C Gigabit Ethernet driver")

The loop really cannot terminate on its own: tpd_ring->count is fixed at
1024 in atl1c_setup_ring_resources() and the driver has no ethtool
ringparam callback, so a 0xffff read never matches any wrapped
next_to_clean value, and atl1c_clean_buffer() returns immediately once
the entries carry ATL1C_BUFFER_FREE.

Could the changelog also say how the 0xffff value was observed - kernel
version, hardware, and the log or reproducer? That makes the hardware
claim easier to confirm.

[Severity: High]
This isn't a bug introduced by this patch, but the sibling Atheros
drivers have the same loop and are left untouched here. Were they
audited?

atl1e_clean_tx_irq() in drivers/net/ethernet/atheros/atl1e/atl1e_main.c
uses the same accessor and the same modulo-count loop:

	u16 hw_next_to_clean = AT_READ_REGW(&adapter->hw, REG_TPD_CONS_IDX);
	u16 next_to_clean = atomic_read(&tx_ring->next_to_clean);

	while (next_to_clean != hw_next_to_clean) {

An all-ones read from a dead or resetting PCIe link is generic bus
behaviour, so this spins forever in softirq context on atl1e too.

atl1_intr_tx() in drivers/net/ethernet/atheros/atlx/atl1.c has the same
shape, driven by the device-written CMB field:

	cmb_tpd_next_to_clean = le16_to_cpu(adapter->cmb.cmb->tpd_cons_idx);

	while (cmb_tpd_next_to_clean != sw_tpd_next_to_clean) {

Would a note in the changelog about these two, or matching patches, make
sense?

> +	if (unlikely(hw_next_to_clean >= tpd_ring->count))
> +		hw_next_to_clean = next_to_clean;
> +
>  	while (next_to_clean != hw_next_to_clean) {
>  		buffer_info = &tpd_ring->buffer_info[next_to_clean];
>  		if (buffer_info->skb) {

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but the
"while the PCIe link/MAC is resetting" scenario named in the changelog
looks like it also has an unsynchronized reset-versus-poll window that
this clamp does not close.

atl1c_common_task() handles the link change by masking interrupts and
then resetting the rings from process context:

	if (test_and_clear_bit(ATL1C_WORK_EVENT_LINK_CHANGE,
		&adapter->work_event)) {
		atl1c_irq_disable(adapter);
		atl1c_check_link_status(adapter);
		atl1c_irq_enable(adapter);
	}

atl1c_irq_disable() only masks IMR and waits for the hard IRQ handler:

	atomic_inc(&adapter->irq_sem);
	AT_WRITE_REG(&adapter->hw, REG_IMR, 0);
	AT_WRITE_REG(&adapter->hw, REG_ISR, ISR_DIS_INT);
	AT_WRITE_FLUSH(&adapter->hw);
	synchronize_irq(adapter->pdev->irq);

There is no napi_disable() or napi_synchronize() on this path, unlike
atl1c_down(). On link down, atl1c_check_link_status() then goes on to
atl1c_reset_mac(), atl1c_reset_dma_ring() and atl1c_configure() with all
TX NAPI instances still enabled.

atl1c_reset_dma_ring() -> atl1c_clean_tx_ring() walks every entry and
resets the index:

	for (index = 0; index < ring_count; index++) {
		buffer_info = &tpd_ring->buffer_info[index];
		atl1c_clean_buffer(pdev, buffer_info, 0);
	}
	...
	atomic_set(&tpd_ring->next_to_clean, 0);

Meanwhile atl1c_clean_tx() calls atl1c_clean_buffer() on the same
entries and writes its entry-time snapshot back:

	atl1c_clean_buffer(pdev, buffer_info, budget);
	if (++next_to_clean == tpd_ring->count)
		next_to_clean = 0;
	atomic_set(&tpd_ring->next_to_clean, next_to_clean);

The only guard against releasing a buffer twice is a plain read of a u16
field in atl1c_clean_buffer():

	if (buffer_info->flags & ATL1C_BUFFER_FREE)
		return;

Can both contexts pass that test for the same buffer_info and end up
doing two dma_unmap_single() and two napi_consume_skb() calls on it?
And can the poll's atomic_set() overwrite the reset path's
next_to_clean = 0, leaving the software index out of sync with the
freshly reset hardware?

Would adding napi_disable()/napi_synchronize() around the reset in the
link-change path be the right complement to this clamp? The clamp still
looks needed on its own, since surprise removal produces all-ones reads
that NAPI synchronization cannot prevent.
-- 
pw-bot: cr

Thanks for the review.

> Should this carry a Fixes: tag and a Cc: stable@vger.kernel.org?
>
> Fixes: 43250ddd75a35d ("atl1c: Atheros L1C Gigabit Ethernet driver")

Agreed on both, will add in v2.

> Could the changelog also say how the 0xffff value was observed - kernel
> version, hardware, and the log or reproducer? That makes the hardware
> claim easier to confirm.

Reproduced on two machines, same NIC (Qualcomm Atheros AR8151 v2.0,
4-port), triggered by rebooting a Mikrotik CCR2004 PCIe card that the
ports are directly linked to:

- Ubuntu 26.04.1 LTS, kernel 7.0.0-31-generic. The link-flap precursor,
  before the lockup was captured with a full trace elsewhere:

    atl1c 0000:05:00.0 enp5s0f0: NETDEV WATCHDOG: CPU: 4: transmit queue 2 timed out 489984 ms
    atl1c 0000:05:00.0: MAC state machine can't be idle since disabled for 10ms second
    atl1c 0000:05:00.0: atl1c: enp5s0f0 NIC Link is Up<65535 Mbps Full Duplex>

  65535 (0xffff) here is the same value the tpd_cons register reads
  back once the loop this patch fixes gets stuck.

- Proxmox VE, kernel 7.0.14-11-pve. Same NIC/trigger, this time caught
  by the soft lockup watchdog with a full stack trace:

    watchdog: BUG: soft lockup - CPU#12 stuck for 354s! [napi/eth%d-0:329]
    CPU: 12 UID: 0 PID: 329 Comm: napi/eth%d-0 Tainted: P O L 7.0.14-11-pve #1 PREEMPT(lazy)
    RIP: 0010:atl1c_clean_tx+0x142/0x2d0 [atl1c]
    Call Trace:
     <TASK>
     __napi_poll+0x32/0x1e0
     napi_threaded_poll_loop+0x286/0x2e0
     napi_threaded_poll+0xfd/0x140
     kthread+0xf7/0x130
     ret_from_fork+0x2da/0x3a0
     ret_from_fork_asm+0x1a/0x30
     </TASK>

Will fold both into the v2 changelog.

> This isn't a bug introduced by this patch, but the sibling Atheros
> drivers have the same loop and are left untouched here. Were they
> audited?

Yes - sending this as a 3-patch series (atl1c/atl1e/atl1), same guard
applied to atl1e_clean_tx_irq() and atl1_intr_tx(). One caveat worth
being upfront about: only the atl1c fix has been validated against
real hardware (repeated reboot cycles against the CCR2004 above,
confirmed fixed). The atl1e and atl1 patches apply the identical guard
against the identical loop shape by code inspection - I don't have
atl1e/atl1 hardware to reproduce and confirm the fix against directly.
Flagging this explicitly rather than implying equivalent test coverage
across all three.

> Would adding napi_disable()/napi_synchronize() around the reset in
> the link-change path be the right complement to this clamp?

Agreed this looks real - still confirming the right fix, will follow
up separately since it's an independent bug from the one this patch
addresses.