[PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[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. Further 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>
---
Changes since v5: make it one patch again.

 drivers/net/can/sja1000/sja1000.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 7164a99..f17c301 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -494,20 +494,20 @@ 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++;
+
 		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,8 +543,9 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 			if (sja1000_err(dev, isrc, status))
 				break;
 		}
+		n++;
 	}
-
+out:
 	if (priv->post_irq)
 		priv->post_irq(priv);
 
-- 
1.8.5.rc0

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Marc Kleine-Budde]

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

On 11/25/2013 12:03 AM, Marc Kleine-Budde wrote:
> 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. Further the irq processing counter 'n' is moved to the end of
> the while statement to return correct IRQ_[NONE|HANDLED] values at error
> conditions.

Austin, can you test if this patch solves your problem.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Oliver Hartkopp]

On 25.11.2013 09:54, Marc Kleine-Budde wrote:
> On 11/25/2013 12:03 AM, Marc Kleine-Budde wrote:
>> 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 sentence of the description is not really related to the patch.

We all looked into the sja1000 driver due to the issue Austin reported and
detected the general problem in error handling then.

With PREEMPT_RT there are irq threads which might lead to Austins specific
problem ...

>>
>> 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. Further the irq processing counter 'n' is moved to the end of
>> the while statement to return correct IRQ_[NONE|HANDLED] values at error
>> conditions.
> 
> Austin, can you test if this patch solves your problem.

Austin should test it, right.

But that should not delay sending this patch to net/stable tree.

Sorry for the confusion. Following the mails must have been hard ;-)

Best regards,
Oliver

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Austin Schuh]

I'll test the patch on the the 2nd when I have access to the machine
again.  Thanks!

Austin

On Mon, Nov 25, 2013 at 12:54 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 11/25/2013 12:03 AM, Marc Kleine-Budde wrote:
>> 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. Further the irq processing counter 'n' is moved to the end of
>> the while statement to return correct IRQ_[NONE|HANDLED] values at error
>> conditions.
>
> Austin, can you test if this patch solves your problem.
>
> Marc
>
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Richard Andrysek]

Marc Kleine-Budde <mkl <at> pengutronix.de> writes:

> 
> From: Oliver Hartkopp <socketcan <at> 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. Further 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 <at> grandegger.com>
> Acked-by: Wolfgang Grandegger <wg <at> grandegger.com>
> Signed-off-by: Oliver Hartkopp <socketcan <at> hartkopp.net>
> Signed-off-by: Marc Kleine-Budde <mkl <at> pengutronix.de>
> ---
> Changes since v5: make it one patch again.
> 
>  drivers/net/can/sja1000/sja1000.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c
b/drivers/net/can/sja1000/sja1000.c
> index 7164a99..f17c301 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
>  <at>  <at>  -494,20 +494,20  <at>  <at>  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++;
> +
>  		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");
>  <at>  <at>  -535,7 +535,7  <at>  <at>  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)) {
>  <at>  <at>  -543,8 +543,9  <at>  <at>  irqreturn_t sja1000_interrupt(int
irq, void *dev_id)
>  			if (sja1000_err(dev, isrc, status))
>  				break;
>  		}
> +		n++;
>  	}
> -
> +out:
>  	if (priv->post_irq)
>  		priv->post_irq(priv);
> 

I've used a patch and now I see an another effect.
I see with a dmesg like this: [16773.935134] peak_pci 0000:03:0b.0 can0:
data overrun interrupt
 
	1)Why there are reported as a frame error, by calling ifconfig –a?
        $ sudo dmesg | grep "data overrun interrupt" | wc
          17     136    1122

        ~$ sudo ifconfig can0
         can0      Link encap:UNSPEC  HWaddr
00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
          UP RUNNING NOARP  MTU:16  Metric:1
          RX packets:12711052 errors:17 dropped:0 overruns:0 frame:17
          TX packets:15023217 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:10
          RX bytes:101688416 (96.9 MiB)  TX bytes:109206486 (104.1 MiB)
          Interrupt:19


          Rx errors:17  == wc over "data overrun interrupt": 17 == RX frame
: 17.
 
	2) How the data overrun can happen, if the can bus load  is ~25%?
	 
	$ sudo cat /proc/net/can/stats
	[sudo] password for ran:
 
  1421819 transmitted frames (TXF)
  2606493 received frames (RXF)
   473873 matched frames (RXMF)
 
       18 % total match ratio (RXMR)
     2400 frames/s total tx rate (TXR)
     4400 frames/s total rx rate (RXR)
 
       18 % current match ratio (CRXMR)
     2400 frames/s current tx rate (CTXR)
     4400 frames/s current rx rate (CRXR)
 
       18 % max match ratio (MRXMR)
     2427 frames/s max tx rate (MTXR)
     4427 frames/s max rx rate (MRXR)
 
        4 current receive list entries (CRCV)
        9 maximum receive list entries (MRCV)
 
	        2 statistic resets (STR)

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Wolfgang Grandegger]

Hi Richard,

