[Xenomai-core] Re: RT-CAN tx_sem

[Jan Kiszka <jan.kiszka@domain.hid>]

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

Wolfgang Grandegger wrote:
> 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().
> 

I thought about this issue again and found the reason for my vague bad
feeling: re-init is not atomic, thus racy. But also the test+sem_init
pattern I suggested is not save.

I guess we need to enhance rtdm_XXX_init in this regard to make the
RT-CAN use case an officially allowed one. /me is planning to spend more
thoughts on this the next days.

Jan


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