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.