Proper lock usage in kingsun-sir
Alex Villacís Lasso <[email protected]>
| Newsgroups | gmane.linux.usb.devel |
|---|---|
| Message-ID | <[email protected]> |
A few days ago I started reading the code of drivers/net/irda/irda-usb.c
in order to compare it to drivers/net/irda/kingsun-sir.c and see whether
my implementation is complete. One of the things I could notice is that
irda-usb.c has a more extensive use of locking than my driver, up to the
point that I am worrying whether my driver could be racy. The attached
patch (just to have something to discuss about, not for merging) adds
some variables and locks to kingsun-sir.c, modeled after what I saw in
irda-usb.c . However, I would like to know whether the patch is doing
the right thing. Are the locks really necessary for the kingsun-sir
driver in the same places as in irda-usb.c ? Should I add locks in more
places in my driver? What do you think?
Alex Villacís Lasso
--
perl -e '$x=2.4;print sprintf("%.0f + %.0f = %.0f\n",$x,$x,$x+$x);'
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
kingsun-sir-add-locking.diff
(text/x-patch, 2.8 KB)
--- linux-2.6.23-rc5/drivers/net/irda/kingsun-sir.c 2007-09-03 21:18:11.000000000 -0500
+++ kingsun-sir.c 2007-09-01 23:53:00.000000000 -0500
@@ -126,6 +126,9 @@
struct urb *tx_urb;
struct urb *rx_urb;
+
+ int present; /* Is device still present on bus? */
+ int netopen; /* Is device still open? */
};
/* Callback transmission routine */
@@ -144,8 +147,16 @@
if (urb->status != 0) {
err("kingsun_send_irq: urb asynchronously failed - %d",
urb->status);
+ return;
+ }
+
+ spin_lock(&kingsun->lock);
+ if (kingsun->present && kingsun->netopen) {
+ netif_wake_queue(netdev);
+ } else {
+ IRDA_DEBUG(0, "%s(), Network is gone...\n", __FUNCTION__);
}
- netif_wake_queue(netdev);
+ spin_unlock(&kingsun->lock);
}
/*
@@ -168,6 +179,10 @@
kingsun = netdev_priv(netdev);
spin_lock(&kingsun->lock);
+ if (!kingsun->present) {
+ IRDA_DEBUG(0, "%s(), Device is gone...\n", __FUNCTION__);
+ goto drop;
+ }
/* Append data to the end of whatever data remains to be transmitted */
wraplen = async_wrap_skb(skb,
@@ -194,7 +209,7 @@
kingsun->stats.tx_packets++;
kingsun->stats.tx_bytes += skb->len;
}
-
+drop:
dev_kfree_skb(skb);
spin_unlock(&kingsun->lock);
@@ -213,6 +228,12 @@
return;
}
+ if (!kingsun->present || !kingsun->netopen) {
+ IRDA_DEBUG(0, "%s(), Network is gone!\n", __FUNCTION__);
+ kingsun->receiving = 0;
+ return;
+ }
+
/* unlink, shutdown, unplug, other nasties */
if (urb->status != 0) {
err("kingsun_rcv_irq: urb asynchronously failed - %d",
@@ -264,6 +285,11 @@
/* At this point, urbs are NULL, and skb is NULL (see kingsun_probe) */
kingsun->receiving = 0;
+ if(!kingsun->present) {
+ IRDA_WARNING("%s(), device not present!\n", __FUNCTION__);
+ return -1;
+ }
+
/* Initialize for SIR to copy data directly into skb. */
kingsun->rx_buff.in_frame = FALSE;
kingsun->rx_buff.state = OUTSIDE_FRAME;
@@ -294,6 +320,7 @@
err("kingsun-sir: irlap_open failed");
goto free_mem;
}
+ kingsun->netopen = 1;
/* Start first reception */
usb_fill_int_urb(kingsun->rx_urb, kingsun->usbdev,
@@ -349,6 +376,8 @@
{
struct kingsun_cb *kingsun = netdev_priv(netdev);
+ kingsun->netopen = 0;
+
/* Stop transmit processing */
netif_stop_queue(netdev);
@@ -506,6 +535,8 @@
kingsun->rx_buff.state = OUTSIDE_FRAME;
kingsun->rx_buff.skb = NULL;
kingsun->receiving = 0;
+ kingsun->present = 1;
+ kingsun->netopen = 0;
spin_lock_init(&kingsun->lock);
/* Allocate input buffer */
@@ -573,6 +604,15 @@
if (!kingsun)
return;
+ /* Update present status under spinlock */
+ spin_lock(&kingsun->lock);
+ kingsun->present = 0;
+ spin_unlock(&kingsun->lock);
+
+ if (kingsun->netopen || kingsun->irlap) {
+ netif_stop_queue(kingsun->netdev);
+ }
+
unregister_netdev(kingsun->netdev);
/* Mop up receive && transmit urb's */