Re: [Xenomai-core] Re: RT-Socket-CAN bus error rate and latencies

[Wolfgang Grandegger <wg@domain.hid>]

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

Wolfgang Grandegger wrote:
> Jan Kiszka wrote:
>> Oliver Hartkopp wrote:
>>> Additionally to the written stuff below (please read that first), i want
>>> to remark:
>>>
>>> - Remember that we are talking about a case that is not a standard
>>> operation mode but a (temporary) error condition that normally leads to
>>> a bus-off state and appears only in development and hardware setup 
>>> phase!
>>> - i would suggest to use some low resolution timestamp (like jiffies)
>>> for this, which is very cheap in CPU usage
>>> - the throttling should be configured as a driver module parameter (e.g.
>>> bei_thr=0 or bei_thr=200 )due to the need of the global use-case. If you
>>> are writing a CAN analysis tool you might want to set bei_thr=0 in other
>>> cases a default of 200ms might be the right thing.
>>
>> We are falling back to #1, i.e. where we are now already. Your
>> suggestion doesn't help us to provide a generic RT-stack for Xenomai.
>>
>>> Regards,
>>> Oliver
>>>
>>>
>>>
>>> Oliver Hartkopp wrote:
>>>> Wolfgang Grandegger wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Wolfgang Grandegger wrote:
>>>>>>> Oliver Hartkopp wrote:
>>>>>>>
>>>>>>>> I would tend to reduce the notifications to the user by creating a
>>>>>>>> timer at the first bus error interrupt. The first BE irq would
>>>>>>>> lead to a CAN_ERR_BUSERROR and after a (configurable) time
>>>>>>>> (e.g.250ms) the next information about bus errors is allowed to be
>>>>>>>> passed to the user. After this time period is over a new
>>>>>>>> CAN_ERR_BUSERROR may be passed to the user containing the count of
>>>>>>>> occurred bus errors somewhere in the data[]-section of the Error
>>>>>>>> Frame. When a normal RX/TX-interrupt indicates a 'working' CAN
>>>>>>>> again, the timer would be terminated.
>>>>>>>>
>>>>>>>> Instead of a fix configurable time we could also think about a
>>>>>>>> dynamic behaviour (e.g. with increasing periods).
>>>>>>>>
>>>>>>>> What do you think about this?
>>>>>>> The question is if one bus-error does provide enough information on
>>>>>>> the cause of the electrical problem or if a sequence is better.
>>>>>>> Furthermore, I personally regard the use of timers as to heavy. But
>>>>>>> the solution is feasible, of course. Any other opinions?
>>>>>>>
>>>>>> I think Oliver's suggestions points in the right direction. But 
>>>>>> instead
>>>>>> of only coding a timer into the stack, I still vote for closing the
>>>>>> loop
>>>>>> over the application:
>>>>>>
>>>>>> After the first error in a potential series, the related error 
>>>>>> frame is
>>>>>> queued, listeners are woken up, and BEI is disabled for now. Once 
>>>>>> some
>>>>>> listener read the error frame *and* decided to call into the stack 
>>>>>> for
>>>>>> further bus errors, BEI is enabled again.
>>>>>>
>>>>>> That way the application decides about the error-related IRQ rate and
>>>>>> can easily throttle it by delaying the next receive call. Moreover,
>>>>>> threads of higher priority will be delayed at worst by one error IRQ.
>>>>>> This mechanism just needs some words in the documentation ("Be 
>>>>>> warned:
>>>>>> error frames may overwhelm you. Throttle your reception!"), but no
>>>>>> further user-visible config options.
>>>>> I understand, BEI interrupts get (re-)enabled in recvmsg() if the
>>>>> socket wants to receive bus errors. There can me multiple readers,
>>>>> but that's not a problem. Just some overhead in this function. This
>>>>> would also simplify the implementation as my previous one with
>>>>> "on-demand" bus error would be obsolete. I start to like this 
>>>>> solution.
>>>> Hm - to reenable the BEI on user interaction would be a nice thing 
>>>> BUT i
>>>> can see several problems:
>>>>
>>>> 1. In socketcan you have receive queues into the userspace with a
>>>> length >1
>>
>> Can you explain to me what the problem behind this is? I don't see it 
>> yet.
>>
>>>> 2. How can we handle multiple subscribers (A reads three error frames
>>>> and reenables therefore the BEI, B reads nothing in this time). Please
>>>> remember: To have multiple applications it a vital idea from socketcan.
>>
>> Same here, I don't see the issue. A and B will both find the first error
>> frame in their queues/ring buffers/whatever. If A has higher priority
>> (or gets an earlier timeslice), it may already re-enable BEI before B
>> was able to run as well. But that's an application-specific scheduling
>> issue and not a problem of the CAN stack (often it is precisely what you
>> want when assigning priorities...).
>>
>>>> 3. The count of occured BEIs gets lost (maybe this is unimportant)
>>
>> Agreed, but I also don't consider this problematic.
>>
>>>> ----
>>>>
>>>> Regarding (2) the solution could be not to reenable the BEI for a 
>>>> device
>>>> until every subscriber has read his error frame. But this collides with
>>>> a raw-socket that's bound to 'any' device (ifindex = 0).
>>
>> That can cause prio-inversion: a low-prio BEI-reader decides about when
>> a high-prio one gets the next message. No-go for RT.
>>
>>>> Regarding (3) we could count the BEIs (which would not reduce the
>>>> interrupt load) or we just stop the BEI after the first occurance which
>>>> might possibly not enough for some people to implement the CAN
>>>> academical correct.
>>>>
>>>> As you may see here a tight coupling of the problems on the CAN bus 
>>>> with
>>>> the application(s!) is very tricky or even impossible in socketcan.
>>>> Regarding other network devices (like ethernet devices) the 
>>>> notification
>>>> about Layer 1/2 problems is unusual. The concept of creating error
>>>> frames was a good compromise for this reason.
>>>>
>>>> As i also would like to avoid to create a timer for "bus error
>>>> throttling", i got a new idea:
>>>>
>>>> - on the first BEI: create an error frame, set a counter to zero and
>>>> save the current timestamp
>>>> - on the next BEI:
>>>>  - increment the counter
>>>>  - check if the time is up for the next error frame (e.g. after 200ms -
>>>> configurable?)
>>>>  - if so: Send the next error frame (including the number of occured
>>>> error frames in this 200ms)
>>>>
>>>> BEI means ONLY to have a BEI (and no other error).
>>>>
>>>> Of course this does NOT reduce the interrupt load but all this
>>>> throttling is performed inside the interrupt context. This should 
>>>> not be
>>>> that problem, or is it? And we do not need a timer ...
>>>>
>>>> Any comments to this idea?
>>>>
>>>> Regards,
>>>> Oliver
>>>>
>>
>> Well, I may oversee some pitfalls of my suggestion, so please help me to
>> understand your concerns.
> 
> There might be a problem with re-enabling BEI interrupts because we need 
>  to read the ECC. OK, I'm going to implement the method as well to check 
> for pitfalls.

Attached is the patch and it works fine. The key function 
rtcan_sja_enable_bus_err() is called from sendmsg():

void rtcan_sja_enable_bus_err(struct rtcan_device *dev)
{
     struct rtcan_sja1000 *chip = (struct rtcan_sja1000 *)dev->priv;

     if (chip->bus_err_on < 2) {
	if (chip->bus_err_on < 1)
	    chip->read_reg(dev, SJA_ECC);
	chip->bus_err_on = 2;
     }
}

And I do also do not see a real problem with multiple readers. I would 
commit this solution. I'm just unsure if we should select it silently or 
if the user should have the choice.

Wolfgang.

[-- Attachment #2: xenomai-rtcan-bus-err-on-send.patch --]
[-- Type: text/x-patch, Size: 7490 bytes --]

Index: ksrc/drivers/can/Kconfig
===================================================================
--- ksrc/drivers/can/Kconfig	(revision 2335)
+++ ksrc/drivers/can/Kconfig	(working copy)
@@ -49,6 +49,19 @@ config XENO_DRIVERS_CAN_MAX_RECEIVERS
 
 	The driver maintains a receive filter list per device for fast access.
 
+config XENO_DRIVERS_CAN_BUS_ERR
+	depends on XENO_DRIVERS_CAN
+	bool
+	default n
+	help
+
+	To avoid unnecessary bus error interrupt flooding, this option enables
+	bus error interrupts when an application is calling a receive function
+	on a socket listening on bus errors. After one bus error has occured,
+	the interrupt will be disabled to allow the application time for error
+	processing. This option is automatically selected for CAN controllers
+	supporting bus error interrupts like the SJA1000.
+
 config XENO_DRIVERS_CAN_VIRT
 	depends on XENO_DRIVERS_CAN
 	tristate "Virtual CAN bus driver"
Index: ksrc/drivers/can/rtcan_dev.h
===================================================================
--- ksrc/drivers/can/rtcan_dev.h	(revision 2335)
+++ ksrc/drivers/can/rtcan_dev.h	(working copy)
@@ -118,6 +118,7 @@ struct rtcan_device {
     int                 (*do_set_bit_time)(struct rtcan_device *dev,
 					   struct can_bittime *bit_time,
 					   rtdm_lockctx_t *lock_ctx);
+    void                (*do_enable_bus_err)(struct rtcan_device *dev);
 
     /* Reception list head. This list contains all filters which have been
      * registered via a bind call. */
Index: ksrc/drivers/can/rtcan_raw_dev.c
===================================================================
--- ksrc/drivers/can/rtcan_raw_dev.c	(revision 2335)
+++ ksrc/drivers/can/rtcan_raw_dev.c	(working copy)
@@ -312,3 +312,33 @@ int rtcan_raw_ioctl_dev(struct rtdm_dev_
 
     return ret;
 }
+
+#ifdef CONFIG_XENO_DRIVERS_CAN_BUS_ERR
+void __rtcan_raw_enable_bus_err(struct rtcan_socket *sock)
+{
+    int i, begin, end;
+    struct rtcan_device *dev;
+    rtdm_lockctx_t lock_ctx;
+    int ifindex = atomic_read(&sock->ifindex);
+
+    if (ifindex) {
+	begin = ifindex;
+	end   = ifindex;
+    } else {
+	begin = 1;
+	end = RTCAN_MAX_DEVICES;
+    }
+
+    for (i = begin; i <= end; i++) {
+	if ((dev = rtcan_dev_get_by_index(i)) == NULL)
+	    continue;
+
+	if (dev->do_enable_bus_err) {
+	    rtdm_lock_get_irqsave(&dev->device_lock, lock_ctx);
+	    dev->do_enable_bus_err(dev);
+	    rtdm_lock_put_irqrestore(&dev->device_lock, lock_ctx);
+	}
+	rtcan_dev_dereference(dev);
+    }
+}
+#endif /* CONFIG_XENO_DRIVERS_CAN_BUS_ERR*/
Index: ksrc/drivers/can/rtcan_raw.c
===================================================================
--- ksrc/drivers/can/rtcan_raw.c	(revision 2335)
+++ ksrc/drivers/can/rtcan_raw.c	(working copy)
@@ -624,6 +624,7 @@ ssize_t rtcan_raw_recvmsg(struct rtdm_de
             return -EINVAL;
     }
 
+    rtcan_raw_enable_bus_err(sock);
 
     /* Set RX timeout */
     timeout = (flags & MSG_DONTWAIT) ? RTDM_TIMEOUT_NONE : sock->rx_timeout;
Index: ksrc/drivers/can/rtcan_raw.h
===================================================================
--- ksrc/drivers/can/rtcan_raw.h	(revision 2335)
+++ ksrc/drivers/can/rtcan_raw.h	(working copy)
@@ -41,6 +41,17 @@ void rtcan_loopback(struct rtcan_device 
 #define rtcan_loopback_pending(dev) (0)
 #endif /* CONFIG_XENO_DRIVERS_CAN_LOOPBACK */
 
+#ifdef CONFIG_XENO_DRIVERS_CAN_BUS_ERR
+void __rtcan_raw_enable_bus_err(struct rtcan_socket *sock);
+static inline void rtcan_raw_enable_bus_err(struct rtcan_socket *sock)
+{
+    if ((sock->err_mask & CAN_ERR_BUSERROR))
+	__rtcan_raw_enable_bus_err(sock);
+}
+#else
+#define rtcan_raw_enable_bus_err(sock)
+#endif
+
 int __init rtcan_raw_proto_register(void);
 void __exit rtcan_raw_proto_unregister(void);
 
Index: ksrc/drivers/can/sja1000/Kconfig
===================================================================
--- ksrc/drivers/can/sja1000/Kconfig	(revision 2335)
+++ ksrc/drivers/can/sja1000/Kconfig	(working copy)
@@ -1,6 +1,7 @@
 config XENO_DRIVERS_CAN_SJA1000
 	depends on XENO_DRIVERS_CAN
 	tristate "Philips SJA1000 CAN controller"
+	select XENO_DRIVERS_CAN_BUS_ERR
 
 config XENO_DRIVERS_CAN_SJA1000_ISA
 	depends on XENO_DRIVERS_CAN_SJA1000
Index: ksrc/drivers/can/sja1000/rtcan_sja1000.c
===================================================================
--- ksrc/drivers/can/sja1000/rtcan_sja1000.c	(revision 2335)
+++ ksrc/drivers/can/sja1000/rtcan_sja1000.c	(working copy)
@@ -293,18 +293,21 @@ static int rtcan_sja_interrupt(rtdm_irq_
             dev->state = dev->state_before_sleep;
 
 	/* Error Interrupt? */
-        if (irq_source & (SJA_IR_EI | SJA_IR_DOI | SJA_IR_EPI | 
+        if (irq_source & (SJA_IR_EI | SJA_IR_DOI | SJA_IR_EPI |
 			  SJA_IR_ALI | SJA_IR_BEI)) {
+
 	    /* Check error condition and fill error frame */
-	    rtcan_sja_err_interrupt(dev, chip, &skb, irq_source);
+	    if (!((irq_source & SJA_IR_BEI) && (chip->bus_err_on-- < 2))) {
+		rtcan_sja_err_interrupt(dev, chip, &skb, irq_source);
 
-	    if (recv_lock_free) {
-		recv_lock_free = 0;
-		rtdm_lock_get(&rtcan_recv_list_lock);
-		rtdm_lock_get(&rtcan_socket_lock);
+		if (recv_lock_free) {
+		    recv_lock_free = 0;
+		    rtdm_lock_get(&rtcan_recv_list_lock);
+		    rtdm_lock_get(&rtcan_socket_lock);
+		}
+		/* Pass error frame out to the sockets */
+		rtcan_rcv(dev, &skb);
 	    }
-	    /* Pass error frame out to the sockets */
-	    rtcan_rcv(dev, &skb);
 	}
 
         /* Transmit Interrupt? */
@@ -625,8 +628,16 @@ int rtcan_sja_set_bit_time(struct rtcan_
     return 0;
 }
 
+void rtcan_sja_enable_bus_err(struct rtcan_device *dev)
+{
+    struct rtcan_sja1000 *chip = (struct rtcan_sja1000 *)dev->priv;
 
-
+    if (chip->bus_err_on < 2) {
+	if (chip->bus_err_on < 1)
+	    chip->read_reg(dev, SJA_ECC);
+	chip->bus_err_on = 2;
+    }
+}
 
 /*
  *  Start a transmission to a SJA1000 device
@@ -745,8 +756,10 @@ int rtcan_sja1000_register(struct rtcan_
     dev->do_set_mode = rtcan_sja_set_mode;
     dev->do_get_state = rtcan_sja_get_state;
     dev->do_set_bit_time = rtcan_sja_set_bit_time;
+    dev->do_enable_bus_err = rtcan_sja_enable_bus_err;
+    chip->bus_err_on = 1;
 
-    ret = rtdm_irq_request(&dev->irq_handle, 
+    ret = rtdm_irq_request(&dev->irq_handle,
 			   chip->irq_num, rtcan_sja_interrupt,
 			   chip->irq_flags, sja_ctrl_name, dev);
     if (ret) {
Index: ksrc/drivers/can/sja1000/Config.in
===================================================================
--- ksrc/drivers/can/sja1000/Config.in	(revision 2335)
+++ ksrc/drivers/can/sja1000/Config.in	(working copy)
@@ -4,6 +4,10 @@
 
 dep_tristate 'Philips SJA1000 CAN controller' CONFIG_XENO_DRIVERS_CAN_SJA1000 $CONFIG_XENO_DRIVERS_CAN
 
+if [ "$CONFIG_XENO_DRIVERS_CAN_SJA1000" != "n" ]; then
+	define_bool CONFIG_XENO_DRIVERS_CAN_BUS_ERR y
+fi
+
 dep_tristate '  Standard ISA controllers' CONFIG_XENO_DRIVERS_CAN_SJA1000_ISA $CONFIG_XENO_DRIVERS_CAN_SJA1000
 if [ "$CONFIG_XENO_DRIVERS_CAN_SJA1000_ISA" != "n" ]; then
 	int '    Maximum number of controllers' CONFIG_XENO_DRIVERS_CAN_SJA1000_ISA_MAX_DEV 4
Index: ksrc/drivers/can/sja1000/rtcan_sja1000.h
===================================================================
--- ksrc/drivers/can/sja1000/rtcan_sja1000.h	(revision 2335)
+++ ksrc/drivers/can/sja1000/rtcan_sja1000.h	(working copy)
@@ -30,6 +30,7 @@ struct rtcan_sja1000 {
     unsigned short irq_flags;
     unsigned char ocr;
     unsigned char cdr;
+    char bus_err_on;
 };
 
 int rtcan_sja_create_proc(struct rtcan_device* dev);