* [PATCH v5 1/2] can: sja1000: fix pre_irq() and post_irq() handling
2013-11-24 21:44 [PATCH v5 1/2] can: sja1000: fix "irq X: nobody cared" problem Marc Kleine-Budde
@ 2013-11-24 21:45 ` Marc Kleine-Budde
2013-11-24 21:45 ` [PATCH v5 2/2] can: sja1000: fix return value of interrupt handler Marc Kleine-Budde
2013-11-25 7:57 ` [PATCH v5 1/2] can: sja1000: fix "irq X: nobody cared" problem Wolfgang Grandegger
2 siblings, 0 replies; 6+ messages in thread
From: Marc Kleine-Budde @ 2013-11-24 21:45 UTC (permalink / raw)
To: linux-can; +Cc: kernel, kurt.van.dijck, Oliver Hartkopp, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
It has been reported that on PREEMPT_RT enabled system the peak SJA1000
hardware can trigger "irq X: nobody cared" errors.
This patch fixes the issue that the sja1000_interrupt() function may have
returned IRQ_NONE without processing the optional pre_irq() and post_irq()
function before.
Reported-by: Wolfgang Grandegger <wg@grandegger.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/sja1000/sja1000.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 7164a99..253845f 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -494,13 +494,13 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
uint8_t isrc, status;
int n = 0;
- /* Shared interrupts and IRQ off? */
- if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
- return IRQ_NONE;
-
if (priv->pre_irq)
priv->pre_irq(priv);
+ /* Shared interrupts and IRQ off? */
+ if (priv->read_reg(priv, SJA1000_IER) == IRQ_OFF)
+ goto out;
+
while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
(n < SJA1000_MAX_IRQ)) {
n++;
@@ -544,7 +544,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
break;
}
}
-
+out:
if (priv->post_irq)
priv->post_irq(priv);
--
1.8.5.rc0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v5 2/2] can: sja1000: fix return value of interrupt handler
2013-11-24 21:44 [PATCH v5 1/2] can: sja1000: fix "irq X: nobody cared" problem Marc Kleine-Budde
2013-11-24 21:45 ` [PATCH v5 1/2] can: sja1000: fix pre_irq() and post_irq() handling Marc Kleine-Budde
@ 2013-11-24 21:45 ` Marc Kleine-Budde
2013-11-24 22:38 ` Oliver Hartkopp
2013-11-25 7:57 ` [PATCH v5 1/2] can: sja1000: fix "irq X: nobody cared" problem Wolfgang Grandegger
2 siblings, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2013-11-24 21:45 UTC (permalink / raw)
To: linux-can; +Cc: kernel, kurt.van.dijck, Oliver Hartkopp, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
The irq processing counter 'n' is moved to the end of the while statement to
return correct IRQ_[NONE|HANDLED] values at error conditions.
Reported-by: Wolfgang Grandegger <wg@grandegger.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
drivers/net/can/sja1000/sja1000.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 253845f..f17c301 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -503,11 +503,11 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
(n < SJA1000_MAX_IRQ)) {
- n++;
+
status = priv->read_reg(priv, SJA1000_SR);
/* check for absent controller due to hw unplug */
if (status == 0xFF && sja1000_is_absent(priv))
- return IRQ_NONE;
+ goto out;
if (isrc & IRQ_WUI)
netdev_warn(dev, "wakeup interrupt\n");
@@ -535,7 +535,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
status = priv->read_reg(priv, SJA1000_SR);
/* check for absent controller */
if (status == 0xFF && sja1000_is_absent(priv))
- return IRQ_NONE;
+ goto out;
}
}
if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
@@ -543,6 +543,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
if (sja1000_err(dev, isrc, status))
break;
}
+ n++;
}
out:
if (priv->post_irq)
--
1.8.5.rc0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v5 2/2] can: sja1000: fix return value of interrupt handler
2013-11-24 21:45 ` [PATCH v5 2/2] can: sja1000: fix return value of interrupt handler Marc Kleine-Budde
@ 2013-11-24 22:38 ` Oliver Hartkopp
2013-11-24 22:44 ` Marc Kleine-Budde
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2013-11-24 22:38 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: kernel, kurt.van.dijck
Hi Marc,
the reason why Kurt asked to split my patch was because of i added a change in pre_irq () to return a value. splitting up the patch as you did here IMO makes no sense. That is why I sent a new patch some days ago only addressing the fix without the 'return value enhancement'.
Best regards,
Oliver
Marc Kleine-Budde <mkl@pengutronix.de> schrieb:
>From: Oliver Hartkopp <socketcan@hartkopp.net>
>
>The irq processing counter 'n' is moved to the end of the while
>statement to
>return correct IRQ_[NONE|HANDLED] values at error conditions.
>
>Reported-by: Wolfgang Grandegger <wg@grandegger.com>
>Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>---
> drivers/net/can/sja1000/sja1000.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/can/sja1000/sja1000.c
>b/drivers/net/can/sja1000/sja1000.c
>index 253845f..f17c301 100644
>--- a/drivers/net/can/sja1000/sja1000.c
>+++ b/drivers/net/can/sja1000/sja1000.c
>@@ -503,11 +503,11 @@ irqreturn_t sja1000_interrupt(int irq, void
>*dev_id)
>
> while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
> (n < SJA1000_MAX_IRQ)) {
>- n++;
>+
> status = priv->read_reg(priv, SJA1000_SR);
> /* check for absent controller due to hw unplug */
> if (status == 0xFF && sja1000_is_absent(priv))
>- return IRQ_NONE;
>+ goto out;
>
> if (isrc & IRQ_WUI)
> netdev_warn(dev, "wakeup interrupt\n");
>@@ -535,7 +535,7 @@ irqreturn_t sja1000_interrupt(int irq, void
>*dev_id)
> status = priv->read_reg(priv, SJA1000_SR);
> /* check for absent controller */
> if (status == 0xFF && sja1000_is_absent(priv))
>- return IRQ_NONE;
>+ goto out;
> }
> }
> if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>@@ -543,6 +543,7 @@ irqreturn_t sja1000_interrupt(int irq, void
>*dev_id)
> if (sja1000_err(dev, isrc, status))
> break;
> }
>+ n++;
> }
> out:
> if (priv->post_irq)
--
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v5 1/2] can: sja1000: fix "irq X: nobody cared" problem
2013-11-24 21:44 [PATCH v5 1/2] can: sja1000: fix "irq X: nobody cared" problem Marc Kleine-Budde
2013-11-24 21:45 ` [PATCH v5 1/2] can: sja1000: fix pre_irq() and post_irq() handling Marc Kleine-Budde
2013-11-24 21:45 ` [PATCH v5 2/2] can: sja1000: fix return value of interrupt handler Marc Kleine-Budde
@ 2013-11-25 7:57 ` Wolfgang Grandegger
2 siblings, 0 replies; 6+ messages in thread
From: Wolfgang Grandegger @ 2013-11-25 7:57 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, kernel, kurt.van.dijck
Hi Marc,
On Sun, 24 Nov 2013 22:44:59 +0100, Marc Kleine-Budde <mkl@pengutronix.de>
wrote:
> Hello,
>
> as Kurt suggested I've split up the patch into two parts. I've
added/keept
> Wolfgang's Acks at both parts, hope that's okay. Further I've added to
the
> description of the first patch, that it fixes the "irq X: nobody cared".
Is
> this corrrect? If I understand the discussion and code correct, the
second
> patch is only relevant in the hot unplug case.
It was Austin Schuh who reported a problem with -rt, which do not show up
with
vanilla Linux. See thread with the subject "sja1000 interrupt problem".
It's
still unclear what the real cause of this problem is. Anyway, Oliver and
myself
looked into the SJA1000 code and realized some inconsistencies with the
interrupt acknowledgement via "post_irq" callback. Oliver already
clarified
that.
Wolfgang.
^ permalink raw reply [flat|nested] 6+ messages in thread