[PATCH] can: sja1000: drop RX frame when skb allocation fails

[PATCH] can: sja1000: drop RX frame when skb allocation fails

[Yun Lu]

From: Yun Lu <luyun@kylinos.cn>

When alloc_can_skb() fails, sja1000_rx() returns without reading
the frame and without releasing the receive buffer (CMD_RRB), so
SR_RBS stays set. The interrupt handler's inner RX loop

	while (status & SR_RBS) {
		sja1000_rx(dev);
		status = priv->read_reg(priv, SJA1000_SR);
		/* check for absent controller */
		if (status == 0xFF && sja1000_is_absent(priv))
			goto out;
	}

has no iteration limit, so under sustained memory pressure every
retry fails the same way and the loop never exits, livelocking the
CPU in hard IRQ context until the hard lockup detector fires.

Release the receive buffer and count the drop instead, as other
CAN drivers do on allocation failure, so the interrupt handler
always makes progress.

Fixes: 429da1cc841b ("can: Driver for the SJA1000 CAN controller")
Signed-off-by: Yun Lu <luyun@kylinos.cn>
---
 drivers/net/can/sja1000/sja1000.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 3cdb583ee3e5..da107feb22af 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -348,8 +348,12 @@ static void sja1000_rx(struct net_device *dev)
 
 	/* create zero'ed CAN frame buffer */
 	skb = alloc_can_skb(dev, &cf);
-	if (skb == NULL)
+	if (!skb) {
+		/* drop the frame to guarantee forward progress of the ISR */
+		sja1000_write_cmdreg(priv, CMD_RRB);
+		stats->rx_dropped++;
 		return;
+	}
 
 	fi = priv->read_reg(priv, SJA1000_FI);
 
-- 
2.43.0

Re: [PATCH] can: sja1000: drop RX frame when skb allocation fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] In sja1000_err(), an SKB allocation failure returns -ENOMEM, discarding IRQ_WAKE_THREAD requests and causing the interrupt handler to erroneously return IRQ_NONE.
--

commit 6507d559134b8295113fac43d6ce819e79885772
Author: Yun Lu <luyun@kylinos.cn>

can: sja1000: drop RX frame when skb allocation fails

This patch prevents a lockup in sja1000_rx() by dropping the frame and
releasing the receive buffer if SKB allocation fails.

> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 3cdb583ee3e50..da107feb22af7 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -348,8 +348,12 @@ static void sja1000_rx(struct net_device *dev)
>  
>  	/* create zero'ed CAN frame buffer */
>  	skb = alloc_can_skb(dev, &cf);
> -	if (skb == NULL)
> +	if (!skb) {
> +		/* drop the frame to guarantee forward progress of the ISR */
> +		sja1000_write_cmdreg(priv, CMD_RRB);
> +		stats->rx_dropped++;
>  		return;
> +	}
>  
>  	fi = priv->read_reg(priv, SJA1000_FI);

[Severity: High]
This is a pre-existing issue, but while looking at SKB allocation failures
in the interrupt handler, does a similar allocation failure in
sja1000_err() lead to the IRQ line being permanently disabled?

Under memory pressure, if the SKB allocation fails in sja1000_err():

drivers/net/can/sja1000/sja1000.c:sja1000_err() {
    ...
	if (!skb)
		return -ENOMEM;

	netif_rx(skb);

	return ret;
}

This unconditionally returns -ENOMEM, which discards any IRQ_WAKE_THREAD
requests needed for overrun recovery like SJA1000_QUIRK_RESET_ON_OVERRUN.

This error propagates to sja1000_interrupt(), causing the loop to break
before incrementing n:

drivers/net/can/sja1000/sja1000.c:sja1000_interrupt() {
    ...
			err = sja1000_err(dev, isrc, status);
			if (err == IRQ_WAKE_THREAD)
				ret = err;
			if (err)
				break;
		}
		n++;
    ...
}

If this happens on the first loop iteration where n is 0, the handler
returns IRQ_NONE despite having successfully acknowledged the interrupt in
hardware:

drivers/net/can/sja1000/sja1000.c:sja1000_interrupt() {
    ...
out:
	if (!ret)
		ret = (n) ? IRQ_HANDLED : IRQ_NONE;
    ...
}