On 12/05/2013 04:50 PM, Richard Andrysek wrote:
> Marc Kleine-Budde <mkl <at> pengutronix.de> writes:
> 
>>
>> From: Oliver Hartkopp <socketcan <at> 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. Further 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 <at> grandegger.com>
>> Acked-by: Wolfgang Grandegger <wg <at> grandegger.com>
>> Signed-off-by: Oliver Hartkopp <socketcan <at> hartkopp.net>
>> Signed-off-by: Marc Kleine-Budde <mkl <at> pengutronix.de>
>> ---
>> Changes since v5: make it one patch again.
>>
>>  drivers/net/can/sja1000/sja1000.c | 17 +++++++++--------
>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/can/sja1000/sja1000.c
> b/drivers/net/can/sja1000/sja1000.c
>> index 7164a99..f17c301 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>>  <at>  <at>  -494,20 +494,20  <at>  <at>  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++;
>> +
>>  		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");
>>  <at>  <at>  -535,7 +535,7  <at>  <at>  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)) {
>>  <at>  <at>  -543,8 +543,9  <at>  <at>  irqreturn_t sja1000_interrupt(int
> irq, void *dev_id)
>>  			if (sja1000_err(dev, isrc, status))
>>  				break;
>>  		}
>> +		n++;
>>  	}
>> -
>> +out:
>>  	if (priv->post_irq)
>>  		priv->post_irq(priv);
>>
> 
> I've used a patch and now I see an another effect.

You mean, the data overruns do not happen without the pach above?

> I see with a dmesg like this: [16773.935134] peak_pci 0000:03:0b.0 can0:
> data overrun interrupt
>  
> 	1)Why there are reported as a frame error, by calling ifconfig –a?
>         $ sudo dmesg | grep "data overrun interrupt" | wc
>           17     136    1122
> 
>         ~$ sudo ifconfig can0
>          can0      Link encap:UNSPEC  HWaddr
> 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
>           UP RUNNING NOARP  MTU:16  Metric:1
>           RX packets:12711052 errors:17 dropped:0 overruns:0 frame:17
>           TX packets:15023217 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:10
>           RX bytes:101688416 (96.9 MiB)  TX bytes:109206486 (104.1 MiB)
>           Interrupt:19
> 
> 
>           Rx errors:17  == wc over "data overrun interrupt": 17 == RX frame
> : 17.
>  
> 	2) How the data overrun can happen, if the can bus load  is ~25%?

If a bunch of messages is sent in a short time. The 25% is an average
load, I assume.

> 	 
> 	$ sudo cat /proc/net/can/stats
> 	[sudo] password for ran:
>  
>   1421819 transmitted frames (TXF)
>   2606493 received frames (RXF)
>    473873 matched frames (RXMF)
>  
>        18 % total match ratio (RXMR)
>      2400 frames/s total tx rate (TXR)
>      4400 frames/s total rx rate (RXR)
>  
>        18 % current match ratio (CRXMR)
>      2400 frames/s current tx rate (CTXR)
>      4400 frames/s current rx rate (CRXR)
>  
>        18 % max match ratio (MRXMR)
>      2427 frames/s max tx rate (MTXR)
>      4427 frames/s max rx rate (MRXR)
>  
>         4 current receive list entries (CRCV)
>         9 maximum receive list entries (MRCV)
>  
> 	        2 statistic resets (STR)

  # ip -d -s link show can0

will list the CAN statistics.

Wolfgang.

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Richard Andrysek]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 	2) How the data overrun can happen, if the can bus load  is ~25%?
> 
> If a bunch of messages is sent in a short time. The 25% is an average
> load, I assume.
> 
> > 	 
> > 	$ sudo cat /proc/net/can/stats
> > 	[sudo] password for ran:
> >  
> >   1421819 transmitted frames (TXF)
> >   2606493 received frames (RXF)
> >    473873 matched frames (RXMF)
> >  
> >        18 % total match ratio (RXMR)
> >      2400 frames/s total tx rate (TXR)
> >      4400 frames/s total rx rate (RXR)
> >  
> >        18 % current match ratio (CRXMR)
> >      2400 frames/s current tx rate (CTXR)
> >      4400 frames/s current rx rate (CRXR)
> >  
> >        18 % max match ratio (MRXMR)
> >      2427 frames/s max tx rate (MTXR)
> >      4427 frames/s max rx rate (MRXR)
> >  
> >         4 current receive list entries (CRCV)
> >         9 maximum receive list entries (MRCV)
> >  
> > 	        2 statistic resets (STR)
> 
>   # ip -d -s link show can0
> 
> will list the CAN statistics.
> 
> Wolfgang.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Hi Wolfgang,

thank you for help. I've recognized that these two lines comes from a
network world:

RX packets:12462166 errors:121380 dropped:0 overruns:0 frame:121380
TX packets:11604619 errors:678 dropped:0 overruns:0 carrier:0

Then each event "arbitration lost" generates an IRQ.
Is it OK to disable it, just now for tests. With that I want to reduce an
amount of IRQs. Is it possible to disable it without a new compilation of a
kernel?

We are running quite fast, in few ms. So we test your driver quite well:)

by

Richard

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Wolfgang Grandegger]

