[PATCH] can: fix sja1000 pre_irq/post_irq handling

[PATCH] can: fix sja1000 pre_irq/post_irq handling

[Oliver Hartkopp]

The SJA1000 driver provides optional function pointers to handle specific
hardware setups at interrupt time.

This patch fixes the issue that the sja1000_interrupt() function may have
returned IRQ_NONE without processing the post_irq function. Additionally
an introduced return value from the pre_irq function is checked to skip the
entire interrupt handling on pre_irq function errors.

Reported-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

---

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 7164a99..bb20470 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 	uint8_t isrc, status;
 	int n = 0;
 
+	if (priv->pre_irq && priv->pre_irq(priv))
+		goto out;
+
 	/* 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);
+		goto out;
 
 	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
 	       (n < SJA1000_MAX_IRQ)) {
@@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 		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)) {
@@ -544,7 +544,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 				break;
 		}
 	}
-
+out:
 	if (priv->post_irq)
 		priv->post_irq(priv);
 
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
index 9d46398..53c9797 100644
--- a/drivers/net/can/sja1000/sja1000.h
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -157,7 +157,7 @@ struct sja1000_priv {
 	/* the lower-layer is responsible for appropriate locking */
 	u8 (*read_reg) (const struct sja1000_priv *priv, int reg);
 	void (*write_reg) (const struct sja1000_priv *priv, int reg, u8 val);
-	void (*pre_irq) (const struct sja1000_priv *priv);
+	int (*pre_irq) (const struct sja1000_priv *priv);
 	void (*post_irq) (const struct sja1000_priv *priv);
 
 	void *priv;		/* for board-specific data */

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Wolfgang Grandegger]

Hi Oliver,

On 11/17/2013 03:17 PM, Oliver Hartkopp wrote:
> The SJA1000 driver provides optional function pointers to handle specific
> hardware setups at interrupt time.
> 
> This patch fixes the issue that the sja1000_interrupt() function may have
> returned IRQ_NONE without processing the post_irq function. Additionally
> an introduced return value from the pre_irq function is checked to skip the
> entire interrupt handling on pre_irq function errors.
> 
> Reported-by: Wolfgang Grandegger <wg@grandegger.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> ---
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 7164a99..bb20470 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>  	uint8_t isrc, status;
>  	int n = 0;
>  
> +	if (priv->pre_irq && priv->pre_irq(priv))
> +		goto out;
> +

Well, not sure if we need that. Looks like a fatal error to me.

