[Xenomai-core] Re: RT-CAN tx_sem

[Wolfgang Grandegger <wg@domain.hid>]

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

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Jan Kiszka wrote:
>>> Wolfgang Grandegger wrote:
>>>> Hi Jan,
>>>>
>>>> Jan Kiszka wrote:
>>>>> Hi Wolfgang,
>>>>>
>>>>> fiddling with the CAN utils, I noticed the behaviour that rtcansend on
>>>>> freshly registered but stopped devices simply blocks. And when you
>>>>> meanwhile call rtcanconfig up on that device, rtcansend will
>>>>> continue to
>>>>> block.
>>>> I see the expected behavior on stopped devices:
>>>>
>>>>   bash-2.05b# rtcansend rtcan0
>>>>   rt_dev_send: Network is down
>>>>
>>>> This is, because the tx_sem is marked "destroyed" already when the
>>>> device gets initialized. I wonder why your app blocks.
>>> Unlikely, because the sem is _not_ marked destroyed on startup:
>>>
>>> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/drivers/can/rtcan_dev.c#181
>>>
>> You missed
>>
>>     /* Set dummy state for following call */
>>     dev->state = CAN_STATE_ACTIVE;
>>     /* Enter reset mode */
>>     rtcan_sja_mode_stop(dev, NULL);
>>
>> in the init part of rtcan_sja1000.c and rtcan_mscan.c, ...
>>
>>> In case it makes a subtle difference: I tested with xeno_can_virt, but I
>>> think I saw the same effect on real SJA1000 hw as well.
>> ... which is missing in rtcan_virt.c. I already thought moving it to the
>> common part. The attached patch fixes that.
> 
> I'm still not happy with this pattern, at least it's hidden from the
> drivers now. But do not rtcan_sja1000.c and rtcan_mscan.c require some
> changes as well to avoid double destruction when this happens in generic
> code from now on?

Well, what needs a better implementaion is the code fragment shown above 
but I don't have a quick solution at hand. Therefore I tend to the 
attached patch.

>>>>> The reason is that during startup of CAN devices the tx-semaphore is
>>>>> re-initialised while it is already set up during device registration.
>>>>> Re-init on an already initialised rtdm_sem is, say, undefined.
>>>> That makes definitely trouble. A CAN_MODE_START should only start the
>>>> device if it's not active. The attached patch fixes this. Another
>>>> possibility would be to force a CAN_MODE_STOP in case the device is
>>>> already active (=restart).
>>>>
>>>>> So rtdm_sem should better only be initialised/destroyed by the drivers.
>>>>> Trying to send on a shut down CAN device could be catched differently,
>>>>> e.g. via the device state. This likely needs more thoughts, take it
>>>>> as a
>>>>> note that $something should be done.
>>>> With the fix above I do not see further problems with the existing
>>>> implementation using the "destroyed" state to mark the device
>>>> unavailable.
>>> The problem of double init remains. I don't think that the sem state
>>> should be used for encoding the CAN device state towards potential
>>> senders.
>> As I see it, Sebastian's trick saves code and synchronization.
> 
> Well, we are discussing a single test here. Or doesn't dev->state hold
> the require information as well. Moreover, the existing pattern was easy
> to get wrong between core and driver.

We speak about adding:

  if (!CAN_STATE_OPERATING(dev->state)) {
	if (dev->state == CAN_STATE_SLEEPING)
	    return-ECOMM;
	else
	    return-ENETDOWN;
  }

before calling rtdm_sem_timeddown().

> On the other hand, if we manage to clean the code up, I may change my mind.

I think we should avoid the above lines in a frequently used function.

Wolfgang.

[-- Attachment #2: xenomai-rtcan-txsem3.patch --]
[-- Type: text/x-patch, Size: 2063 bytes --]

+ diff -u xenomai/ksrc/drivers/can/rtcan_dev.c.TXSEM xenomai/ksrc/drivers/can/rtcan_dev.c
+ diff -u xenomai/ksrc/drivers/can/rtcan_raw.c.TXSEM xenomai/ksrc/drivers/can/rtcan_raw.c
--- xenomai/ksrc/drivers/can/rtcan_raw.c.TXSEM	2007-02-15 17:11:11.000000000 +0100
+++ xenomai/ksrc/drivers/can/rtcan_raw.c	2007-02-15 13:46:02.000000000 +0100
@@ -964,12 +964,13 @@
 
     /* Controller should be operating */
     if (!CAN_STATE_OPERATING(dev->state)) {
+	if (dev->state == CAN_STATE_SLEEPING) {
+	    ret = -ECOMM;
+	    rtdm_sem_up(&dev->tx_sem);
+	    goto send_out2;
+	}
         ret = -ENETDOWN;
         goto send_out2;
-    } else if (dev->state == CAN_STATE_SLEEPING) {
-        ret = -ECOMM;
-        rtdm_sem_up(&dev->tx_sem);
-        goto send_out2;
     }
 
     dev->tx_count++;
+ diff -u xenomai/ksrc/drivers/can/rtcan_raw_dev.c.TXSEM xenomai/ksrc/drivers/can/rtcan_raw_dev.c
--- xenomai/ksrc/drivers/can/rtcan_raw_dev.c.TXSEM	2007-02-15 11:21:43.000000000 +0100
+++ xenomai/ksrc/drivers/can/rtcan_raw_dev.c	2007-02-15 14:16:16.000000000 +0100
@@ -193,7 +193,8 @@
     switch (request) {
     case SIOCSCANMODE:
 	mode = (can_mode_t *)&ifr->ifr_ifru;
-	if (dev->do_set_mode)
+	if (dev->do_set_mode &&
+	    !(*mode == CAN_MODE_START && CAN_STATE_OPERATING(dev->state)))
 	    ret = dev->do_set_mode(dev, *mode, &lock_ctx);
 	break;
 
+ diff -u xenomai/ksrc/drivers/can/rtcan_virt.c.TXSEM xenomai/ksrc/drivers/can/rtcan_virt.c
--- xenomai/ksrc/drivers/can/rtcan_virt.c.TXSEM	2007-02-15 11:24:14.000000000 +0100
+++ xenomai/ksrc/drivers/can/rtcan_virt.c	2007-02-15 17:13:23.000000000 +0100
@@ -102,8 +102,7 @@
 		break;
 
 	case CAN_MODE_START:
-		if (dev->state == CAN_STATE_STOPPED)
-			rtdm_sem_init(&dev->tx_sem, VIRT_TX_BUFS);
+		rtdm_sem_init(&dev->tx_sem, VIRT_TX_BUFS);
 		dev->state = CAN_STATE_ACTIVE;
 		break;
 
@@ -126,7 +125,7 @@
 	dev->ctrl_name = virt_ctlr_name;
 	dev->board_name = virt_board_name;
 
-	dev->state = CAN_STATE_STOPPED;
+	rtcan_virt_set_mode(dev, CAN_MODE_STOP, NULL);
 
 	strncpy(dev->name, RTCAN_DEV_NAME, IFNAMSIZ);