On 12/05/2013 08:37 PM, Richard Andrysek wrote:
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
> 
>> 	2) How the data overrun can happen, if the can bus load  is ~25%?
>>
>> If a bunch of messages is sent in a short time. The 25% is an average
>> load, I assume.
>>
>>> 	 
>>> 	$ sudo cat /proc/net/can/stats
>>> 	[sudo] password for ran:
>>>  
>>>   1421819 transmitted frames (TXF)
>>>   2606493 received frames (RXF)
>>>    473873 matched frames (RXMF)
>>>  
>>>        18 % total match ratio (RXMR)
>>>      2400 frames/s total tx rate (TXR)
>>>      4400 frames/s total rx rate (RXR)
>>>  
>>>        18 % current match ratio (CRXMR)
>>>      2400 frames/s current tx rate (CTXR)
>>>      4400 frames/s current rx rate (CRXR)
>>>  
>>>        18 % max match ratio (MRXMR)
>>>      2427 frames/s max tx rate (MTXR)
>>>      4427 frames/s max rx rate (MRXR)
>>>  
>>>         4 current receive list entries (CRCV)
>>>         9 maximum receive list entries (MRCV)
>>>  
>>> 	        2 statistic resets (STR)
>>
>>   # ip -d -s link show can0
>>
>> will list the CAN statistics.
>>
>> Wolfgang.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo <at> vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> 
> Hi Wolfgang,
> 
> thank you for help. I've recognized that these two lines comes from a
> network world:
> 
> RX packets:12462166 errors:121380 dropped:0 overruns:0 frame:121380
> TX packets:11604619 errors:678 dropped:0 overruns:0 carrier:0

What do you mean? CAN belongs to the network world. What does the
following command report:

 # ip -d -s link show can0

> Then each event "arbitration lost" generates an IRQ.

From a CAN device?

> Is it OK to disable it, just now for tests. With that I want to reduce an
> amount of IRQs. Is it possible to disable it without a new compilation of a
> kernel?

"ifconfig <device> down" should do the job. You may get in trouble if
it's your ethernet device, though.

> We are running quite fast, in few ms. So we test your driver quite well:)

With "-rt" or with vanilla Linux?

Wolfgang.

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Richard Andrysek]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> What do you mean? CAN belongs to the network world. What does the
> following command report:
> 
>  # ip -d -s link show can0

I have now already modified driver, but before it shown the overrun &
arbitration lost errors. Perfect command:)
> 
> > Then each event "arbitration lost" generates an IRQ.
> 
> From a CAN device?

Yes you can see it in a code of driver/net/can/sja1000/sja1000.c,
function:

static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
{ 
   ...
   if (isrc & IRQ_ALI) {
      /* arbitration lost interrupt */
      netdev_dbg(dev, "arbitration lost interrupt\n");
      alc = priv->read_reg(priv, SJA1000_ALC);
      priv->can.can_stats.arbitration_lost++;
      stats->tx_errors++;
      cf->can_id |= CAN_ERR_LOSTARB;
      cf->data[0] = alc & 0x1f;
   }
   ...
}


I've just studied sja1000.c I do not know yet, if it is healthy.
May be something in upper levels will be not anymore well handled. 
I've changed a code in a function (see IRQ_ALI part):

static void set_normal_mode(struct net_device *dev)
{
  ...
               if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
                    priv->write_reg(priv, SJA1000_IER, IRQ_ALL & ~IRQ_ALI);
               else
                    priv->write_reg(priv, SJA1000_IER,
                                    IRQ_ALL & ~(IRQ_BEI | IRQ_ALI));
  ...
}

But I prefare to make some kind of "ioctl" support for that. There are
applications, where it shall not happend. Concurrently I've played with
taskset and priorities. 

$ ps -e -o pid,rtprio,comm | grep "irq/19"
   56     89 irq/19-uhci_hcd
 1877     88 irq/19-can0
 1922     88 irq/19-can1

I do not still understand, what does it mean "irq/19-uhci_hcd". Which is
also active, even I do not have any USB device. Mouse and keyboard are
connected through PS2.

$ lsusb
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 004 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub

Now I have NO FAILURE. To make 100% confirmation, I have to improve  tests.
Until now:

$ ip -d -s link show can0
3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
DEFAULT qlen 10
    link/can
    can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
    bitrate 1000000 sample-point 0.750
    tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1
    sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
    clock 8000000
    re-started bus-errors arbit-lost error-warn error-pass bus-off
    0          0          0          0          0          0
    RX: bytes  packets  errors  dropped overrun mcast
    1874449440 234306180 0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    771292494  107205646 0       0       0       0

$ ip -d -s link show can1
4: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
DEFAULT qlen 10
    link/can
    can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
    bitrate 1000000 sample-point 0.750
    tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1
    sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
    clock 8000000
    re-started bus-errors arbit-lost error-warn error-pass bus-off
    0          0          0          0          0          0
    RX: bytes  packets  errors  dropped overrun mcast
    384541802  59160277 0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    77591108   9843156  0       0       0       0


Please keep in mind arbit-lost is now zero forever.

> 
> > Is it OK to disable it, just now for tests. With that I want to reduce an
> > amount of IRQs. Is it possible to disable it without a new compilation of a
> > kernel?
> 
> "ifconfig <device> down" should do the job. You may get in trouble if
> it's your ethernet device, though.
> 

I do not want to disable a can, just an interrupt "arbitration lost" for TX.

> > We are running quite fast, in few ms. So we test your driver quite well:)
> 
> With "-rt" or with vanilla Linux?

3.12.1-custom6-rt4

> Wolfgang.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

What do you think about support for enabling/disabling selected IRQs? Is it
already done?

By

Richard

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Wolfgang Grandegger]