>  	/* 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);
> +		goto out;
>  
>  	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
>  	       (n < SJA1000_MAX_IRQ)) {
> @@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>  		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;

Be aware that the function will return IRQ_HANDLED if n > 0... which may
happen. Not exactly the same behaviour.

>  
>  		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;

Ditto.

>  			}
>  		}
>  		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
> @@ -544,7 +544,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>  				break;
>  		}
>  	}
> -
> +out:
>  	if (priv->post_irq)
>  		priv->post_irq(priv);

Wolfgang.

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Oliver Hartkopp]

On 17.11.2013 21:51, Wolfgang Grandegger wrote:
> Hi Oliver,
> 
> On 11/17/2013 03:17 PM, Oliver Hartkopp wrote:
>> The SJA1000 driver provides optional function pointers to handle specific
>> hardware setups at interrupt time.
>>
>> This patch fixes the issue that the sja1000_interrupt() function may have
>> returned IRQ_NONE without processing the post_irq function. Additionally
>> an introduced return value from the pre_irq function is checked to skip the
>> entire interrupt handling on pre_irq function errors.
>>
>> Reported-by: Wolfgang Grandegger <wg@grandegger.com>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>
>> ---
>>
>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>> index 7164a99..bb20470 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>  	uint8_t isrc, status;
>>  	int n = 0;
>>  
>> +	if (priv->pre_irq && priv->pre_irq(priv))
>> +		goto out;
>> +
> 
> Well, not sure if we need that. Looks like a fatal error to me.
> 

Why?
Think about a pre_irq() function that can look into hw specific registers and
which can determine, that there was no interrupt for this CAN device.
In this case the access to the sja1000 registers might/should be skipped
depending on the new return value of pre_irq().

So what's the error here?
Btw. by now there's no user of priv->pre_irq() anyway.


>>  	/* 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);
>> +		goto out;
>>  
>>  	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&
>>  	       (n < SJA1000_MAX_IRQ)) {
>> @@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>  		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;
> 
> Be aware that the function will return IRQ_HANDLED if n > 0... which may
> happen. Not exactly the same behaviour.
> 

Right.

We should move the 'n++' from the begin to the end of this while statement.

Setting 'n=0' before 'goto out' won't be correct either, as you might have
processed a number of correct CAN frames before.

Regards,
Oliver

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Wolfgang Grandegger]

On Mon, 18 Nov 2013 07:17:19 +0100, Oliver Hartkopp

<socketcan@hartkopp.net> wrote:

> On 17.11.2013 21:51, Wolfgang Grandegger wrote:

>> Hi Oliver,

>> 

>> On 11/17/2013 03:17 PM, Oliver Hartkopp wrote:

>>> The SJA1000 driver provides optional function pointers to handle

>>> specific

>>> hardware setups at interrupt time.

>>>

>>> This patch fixes the issue that the sja1000_interrupt() function may

>>> have

>>> returned IRQ_NONE without processing the post_irq function.

Additionally

>>> an introduced return value from the pre_irq function is checked to

skip

>>> the

>>> entire interrupt handling on pre_irq function errors.

>>>

>>> Reported-by: Wolfgang Grandegger <wg@grandegger.com>

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

>>>

>>> ---

>>>

>>> diff --git a/drivers/net/can/sja1000/sja1000.c

>>> b/drivers/net/can/sja1000/sja1000.c

>>> index 7164a99..bb20470 100644

>>> --- a/drivers/net/can/sja1000/sja1000.c

>>> +++ b/drivers/net/can/sja1000/sja1000.c

>>> @@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void

>>> *dev_id)

>>>  	uint8_t isrc, status;

>>>  	int n = 0;

>>>  

>>> +	if (priv->pre_irq && priv->pre_irq(priv))

>>> +		goto out;

>>> +

>> 

>> Well, not sure if we need that. Looks like a fatal error to me.

>> 

> 

> Why?

> Think about a pre_irq() function that can look into hw specific

registers

> and

> which can determine, that there was no interrupt for this CAN device.

> In this case the access to the sja1000 registers might/should be skipped

> depending on the new return value of pre_irq().



OK, I see. But you should add some comments about the valid return values.



> 

> So what's the error here?

> Btw. by now there's no user of priv->pre_irq() anyway.

> 

> 

>>>  	/* 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);

>>> +		goto out;

>>>  

>>>  	while ((isrc = priv->read_reg(priv, SJA1000_IR)) &&

>>>  	       (n < SJA1000_MAX_IRQ)) {

>>> @@ -507,7 +507,7 @@ irqreturn_t sja1000_interrupt(int irq, void

*dev_id)

>>>  		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;

>> 

>> Be aware that the function will return IRQ_HANDLED if n > 0... which

may

>> happen. Not exactly the same behaviour.

>> 

> 

> Right.

> 

> We should move the 'n++' from the begin to the end of this while

statement.

> 

> Setting 'n=0' before 'goto out' won't be correct either, as you might

have

> processed a number of correct CAN frames before.



I think you code is correct. If at least one interrupt has been handled we

should return IRQ_HANDLED. I just realized that this is another mistake,

apart from not calling "post_irq". Sorry for confusion.



Another thing I don't like is that the device present check is done for

all devices including devices which will never hotplug. I think this

code has been introduced for the PCMCIA/PCcard CAN controllers. I would

prefer somethink like:



         if (priv->hotpluggable && status == 0xFF &&

             sja1000_is_absent(priv)) {

                 ...

         }



What do you think?



Wolfgang.

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Oliver Hartkopp]

On 18.11.2013 08:48, Wolfgang Grandegger wrote:

> 
> 
> Another thing I don't like is that the device present check is done for
> 
> all devices including devices which will never hotplug. I think this
> 
> code has been introduced for the PCMCIA/PCcard CAN controllers. I would
> 
> prefer somethink like:
> 
> 
> 
>          if (priv->hotpluggable && status == 0xFF &&
> 
>              sja1000_is_absent(priv)) {
> 
>                  ...
> 
>          }
> 
> 
> 
> What do you think?

The status variable is retrieved from the sja1000 register anyway and sits in
the CPU register.

Checking the status against 0xFF already hints to several problems (bus-off
together with overrun, etc. ) which are unusual in normal operation.
Only in this unusual cased the sja1000_is_absent() is invoked.

This is pretty optimal.

Accessing priv->hotpluggable needs additional pointer dereferencing which is
more costly than accessing/checking the already retrieved status variable.

Best regards,
Oliver

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Wolfgang Grandegger]

On Mon, 18 Nov 2013 14:13:55 +0100, Oliver Hartkopp

<socketcan@hartkopp.net> wrote:

> On 18.11.2013 08:48, Wolfgang Grandegger wrote:

> 

>> 

>> 

>> Another thing I don't like is that the device present check is done for

>> 

>> all devices including devices which will never hotplug. I think this

>> 

>> code has been introduced for the PCMCIA/PCcard CAN controllers. I would

>> 

>> prefer somethink like:

>> 

>> 

>> 

>>          if (priv->hotpluggable && status == 0xFF &&

>> 

>>              sja1000_is_absent(priv)) {

>> 

>>                  ...

>> 

>>          }

>> 

>> 

>> 

>> What do you think?

> 

> The status variable is retrieved from the sja1000 register anyway and

sits

> in

> the CPU register.

> 

> Checking the status against 0xFF already hints to several problems

(bus-off

> together with overrun, etc. ) which are unusual in normal operation.

> Only in this unusual cased the sja1000_is_absent() is invoked.

> 

> This is pretty optimal.

> 

> Accessing priv->hotpluggable needs additional pointer dereferencing

which

> is

> more costly than accessing/checking the already retrieved status

variable.



You are right, sorry for the noise.



Wolfgang.

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Kurt Van Dijck]

Oliver,

just 2 small remarks below.

On Mon, Nov 18, 2013 at 07:17:19AM +0100, Oliver Hartkopp wrote:
> 
> 
> On 17.11.2013 21:51, Wolfgang Grandegger wrote:
> > Hi Oliver,
> > 
> > On 11/17/2013 03:17 PM, Oliver Hartkopp wrote:
> >> The SJA1000 driver provides optional function pointers to handle specific
> >> hardware setups at interrupt time.
> >>
> >> This patch fixes the issue that the sja1000_interrupt() function may have
> >> returned IRQ_NONE without processing the post_irq function. Additionally
> >> an introduced return value from the pre_irq function is checked to skip the
> >> entire interrupt handling on pre_irq function errors.
> >>
> >> Reported-by: Wolfgang Grandegger <wg@grandegger.com>
> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >>
> >> ---
> >>
> >> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> >> index 7164a99..bb20470 100644
> >> --- a/drivers/net/can/sja1000/sja1000.c
> >> +++ b/drivers/net/can/sja1000/sja1000.c
> >> @@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
> >>  	uint8_t isrc, status;
> >>  	int n = 0;
> >>  
> >> +	if (priv->pre_irq && priv->pre_irq(priv))
> >> +		goto out;
> >> +
> > 
> > Well, not sure if we need that. Looks like a fatal error to me.
> > 
> 
> Why?
> Think about a pre_irq() function that can look into hw specific registers and
> which can determine, that there was no interrupt for this CAN device.
> In this case the access to the sja1000 registers might/should be skipped
> depending on the new return value of pre_irq().
> 
> So what's the error here?

The accuracy of reading register SJA1000_IR remains unbeaten when
deciding if an SJA1000 has interrupts pending.
Any pre_irq() return value is close approximation.

> Btw. by now there's no user of priv->pre_irq() anyway.

"No in-kernel users" is a strong argument for deleting
the pre_irq entirely.

Kind regards,
Kurt

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Wolfgang Grandegger]

On 11/18/2013 08:48 PM, Kurt Van Dijck wrote:
> Oliver,
> 
> just 2 small remarks below.
> 
> On Mon, Nov 18, 2013 at 07:17:19AM +0100, Oliver Hartkopp wrote:
>>
>>
>> On 17.11.2013 21:51, Wolfgang Grandegger wrote:
>>> Hi Oliver,
>>>
>>> On 11/17/2013 03:17 PM, Oliver Hartkopp wrote:
>>>> The SJA1000 driver provides optional function pointers to handle specific
>>>> hardware setups at interrupt time.
>>>>
>>>> This patch fixes the issue that the sja1000_interrupt() function may have
>>>> returned IRQ_NONE without processing the post_irq function. Additionally
>>>> an introduced return value from the pre_irq function is checked to skip the
>>>> entire interrupt handling on pre_irq function errors.
>>>>
>>>> Reported-by: Wolfgang Grandegger <wg@grandegger.com>
>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>>
>>>> ---
>>>>
>>>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>>>> index 7164a99..bb20470 100644
>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>> @@ -494,12 +494,12 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>>  	uint8_t isrc, status;
>>>>  	int n = 0;
>>>>  
>>>> +	if (priv->pre_irq && priv->pre_irq(priv))
>>>> +		goto out;
>>>> +
>>>
>>> Well, not sure if we need that. Looks like a fatal error to me.
>>>
>>
>> Why?
>> Think about a pre_irq() function that can look into hw specific registers and
>> which can determine, that there was no interrupt for this CAN device.
>> In this case the access to the sja1000 registers might/should be skipped
>> depending on the new return value of pre_irq().
>>
>> So what's the error here?
> 
> The accuracy of reading register SJA1000_IR remains unbeaten when
> deciding if an SJA1000 has interrupts pending.
> Any pre_irq() return value is close approximation.
> 
>> Btw. by now there's no user of priv->pre_irq() anyway.
> 
> "No in-kernel users" is a strong argument for deleting
> the pre_irq entirely.

In general yes. But we may rename that callback to "irq_is_pending" and
use it as fast path to "return IRQ_NONE". I could then be used on the
peak_pci to save quite a few instructions.

Wolfgang.

Re: [PATCH] can: fix sja1000 pre_irq/post_irq handling

[Pavel Pisa]

Hello all,

On Monday 18 of November 2013 20:48:33 Kurt Van Dijck wrote:
> Oliver,
>
> just 2 small remarks below.
>
> On Mon, Nov 18, 2013 at 07:17:19AM +0100, Oliver Hartkopp wrote:
> > On 17.11.2013 21:51, Wolfgang Grandegger wrote:
> > So what's the error here?
>
> The accuracy of reading register SJA1000_IR remains unbeaten when
> deciding if an SJA1000 has interrupts pending.
> Any pre_irq() return value is close approximation.
>
> > Btw. by now there's no user of priv->pre_irq() anyway.
>

The use of some PCI to local-bus bridge register to check
SJA1000 interrupt signal/pin state can be much much faster
then SJA1000 IR register access for some hardware. SJA1000 has multiplexed
bus and quite low maximal allowed frequency when compared to today
PCI/PCIe connection (even that PCIe has quite some overhead and latencies
for single register read too - it is optimized for stream/net/video data,
but that is different story).

I have found some while to go through kernel interrupt handling
to refresh my memory and state is as I have it in memory from years ago.

There could be present pathological situation of more edge triggered interrupt 
sources which do not provide way to read actual state of interrupt
line behind bridge read through some interrupt controller/bridge register.
(if the register can be read after interrupts handling then it can be
used to start next loop through handles) But if such (considered broken)
local bus is connected through local bus to PCI bridge then there
are significant problems. Default PCI interrupts behavior is/expect
level triggered logic. It may be possible to reconfigure given
PCI interrupt to IRQ_TYPE_EDGE_RISING or FAILING type, there was even
some set of kernel parameters options for that. But I expect that it
is not possible to request from CAN board driver easily or at all
on x86 PC architecture.

Anyway, even if IRQ_TYPE_EDGE_FAILING type of IRQ is selected then
there is probably problem in

linux-devel/kernel/irq/chip.c:
       handle_edge_irq()

Because it calls handle_irq_event() only once if there is present
next sequence of events

  - chip B signals IRQ
  - handle_irq_event() is called
  - chip A resturns IRQ_NONE
  - chip A event arrives and A signals IRQ but line is held
    by B so no edge appears and IRQS_PENDING is not set
  - chip B is handled stops signalling, returns IRQ_HANDLED,
    but IRQ line is still held by chip A

loop condition in handle_edge_irq() is false and returns even that
hidden IRQ line is active still. The safe handling for such case is
to call handle_irq_event() as long as it does not return IRQ_NONE
(or do as many iterations through action next as one cycle with IRQ_NONE
is reached).
But that such change in handle_edge_irq() or handle_irq_event_percpu()
cannot be done even for testing because last call to handle_irq_event()
would finish with IRQ_NONE and that triggers spurious interrupt handling

linux-devel/kernel/irq/spurious.c:
       note_interrupt()

after couple of IRQs arrival. So even more changes would be required
to support that corner case.

Concussion:

My understanding is that if there is no register which allows to read
actual interrupt signal state behind bridge then there is no clean
kernel level solution and local loop in the driver over sibling
chips behind bridge with spin-locking etc. is required.

If the register to read actual wire-ored or all line states is present
then option is to register IRQ chip for given PCI-localbus bridge
and connect SJA1000 IRQs handling to this IRQ chip node.

I am sending copy to Thomas Gleixner because he can find most quickly
if I am wrong and generic IRQ infrastructure has solution for this
problem or my analysis is wrong (which would be win for solution
of the initially reported problem).

Best wishes,

             Pavel