Could repeated IRQ_NONE returns under sustained memory pressure cause the
core IRQ subsystem to permanently disable the device's IRQ line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903095444.495807-1-luyun_611@163.com?part=1

Re: [PATCH] can: sja1000: drop RX frame when skb allocation fails

[luyun]

在 2026/9/3 18:04, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] In sja1000_err(), an SKB allocation failure returns -ENOMEM, discarding IRQ_WAKE_THREAD requests and causing the interrupt handler to erroneously return IRQ_NONE.
> --
>
> commit 6507d559134b8295113fac43d6ce819e79885772
> Author: Yun Lu <luyun@kylinos.cn>
>
> can: sja1000: drop RX frame when skb allocation fails
>
> This patch prevents a lockup in sja1000_rx() by dropping the frame and
> releasing the receive buffer if SKB allocation fails.
>
>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>> index 3cdb583ee3e50..da107feb22af7 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -348,8 +348,12 @@ static void sja1000_rx(struct net_device *dev)
>>   
>>   	/* create zero'ed CAN frame buffer */
>>   	skb = alloc_can_skb(dev, &cf);
>> -	if (skb == NULL)
>> +	if (!skb) {
>> +		/* drop the frame to guarantee forward progress of the ISR */
>> +		sja1000_write_cmdreg(priv, CMD_RRB);
>> +		stats->rx_dropped++;
>>   		return;
>> +	}
>>   
>>   	fi = priv->read_reg(priv, SJA1000_FI);
> [Severity: High]
> This is a pre-existing issue, but while looking at SKB allocation failures
> in the interrupt handler, does a similar allocation failure in
> sja1000_err() lead to the IRQ line being permanently disabled?
>
> Under memory pressure, if the SKB allocation fails in sja1000_err():
>
> drivers/net/can/sja1000/sja1000.c:sja1000_err() {
>      ...
> 	if (!skb)
> 		return -ENOMEM;
>
> 	netif_rx(skb);
>
> 	return ret;
> }
>
> This unconditionally returns -ENOMEM, which discards any IRQ_WAKE_THREAD
> requests needed for overrun recovery like SJA1000_QUIRK_RESET_ON_OVERRUN.
>
> This error propagates to sja1000_interrupt(), causing the loop to break
> before incrementing n:
>
> drivers/net/can/sja1000/sja1000.c:sja1000_interrupt() {
>      ...
> 			err = sja1000_err(dev, isrc, status);
> 			if (err == IRQ_WAKE_THREAD)
> 				ret = err;
> 			if (err)
> 				break;
> 		}
> 		n++;
>      ...
> }
>
> If this happens on the first loop iteration where n is 0, the handler
> returns IRQ_NONE despite having successfully acknowledged the interrupt in
> hardware:
>
> drivers/net/can/sja1000/sja1000.c:sja1000_interrupt() {
>      ...
> out:
> 	if (!ret)
> 		ret = (n) ? IRQ_HANDLED : IRQ_NONE;
>      ...
> }
>
> Could repeated IRQ_NONE returns under sustained memory pressure cause the
> core IRQ subsystem to permanently disable the device's IRQ line?

Thanks for pointing this out.

The IRQ_WAKE_THREAD result is indeed lost when alloc_can_err_skb()
fails, because sja1000_err() returns -ENOMEM instead of the value stored
in ret. This can prevent the threaded reset handler from running on
controllers with SJA1000_QUIRK_RESET_ON_OVERRUN. It may also cause the
top-level handler to return IRQ_NONE when the failure occurs during the
first loop iteration.

But this is separate from the issue addressed by the current patch. The
current patch fixes the receive path in sja1000_rx(), where an allocation
failure leaves the RX buffer unreleased and prevents the ISR from making
progress. The issue reported here concerns error interrupt handling in
sja1000_err() and the propagation of its IRQ return value.

Would it be better to address this in a separate patch?  Since the two 
issues
affect different paths and have different failure modes, I think this should
be addressed in a separate patch.

Please let me know if you would prefer it to be included in a v2 series 
instead.

---

Thanks.

Yun Lu