On Fri, 6 Dec 2013 09:27:27 +0000 (UTC), Richard Andrysek

<richard.andrysek@rg-mechatronics.com> wrote:

> Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 

>> 

>> What do you mean? CAN belongs to the network world. What does the

>> following command report:

>> 

>>  # ip -d -s link show can0

> 

> I have now already modified driver, but before it shown the overrun &

> arbitration lost errors. Perfect command:)

>> 

>> > Then each event "arbitration lost" generates an IRQ.

>> 

>> From a CAN device?

> 

> Yes you can see it in a code of driver/net/can/sja1000/sja1000.c,

> function:

> 

> static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t

> status)

> { 

>    ...

>    if (isrc & IRQ_ALI) {

>       /* arbitration lost interrupt */

>       netdev_dbg(dev, "arbitration lost interrupt\n");

>       alc = priv->read_reg(priv, SJA1000_ALC);

>       priv->can.can_stats.arbitration_lost++;

>       stats->tx_errors++;

>       cf->can_id |= CAN_ERR_LOSTARB;

>       cf->data[0] = alc & 0x1f;

>    }

>    ...

> }

> 

> 

> I've just studied sja1000.c I do not know yet, if it is healthy.



This is normally due to hardware/electrical problems. It has nothing to do

with software. Well, is there *any* relation to the patch mentioned in the

subject? I mean, does the problem *not* show up without that patch.



> May be something in upper levels will be not anymore well handled. 

> I've changed a code in a function (see IRQ_ALI part):

> 

> static void set_normal_mode(struct net_device *dev)

> {

>   ...

>                if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)

>                     priv->write_reg(priv, SJA1000_IER, IRQ_ALL &

~IRQ_ALI);

>                else

>                     priv->write_reg(priv, SJA1000_IER,

>                                     IRQ_ALL & ~(IRQ_BEI | IRQ_ALI));

>   ...

> }

> 

> But I prefare to make some kind of "ioctl" support for that. There are

> applications, where it shall not happend. Concurrently I've played with

> taskset and priorities. 

> 

> $ ps -e -o pid,rtprio,comm | grep "irq/19"

>    56     89 irq/19-uhci_hcd

>  1877     88 irq/19-can0

>  1922     88 irq/19-can1

> 

> I do not still understand, what does it mean "irq/19-uhci_hcd". Which is

> also active, even I do not have any USB device. Mouse and keyboard are

> connected through PS2.



Seem that the interrupts are shared. "$ cat /pric/interrupts" provide

further

information.



Wolfgang.

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Marc Kleine-Budde]

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

On 12/06/2013 10:27 AM, Richard Andrysek wrote:
>> What do you mean? CAN belongs to the network world. What does the
>> following command report:
>>
>>  # ip -d -s link show can0
> 
> I have now already modified driver, but before it shown the overrun &
> arbitration lost errors. Perfect command:)
>>
>>> Then each event "arbitration lost" generates an IRQ.
>>
>> From a CAN device?
> 
> Yes you can see it in a code of driver/net/can/sja1000/sja1000.c,
> function:
> 
> static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> { 
>    ...
>    if (isrc & IRQ_ALI) {
>       /* arbitration lost interrupt */
>       netdev_dbg(dev, "arbitration lost interrupt\n");
>       alc = priv->read_reg(priv, SJA1000_ALC);
>       priv->can.can_stats.arbitration_lost++;
>       stats->tx_errors++;
>       cf->can_id |= CAN_ERR_LOSTARB;
>       cf->data[0] = alc & 0x1f;
>    }
>    ...
> }
> 
> 
> I've just studied sja1000.c I do not know yet, if it is healthy.
> May be something in upper levels will be not anymore well handled. 
> I've changed a code in a function (see IRQ_ALI part):

You can send a diff, to illustrate your changes.

> static void set_normal_mode(struct net_device *dev)
> {
>   ...
>                if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
>                     priv->write_reg(priv, SJA1000_IER, IRQ_ALL & ~IRQ_ALI);
>                else
>                     priv->write_reg(priv, SJA1000_IER,
>                                     IRQ_ALL & ~(IRQ_BEI | IRQ_ALI));
>   ...
> }
> 
> But I prefare to make some kind of "ioctl" support for that. There are
> applications, where it shall not happend. Concurrently I've played with
> taskset and priorities.

ioctl() is not a option here, but there are two options:
1) We can put the arbitration lost error to the bus errors,
   but I think that's wrong.
2) You can add another ctrlmode to disable arbitration lost error
   reporting.

> $ ps -e -o pid,rtprio,comm | grep "irq/19"
>    56     89 irq/19-uhci_hcd
>  1877     88 irq/19-can0
>  1922     88 irq/19-can1
> 
> I do not still understand, what does it mean "irq/19-uhci_hcd". Which is
> also active, even I do not have any USB device. Mouse and keyboard are
> connected through PS2.

The USB interrupt is needed, even if you don't have a device plugged
into an USB port. You have to disable you USB Device in your BIOS,
disable the driver or something like that.

