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?

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

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

Jan

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to