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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