> $ lsusb
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 004 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> 
> Now I have NO FAILURE. To make 100% confirmation, I have to improve  tests.
> Until now:
> 
> $ ip -d -s link show can0
> 3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
> DEFAULT qlen 10
>     link/can
>     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>     bitrate 1000000 sample-point 0.750
>     tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1
>     sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 8000000
>     re-started bus-errors arbit-lost error-warn error-pass bus-off
>     0          0          0          0          0          0
>     RX: bytes  packets  errors  dropped overrun mcast
>     1874449440 234306180 0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     771292494  107205646 0       0       0       0
> 
> $ ip -d -s link show can1
> 4: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
> DEFAULT qlen 10
>     link/can
>     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>     bitrate 1000000 sample-point 0.750
>     tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1
>     sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 8000000
>     re-started bus-errors arbit-lost error-warn error-pass bus-off
>     0          0          0          0          0          0
>     RX: bytes  packets  errors  dropped overrun mcast
>     384541802  59160277 0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     77591108   9843156  0       0       0       0
> 
> 
> Please keep in mind arbit-lost is now zero forever.

Because you disabled it, right?

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Richard Andrysek]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
>...
> 
> > I've just studied sja1000.c I do not know yet, if it is healthy.
> 
> This is normally due to hardware/electrical problems. It has nothing to do
> 
> with software. Well, is there *any* relation to the patch mentioned in the
> 
> subject? I mean, does the problem *not* show up without that patch.


Patch solved a problem with unhandled IRQs. The original problem is solved.
CAN arbitration itself is for me a feature, which I as a programmer want to
control and use it as I want. Similar other IRQs.

> 
> > May be something in upper levels will be not anymore well handled. 
> 
> > I've changed a code in a function (see IRQ_ALI part):
> 
> > 
> 
> > static void set_normal_mode(struct net_device *dev)
> 
> > {
> 
> >   ...
> 
> >                if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> 
> >                     priv->write_reg(priv, SJA1000_IER, IRQ_ALL &
> 
> ~IRQ_ALI);
> 
> >                else
> 
> >                     priv->write_reg(priv, SJA1000_IER,
> 
> >                                     IRQ_ALL & ~(IRQ_BEI | IRQ_ALI));
> 
> >   ...
> 
> > }
> 
> > 
> 
> > But I prefare to make some kind of "ioctl" support for that. There are
> 
> > applications, where it shall not happend. Concurrently I've played with
> 
> > taskset and priorities. 
> 
> > 
> 
> > $ ps -e -o pid,rtprio,comm | grep "irq/19"
> 
> >    56     89 irq/19-uhci_hcd
> 
> >  1877     88 irq/19-can0
> 
> >  1922     88 irq/19-can1
> 
> > 
> 
> > I do not still understand, what does it mean "irq/19-uhci_hcd". Which is
> 
> > also active, even I do not have any USB device. Mouse and keyboard are
> 
> > connected through PS2.
> 
> Seem that the interrupts are shared. "$ cat /pric/interrupts" provide
> 
> further
> 
> information.

Yes, it is so.

$ cat /proc/interrupts
           CPU0       CPU1
  0:        127          0   IO-APIC-edge      timer
  1:       5582          0   IO-APIC-edge      i8042
  6:          3          0   IO-APIC-edge      floppy
  7:          1          0   IO-APIC-edge      parport0
  8:          1          0   IO-APIC-edge      rtc0
  9:          0          0   IO-APIC-fasteoi   acpi
 12:      30627          0   IO-APIC-edge      i8042
 14:      43351          0   IO-APIC-edge      ata_piix
 15:     286242          0   IO-APIC-edge      ata_piix
 16:          0          0   IO-APIC-fasteoi   uhci_hcd:usb2, uhci_hcd:usb5
 17:        370          0   IO-APIC-fasteoi   i801_smbus, snd_intel8x0
 18:     102206          0   IO-APIC-fasteoi   uhci_hcd:usb4, eth0
 19:  397673976          0   IO-APIC-fasteoi   uhci_hcd:usb3, can0, can1
 23:          0          0   IO-APIC-fasteoi   ehci_hcd:usb1
NMI:          1          1   Non-maskable interrupts
LOC:   15370305   11437013   Local timer interrupts
SPU:          0          0   Spurious interrupts
PMI:          0          0   Performance monitoring interrupts
IWI:          0          0   IRQ work interrupts
RTR:          0          0   APIC ICR read retries
RES:     283821   18429091   Rescheduling interrupts
CAL:       9623       3662   Function call interrupts
TLB:       7408      66943   TLB shootdowns
THR:          0          0   Threshold APIC interrupts
MCE:          0          0   Machine check exceptions
MCP:          0          0   Machine check polls
ERR:          0
MIS:          0

I prefer to switch off it from a linux side. I will look for that.


> 
> Wolfgang.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Richard Andrysek]

Marc Kleine-Budde <mkl <at> pengutronix.de> writes:

> 
> You can send a diff, to illustrate your changes.

$ diff /cygdrive/X/bug1/sja1000.c /cygdrive/X/modified_sja1000.c

151a152
>                         /* RAN 5.12.2013: Disable arbitration IRQ */
153c154
<                               priv->write_reg(priv, SJA1000_IER, IRQ_ALL);
---
>                               priv->write_reg(priv, SJA1000_IER, IRQ_ALL &
~IRQ_ALI);
156c157
<                                               IRQ_ALL & ~IRQ_BEI);
---
>                                               IRQ_ALL & ~(IRQ_BEI | IRQ_ALI));


