Oliver,
while sitting on the train back home, and looking in your isotp code,
I suspected a possible race condition between waiting for the socket
to become tx.state == ISOTP_IDLE and actually tx.state = ISOTP_SENDING.
I think I solved this by using a semaphore.
The patch is not tested, and may be incomplete. I just send it already
so people can start thinking about the problem.
Regards,
Kurt
Index: net/can/isotp.c
===================================================================
--- net/can/isotp.c (revision 1198)
+++ net/can/isotp.c (working copy)
@@ -61,6 +61,7 @@
#include <linux/socket.h>
#include <linux/if_arp.h>
#include <linux/skbuff.h>
+#include <linux/semaphore.h>
#include <socketcan/can.h>
#include <socketcan/can/core.h>
#include <socketcan/can/isotp.h>
@@ -139,6 +140,16 @@
struct tpcon rx, tx;
struct notifier_block notifier;
wait_queue_head_t wait;
+ /*
+ * a mutex that takes care of grabbing
+ * the socket for transmission
+ * The lock is held for as long as the
+ * transmission is pending. It should be released
+ * when tx.state = ISOTP_IDLE
+ * Since the lock & unlock is done from different
+ * process contexts, a semaphore is used
+ */
+ struct semaphore tx_lock;
};
static inline struct isotp_sock *isotp_sk(const struct sock *sk)
@@ -261,7 +272,7 @@
if ((so->opt.flags & CAN_ISOTP_TX_PADDING) &&
check_pad(so, cf, ae+3, so->opt.txpad_content)) {
so->tx.state = ISOTP_IDLE;
- wake_up_interruptible(&so->wait);
+ up(&so->tx_lock);
return 1;
}
@@ -318,7 +329,7 @@
default:
/* stop this tx job. TODO: error reporting? */
so->tx.state = ISOTP_IDLE;
- wake_up_interruptible(&so->wait);
+ up(&so->tx_lock);
}
return 0;
}
@@ -576,7 +587,7 @@
#endif
/* reset tx state */
so->tx.state = ISOTP_IDLE;
- wake_up_interruptible(&so->wait);
+ up(&so->tx_lock);
break;
case ISOTP_SENDING:
@@ -616,7 +627,7 @@
DBG("we are done\n");
so->tx.state = ISOTP_IDLE;
dev_put(dev);
- wake_up_interruptible(&so->wait);
+ up(&so->tx_lock);
break;
}
@@ -671,29 +682,37 @@
return -EADDRNOTAVAIL;
/* we do not support multiple buffers - for now */
- if (so->tx.state != ISOTP_IDLE) {
- if (msg->msg_flags & MSG_DONTWAIT)
+ if (msg->msg_flags & MSG_DONTWAIT) {
+ if (down_trylock(&so->tx_lock))
return -EAGAIN;
-
- /* wait for complete transmission of current pdu */
- wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ } else {
+ ret = down_interruptible(&so->tx_lock);
+ if (ret)
+ return ret;
}
- if (!size || size > 4095)
+ if (!size || size > 4095) {
+ up(&so->tx_lock);
return -EINVAL;
+ }
err = memcpy_fromiovec(so->tx.buf, msg->msg_iov, size);
- if (err < 0)
+ if (err < 0) {
+ up(&so->tx_lock);
return err;
+ }
dev = dev_get_by_index(&init_net, so->ifindex);
- if (!dev)
+ if (!dev) {
+ up(&so->tx_lock);
return -ENXIO;
+ }
skb = sock_alloc_send_skb(sk, sizeof(*cf),
msg->msg_flags & MSG_DONTWAIT, &err);
if (!skb) {
dev_put(dev);
+ up(&so->tx_lock);
return err;
}
@@ -713,7 +732,7 @@
cf->data[ae] = size | N_PCI_SF;
so->tx.state = ISOTP_IDLE;
- wake_up_interruptible(&so->wait);
+ up(&so->tx_lock);
} else {
/* send first frame and wait for FC */
@@ -1072,7 +1091,7 @@
tasklet_init(&so->txtsklet, isotp_tx_timer_tsklet, (unsigned long)so);
- init_waitqueue_head(&so->wait);
+ sema_init(&so->tx_lock);
so->notifier.notifier_call = isotp_notifier;
register_netdevice_notifier(&so->notifier);
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
