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.
+ 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);
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