> > But I prefare to make some kind of "ioctl" support for that. There are
> > applications, where it shall not happend. Concurrently I've played with
> > taskset and priorities.
> 
> ioctl() is not a option here, but there are two options:
> 1) We can put the arbitration lost error to the bus errors,
>    but I think that's wrong.
> 2) You can add another ctrlmode to disable arbitration lost error
>    reporting.

> ...
> > 4: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode
> > DEFAULT qlen 10
> >     link/can
> >     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
> >     bitrate 1000000 sample-point 0.750
> >     tq 250 prop-seg 1 phase-seg1 1 phase-seg2 1 sjw 1
> >     sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
> >     clock 8000000
> >     re-started bus-errors arbit-lost error-warn error-pass bus-off
> >     0          0          0          0          0          0
> >     RX: bytes  packets  errors  dropped overrun mcast
> >     384541802  59160277 0       0       0       0
> >     TX: bytes  packets  errors  dropped carrier collsns
> >     77591108   9843156  0       0       0       0
> > 
> > 
> > Please keep in mind arbit-lost is now zero forever.
> 
> Because you disabled it, right?

Yes, that's right.

> 
> Marc

As Wolgang mentioned it has nothing to do with the patch v6.
Enabling/diabling IRQs is a new feature. So I don't know, if here it is a
right place to continue about that.

By

Richard

arbitration lost error reporting (was: Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value)

[Marc Kleine-Budde]

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

On 12/06/2013 11:57 AM, Richard Andrysek wrote:
>> You can send a diff, to illustrate your changes.
> 
> $ diff /cygdrive/X/bug1/sja1000.c /cygdrive/X/modified_sja1000.c

"diff -u" is preferred, but no need to resend it.

>>> But I prefare to make some kind of "ioctl" support for that. There are
>>> applications, where it shall not happend. Concurrently I've played with
>>> taskset and priorities.
>>
>> ioctl() is not a option here, but there are two options:
>> 1) We can put the arbitration lost error to the bus errors,
>>    but I think that's wrong.
>> 2) You can add another ctrlmode to disable arbitration lost error
>>    reporting.

> As Wolgang mentioned it has nothing to do with the patch v6.
> Enabling/diabling IRQs is a new feature. So I don't know, if here it is a
> right place to continue about that.

Yes, why not? :)
As I outlined above you have two options. I prefer option 2 and it has a
big change of going mainline. The implementation is similar to
CAN_CTRLMODE_BERR_REPORTING, just look for it in the kernel source tree
and add a new define for arbitration lost error reporting.

Do you have a preferred name for the define?
- CAN_CTRLMODE_AERR_REPORTING
- CAN_CTRLMODE_ARBITRATIONERR_REPORTING to

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: arbitration lost error reporting

[Oliver Hartkopp]

On 06.12.2013 12:45, Marc Kleine-Budde wrote:
> On 12/06/2013 11:57 AM, Richard Andrysek wrote:
>>> You can send a diff, to illustrate your changes.
>>
>> $ diff /cygdrive/X/bug1/sja1000.c /cygdrive/X/modified_sja1000.c
> 
> "diff -u" is preferred, but no need to resend it.
> 
>>>> But I prefare to make some kind of "ioctl" support for that. There are
>>>> applications, where it shall not happend. Concurrently I've played with
>>>> taskset and priorities.
>>>
>>> ioctl() is not a option here, but there are two options:
>>> 1) We can put the arbitration lost error to the bus errors,
>>>    but I think that's wrong.
>>> 2) You can add another ctrlmode to disable arbitration lost error
>>>    reporting.
> 
>> As Wolgang mentioned it has nothing to do with the patch v6.
>> Enabling/diabling IRQs is a new feature. So I don't know, if here it is a
>> right place to continue about that.
> 
> Yes, why not? :)
> As I outlined above you have two options. I prefer option 2 and it has a
> big change of going mainline. The implementation is similar to
> CAN_CTRLMODE_BERR_REPORTING, just look for it in the kernel source tree
> and add a new define for arbitration lost error reporting.
> 
> Do you have a preferred name for the define?
> - CAN_CTRLMODE_AERR_REPORTING
> - CAN_CTRLMODE_ARBITRATIONERR_REPORTING to

Loosing the arbitration is not an error. It just can happen from time to time.

If you thing about a define, what about:

CAN_CTRLMODE_ARB_LOST_REPORTING
CAN_CTRLMODE_ARBITRATION_LOST_REPORTING

Regards,
Oliver

Re: arbitration lost error reporting

[Marc Kleine-Budde]

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

On 12/06/2013 01:02 PM, Oliver Hartkopp wrote:
>> Do you have a preferred name for the define?
>> - CAN_CTRLMODE_AERR_REPORTING
>> - CAN_CTRLMODE_ARBITRATIONERR_REPORTING to
> 
> Loosing the arbitration is not an error. It just can happen from time to time.

Details :)

> If you thing about a define, what about:
> 
> [ ] CAN_CTRLMODE_ARB_LOST_REPORTING
> [x] CAN_CTRLMODE_ARBITRATION_LOST_REPORTING

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: arbitration lost error reporting

[Richard Andrysek]

Marc Kleine-Budde <mkl <at> pengutronix.de> writes:


> > If you thing about a define, what about:
> > 
> > [ ] CAN_CTRLMODE_ARB_LOST_REPORTING
> > [x] CAN_CTRLMODE_ARBITRATION_LOST_REPORTING
> 
> Marc
> 

