[PATCH] can: sja1000: fix max irq loop handling

[PATCH] can: sja1000: fix max irq loop handling

[Thomas Mühlbacher]

Reading the interrupt register `SJA1000_IR` causes all of its bits to be
reset. If we ever reach the condition of handling more than
`SJA1000_MAX_IRQ` IRQs, we will have read the register and reset all its
bits but without actually handling the interrupt inside of the loop
body.

This may, among other issues, cause us to never `netif_wake_queue()`
again after a transmission interrupt.

Fixes: 429da1cc841b ("can: Driver for the SJA1000 CAN controller")
Signed-off-by: Thomas Mühlbacher <tmuehlbacher@posteo.net>
---
 drivers/net/can/sja1000/sja1000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4d245857ef1c..83476af8adb5 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -548,8 +548,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
 		goto out;
 
-	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
-	       (n < SJA1000_MAX_IRQ)) {
+	while ((n < SJA1000_MAX_IRQ) &&
+	       (isrc = priv->read_reg(priv, SJA1000_IR))) {
 
 		status = priv->read_reg(priv, SJA1000_SR);
 		/* check for absent controller due to hw unplug */
-- 
2.51.2

Re: [PATCH] can: sja1000: fix max irq loop handling

[Oliver Hartkopp]

On 15.11.25 16:34, Thomas Mühlbacher wrote:
> Reading the interrupt register `SJA1000_IR` causes all of its bits to be
> reset. If we ever reach the condition of handling more than
> `SJA1000_MAX_IRQ` IRQs, we will have read the register and reset all its
> bits but without actually handling the interrupt inside of the loop
> body.
> 
> This may, among other issues, cause us to never `netif_wake_queue()`
> again after a transmission interrupt.
> 
> Fixes: 429da1cc841b ("can: Driver for the SJA1000 CAN controller")
> Signed-off-by: Thomas Mühlbacher <tmuehlbacher@posteo.net>

Good catch in this ~20 yo code!

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks Thomas!

> ---
>   drivers/net/can/sja1000/sja1000.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 4d245857ef1c..83476af8adb5 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -548,8 +548,8 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>   	if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
>   		goto out;
>   
> -	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
> -	       (n < SJA1000_MAX_IRQ)) {
> +	while ((n < SJA1000_MAX_IRQ) &&
> +	       (isrc = priv->read_reg(priv, SJA1000_IR))) {
>   
>   		status = priv->read_reg(priv, SJA1000_SR);
>   		/* check for absent controller due to hw unplug */

Re: [PATCH] can: sja1000: fix max irq loop handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On 15.11.2025 15:34:56, Thomas Mühlbacher wrote:
> Reading the interrupt register `SJA1000_IR` causes all of its bits to be
> reset. If we ever reach the condition of handling more than
> `SJA1000_MAX_IRQ` IRQs, we will have read the register and reset all its
> bits but without actually handling the interrupt inside of the loop
> body.
>
> This may, among other issues, cause us to never `netif_wake_queue()`
> again after a transmission interrupt.

Applied to linux-can and added stable to Cc. Out of curiosity, what
hardware are you using that has an SJA1000 IP core built in?

regards,
Marc
-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: sja1000: fix max irq loop handling

[Thomas Mühlbacher]

Thanks for reviewing.

Marc Kleine-Budde <mkl@pengutronix.de> writes:
> On 15.11.2025 15:34:56, Thomas Mühlbacher wrote:
>> Reading the interrupt register `SJA1000_IR` causes all of its bits to be
>> reset. If we ever reach the condition of handling more than
>> `SJA1000_MAX_IRQ` IRQs, we will have read the register and reset all its
>> bits but without actually handling the interrupt inside of the loop
>> body.
>>
>> This may, among other issues, cause us to never `netif_wake_queue()`
>> again after a transmission interrupt.
>
> Applied to linux-can and added stable to Cc. Out of curiosity, what
> hardware are you using that has an SJA1000 IP core built in?

It's a B&R APC3200, relying on the sja1000_isa module. Currently
debugging something there. However, this I actually found from staring
at the code and data sheet for a while.

Best,
Thomas