To be more compatible with compilers and standards the option 1 is prefferd.
This one si only 32 characters long.

Re: arbitration lost error reporting

[Marc Kleine-Budde]

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

On 12/06/2013 02:21 PM, Richard Andrysek wrote:
> Marc Kleine-Budde <mkl <at> pengutronix.de> writes:
> 
> 
>>> If you thing about a define, what about:
>>>
>>> [ ] CAN_CTRLMODE_ARB_LOST_REPORTING
>>> [x] CAN_CTRLMODE_ARBITRATION_LOST_REPORTING
>>
>> Marc
>>
> 
> To be more compatible with compilers and standards the option 1 is prefferd.
> This one si only 32 characters long.

Okay, option 1 is verbose enough.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: arbitration lost error reporting

[Wolfgang Grandegger]

On 12/06/2013 01:02 PM, Oliver Hartkopp wrote:
> 
> 
> On 06.12.2013 12:45, Marc Kleine-Budde wrote:
>> On 12/06/2013 11:57 AM, Richard Andrysek wrote:
>>>> You can send a diff, to illustrate your changes.
>>>
>>> $ diff /cygdrive/X/bug1/sja1000.c /cygdrive/X/modified_sja1000.c
>>
>> "diff -u" is preferred, but no need to resend it.
>>
>>>>> But I prefare to make some kind of "ioctl" support for that. There are
>>>>> applications, where it shall not happend. Concurrently I've played with
>>>>> taskset and priorities.
>>>>
>>>> ioctl() is not a option here, but there are two options:
>>>> 1) We can put the arbitration lost error to the bus errors,
>>>>    but I think that's wrong.
>>>> 2) You can add another ctrlmode to disable arbitration lost error
>>>>    reporting.
>>
>>> As Wolgang mentioned it has nothing to do with the patch v6.
>>> Enabling/diabling IRQs is a new feature. So I don't know, if here it is a
>>> right place to continue about that.
>>
>> Yes, why not? :)
>> As I outlined above you have two options. I prefer option 2 and it has a
>> big change of going mainline. The implementation is similar to
>> CAN_CTRLMODE_BERR_REPORTING, just look for it in the kernel source tree
>> and add a new define for arbitration lost error reporting.
>>
>> Do you have a preferred name for the define?
>> - CAN_CTRLMODE_AERR_REPORTING
>> - CAN_CTRLMODE_ARBITRATIONERR_REPORTING to
> 
> Loosing the arbitration is not an error. It just can happen from time to time.

Well, if it does not happen often, why do we want to suppress reporting
this error? I still do not see what it's good for.

Wolfgang.

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Wolfgang Grandegger]

On 12/06/2013 11:32 AM, Richard Andrysek wrote:
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
> 
>>
>> ...
>>
>>> I've just studied sja1000.c I do not know yet, if it is healthy.
>>
>> This is normally due to hardware/electrical problems. It has nothing to do
>>
>> with software. Well, is there *any* relation to the patch mentioned in the
>>
>> subject? I mean, does the problem *not* show up without that patch.
> 
> 
> Patch solved a problem with unhandled IRQs. The original problem is solved.

What did it fix? Could you elaborate on that issue. You actually never
reported a problem.

Wolfgang.

Re: arbitration lost error reporting

[Oliver Hartkopp]

On 06.12.2013 18:59, Wolfgang Grandegger wrote:
> On 12/06/2013 01:02 PM, Oliver Hartkopp wrote:
>> On 06.12.2013 12:45, Marc Kleine-Budde wrote:

>>> As I outlined above you have two options. I prefer option 2 and it has a
>>> big change of going mainline. The implementation is similar to
>>> CAN_CTRLMODE_BERR_REPORTING, just look for it in the kernel source tree
>>> and add a new define for arbitration lost error reporting.
>>>
>>> Do you have a preferred name for the define?
>>> - CAN_CTRLMODE_AERR_REPORTING
>>> - CAN_CTRLMODE_ARBITRATIONERR_REPORTING to
>>
>> Loosing the arbitration is not an error. It just can happen from time to time.
> 
> Well, if it does not happen often, why do we want to suppress reporting
> this error? I still do not see what it's good for.

Sometimes it's good to let it settle.

Indeed the CAN_CTRLMODE_BERR_REPORTING was introduced to reduce the irq storms
that can occur when bus errors happen on the bus.

All other states are promoted by the driver when this information can be
retrieved from the hardware.

Btw. it seems reasonable to ask whether it makes sense to create error message
skbs when there's no consumer of the information.
E.g. the networking timestamps are only enabled when (at least) one
application requires timestamps on the host.

Therefore I would suggest to create the error message skb only when someone is
requesting this information (== an error message filter is enabled).

I'll send a RFC patch for that ...

Oliver

Re: arbitration lost error reporting

[Richard Andrysek]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

...
> >> big change of going mainline. The implementation is similar to
> >> CAN_CTRLMODE_BERR_REPORTING, just look for it in the kernel source tree
> >> and add a new define for arbitration lost error reporting.
> >>
> >> Do you have a preferred name for the define?
> >> - CAN_CTRLMODE_AERR_REPORTING
> >> - CAN_CTRLMODE_ARBITRATIONERR_REPORTING to
> > 
> > Loosing the arbitration is not an error. It just can happen from time to
time.
> 
> Well, if it does not happen often, why do we want to suppress reporting
> this error? I still do not see what it's good for.
> 
> Wolfgang.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

How do you define not often? Is it not often one time per ~130us? For X
channels you should divide it through X. Please assume this case stochastic
=> not always. 
Next if the information is for an application irrelevant, why I need to
generate an IRQ and use CPU resources?

How to fight with such case:

1) Use time triggered protocol - not always possible 
2) Slow done communication - not always possible
3) Buy faster machine - not always possible
4) Combination of 1..4 - ...

or

5) Let a freedom to a programmer, for the price that he shall to know it.

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Richard Andrysek]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> On 12/06/2013 11:32 AM, Richard Andrysek wrote:
> > Wolfgang Grandegger <wg <at> grandegger.com> writes:
> > 
> >>
> >> ...
> >>
> >>> I've just studied sja1000.c I do not know yet, if it is healthy.
> >>
> >> This is normally due to hardware/electrical problems. It has nothing to do
> >>
> >> with software. Well, is there *any* relation to the patch mentioned in the
> >>
> >> subject? I mean, does the problem *not* show up without that patch.
> > 
> > 
> > Patch solved a problem with unhandled IRQs. The original problem is solved.
> 
> What did it fix? Could you elaborate on that issue. You actually never
> reported a problem.
> 
> Wolfgang.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

I've reported a problem by peak-systems and they recommend me this patch.
Without this patch with gotos, it leaves unhandled IRQs, e.g.:

Dec  1 04:59:33 xxx kernel: [141189.277965] irq 19: nobody cared (try
booting with the "irqpoll" option)
...
Dec  1 04:59:33 xxx kernel: [141189.278131] Disabling IRQ #19


With this patch the sja1000 IRQs are not lost. 

Re: arbitration lost error reporting

[Marc Kleine-Budde]

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

On 12/09/2013 10:01 AM, Richard Andrysek wrote:
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
> 
> ...
>>>> big change of going mainline. The implementation is similar to
>>>> CAN_CTRLMODE_BERR_REPORTING, just look for it in the kernel source tree
>>>> and add a new define for arbitration lost error reporting.
>>>>
>>>> Do you have a preferred name for the define?
>>>> - CAN_CTRLMODE_AERR_REPORTING
>>>> - CAN_CTRLMODE_ARBITRATIONERR_REPORTING to
>>>
>>> Loosing the arbitration is not an error. It just can happen from time to
> time.
>>
>> Well, if it does not happen often, why do we want to suppress reporting
>> this error? I still do not see what it's good for.
>>
>> Wolfgang.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo <at> vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> 
> How do you define not often? Is it not often one time per ~130us? For X
> channels you should divide it through X. Please assume this case stochastic
> => not always. 
> Next if the information is for an application irrelevant, why I need to
> generate an IRQ and use CPU resources?
> 
> How to fight with such case:
> 
> 1) Use time triggered protocol - not always possible 
> 2) Slow done communication - not always possible
> 3) Buy faster machine - not always possible
> 4) Combination of 1..4 - ...
> 
> or
> 
> 5) Let a freedom to a programmer, for the price that he shall to know it.

+1

Feel free to implement it, as outlined in the previous mails.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Austin Schuh]

Hi Marc,

I have applied the patch.  I ran a test with 75% bus load @500000
bits/sec (15% sending from the SJA1000 and 60% from another device on
the bus, running candump can1 to grab the data).  It failed within
minutes.  I'll add the code back in to disable tracing when the IRQ
gets disabled.

Austin

On Mon, Nov 25, 2013 at 12:54 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 11/25/2013 12:03 AM, Marc Kleine-Budde wrote:
>> 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. Further the irq processing counter 'n' is moved to the end of
>> the while statement to return correct IRQ_[NONE|HANDLED] values at error
>> conditions.
>
> Austin, can you test if this patch solves your problem.
>
> Marc
>
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Marc Kleine-Budde]

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

On 12/09/2013 08:48 PM, Austin Schuh wrote:
> Hi Marc,
> 
> I have applied the patch.  I ran a test with 75% bus load @500000
> bits/sec (15% sending from the SJA1000 and 60% from another device on
> the bus, running candump can1 to grab the data).  It failed within
> minutes.  I'll add the code back in to disable tracing when the IRQ
> gets disabled.

Does the patch make any difference at all?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH v6] can: sja1000: fix {pre,post}_irq() handling and IRQ handler return value

[Austin Schuh]

If anything, it increased the frequency of failure (on my RT kernel).
It took 14 minutes to fail the first time, and a little under 2 hours
the second time.

That doesn't mean that it doesn't fix another problem, just that it
doesn't fix the problem that I am having.  I don't see any obvious new
problems caused by the patch.

Austin

On Mon, Dec 9, 2013 at 1:07 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 12/09/2013 08:48 PM, Austin Schuh wrote:
>> Hi Marc,
>>
>> I have applied the patch.  I ran a test with 75% bus load @500000
>> bits/sec (15% sending from the SJA1000 and 60% from another device on
>> the bus, running candump can1 to grab the data).  It failed within
>> minutes.  I'll add the code back in to disable tracing when the IRQ
>> gets disabled.
>
> Does the patch make any difference at all?
>
> Marc
>
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>