This patch cleans up much of the locking in usbip, as well as fixing
some logic errors. In particular:
- some spinlocks were taken with interrupts disabled in some
places, but not always.
- many error paths duplicated unwinding code; these have been
changed to use conventional goto idioms for unwinding on error.
- usb_hcd_giveback_urb was called incorrectly when vhci_urb_enqueue
returned an error.
- vhci_device_unlink_cleanup did not properly clean up URBs which
had not yet been transmitted yet.
Signed-off-by: Bernard Blackham <b-linuxgit@xxxxxxxxxxxxxxxx>
---
drivers/staging/usbip/vhci_hcd.c | 147 ++++++++++++++++++------------------
drivers/staging/usbip/vhci_rx.c | 34 ++++-----
drivers/staging/usbip/vhci_sysfs.c | 31 ++++----
3 files changed, 104 insertions(+), 108 deletions(-)
diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index b51e4ee..bb54fc5 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -453,7 +453,7 @@ static void vhci_tx_urb(struct urb *urb)
{
struct vhci_device *vdev = get_vdev(urb->dev);
struct vhci_priv *priv;
- unsigned long flag;
+ unsigned long flags;
if (!vdev) {
pr_err("could not get virtual device");
@@ -463,11 +463,8 @@ static void vhci_tx_urb(struct urb *urb)
priv = kzalloc(sizeof(struct vhci_priv), GFP_ATOMIC);
- spin_lock_irqsave(&vdev->priv_lock, flag);
-
if (!priv) {
dev_err(&urb->dev->dev, "malloc vhci_priv\n");
- spin_unlock_irqrestore(&vdev->priv_lock, flag);
usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_MALLOC);
return;
}
@@ -481,10 +478,12 @@ static void vhci_tx_urb(struct urb *urb)
urb->hcpriv = (void *) priv;
+ spin_lock_irqsave(&vdev->priv_lock, flags);
+
list_add_tail(&priv->list, &vdev->priv_tx);
wake_up(&vdev->waitq_tx);
- spin_unlock_irqrestore(&vdev->priv_lock, flag);
+ spin_unlock_irqrestore(&vdev->priv_lock, flags);
}
static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
@@ -505,8 +504,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
if (urb->status != -EINPROGRESS) {
dev_err(dev, "URB already unlinked!, status %d\n", urb->status);
- spin_unlock_irqrestore(&the_controller->lock, flags);
- return urb->status;
+ ret = urb->status;
+ goto no_need_unlink;
}
vdev = port_to_vdev(urb->dev->portnum-1);
@@ -517,8 +516,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
vdev->ud.status == VDEV_ST_ERROR) {
dev_err(dev, "enqueue for inactive port %d\n", vdev->rhport);
spin_unlock(&vdev->ud.lock);
- spin_unlock_irqrestore(&the_controller->lock, flags);
- return -ENODEV;
+ ret = -ENODEV;
+ goto no_need_unlink;
}
spin_unlock(&vdev->ud.lock);
@@ -600,7 +599,9 @@ no_need_xmit:
usb_hcd_unlink_urb_from_ep(hcd, urb);
no_need_unlink:
spin_unlock_irqrestore(&the_controller->lock, flags);
- usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb, urb->status);
+ if (ret == 0)
+ usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb,
+ urb->status);
return ret;
}
@@ -655,43 +656,38 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
unsigned long flags;
struct vhci_priv *priv;
struct vhci_device *vdev;
+ int ret = 0;
+ int giveback = 0;
pr_info("dequeue a urb %p\n", urb);
- spin_lock_irqsave(&the_controller->lock, flags);
-
priv = urb->hcpriv;
if (!priv) {
- /* URB was never linked! or will be soon given back by
- * vhci_rx. */
- spin_unlock_irqrestore(&the_controller->lock, flags);
- return 0;
- }
-
- {
- int ret = 0;
- ret = usb_hcd_check_unlink_urb(hcd, urb, status);
- if (ret) {
- spin_unlock_irqrestore(&the_controller->lock, flags);
- return ret;
- }
+ /* URB was never linked!
+ * or will be soon given back by vhci_rx. */
+ goto out;
}
/* send unlink request here? */
vdev = priv->vdev;
+ spin_lock_irqsave(&the_controller->lock, flags);
+
+ ret = usb_hcd_check_unlink_urb(hcd, urb, status);
+ if (ret) {
+ goto out_unlock;
+ }
+
if (!vdev->ud.tcp_socket) {
/* tcp connection is closed */
- unsigned long flags2;
-
- spin_lock_irqsave(&vdev->priv_lock, flags2);
pr_info("device %p seems to be disconnected\n", vdev);
+ spin_lock(&vdev->priv_lock);
list_del(&priv->list);
- kfree(priv);
- urb->hcpriv = NULL;
+ spin_unlock(&vdev->priv_lock);
- spin_unlock_irqrestore(&vdev->priv_lock, flags2);
+ urb->hcpriv = NULL;
+ kfree(priv);
/*
* If tcp connection is alive, we have sent CMD_UNLINK.
@@ -702,26 +698,18 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock_irqrestore(&the_controller->lock, flags);
- usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb,
- urb->status);
- spin_lock_irqsave(&the_controller->lock, flags);
-
+ /* Give back the URB outside of the lock. */
+ giveback = 1;
} else {
/* tcp connection is alive */
- unsigned long flags2;
struct vhci_unlink *unlink;
- spin_lock_irqsave(&vdev->priv_lock, flags2);
-
/* setup CMD_UNLINK pdu */
unlink = kzalloc(sizeof(struct vhci_unlink), GFP_ATOMIC);
if (!unlink) {
pr_err("malloc vhci_unlink\n");
- spin_unlock_irqrestore(&vdev->priv_lock, flags2);
- spin_unlock_irqrestore(&the_controller->lock, flags);
- usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_MALLOC);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_unlock;
}
unlink->seqnum = atomic_inc_return(&the_controller->seqnum);
@@ -732,71 +720,77 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
pr_info("device %p seems to be still connected\n", vdev);
+ spin_lock(&vdev->priv_lock);
+
/* send cmd_unlink and try to cancel the pending URB in the
* peer */
list_add_tail(&unlink->list, &vdev->unlink_tx);
wake_up(&vdev->waitq_tx);
- spin_unlock_irqrestore(&vdev->priv_lock, flags2);
+ spin_unlock(&vdev->priv_lock);
}
+out_unlock:
spin_unlock_irqrestore(&the_controller->lock, flags);
+ if (giveback)
+ usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb,
+ urb->status);
+
+ if (ret == -ENOMEM)
+ usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_MALLOC);
+
+out:
usbip_dbg_vhci_hc("leave\n");
- return 0;
+ return ret;
}
static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
{
- struct vhci_unlink *unlink, *tmp;
-
- spin_lock(&the_controller->lock);
- spin_lock(&vdev->priv_lock);
-
- list_for_each_entry_safe(unlink, tmp, &vdev->unlink_tx, list) {
- pr_info("unlink cleanup tx %lu\n", unlink->unlink_seqnum);
- list_del(&unlink->list);
- kfree(unlink);
- }
+ struct vhci_unlink *unlink;
+ unsigned long flags;
- while (!list_empty(&vdev->unlink_rx)) {
+ spin_lock_irqsave(&vdev->priv_lock, flags);
+ while (!list_empty(&vdev->unlink_tx) || !list_empty(&vdev->unlink_rx)) {
struct urb *urb;
- unlink = list_first_entry(&vdev->unlink_rx, struct vhci_unlink,
- list);
+ if (list_empty(&vdev->unlink_tx)) {
+ unlink = list_first_entry(&vdev->unlink_rx,
+ struct vhci_unlink, list);
+ } else {
+ unlink = list_first_entry(&vdev->unlink_tx,
+ struct vhci_unlink, list);
+ }
/* give back URB of unanswered unlink request */
pr_info("unlink cleanup rx %lu\n", unlink->unlink_seqnum);
+ list_del(&unlink->list);
urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
if (!urb) {
pr_info("the urb (seqnum %lu) was already given back\n",
unlink->unlink_seqnum);
- list_del(&unlink->list);
kfree(unlink);
continue;
}
urb->status = -ENODEV;
- usb_hcd_unlink_urb_from_ep(vhci_to_hcd(the_controller), urb);
-
- list_del(&unlink->list);
+ spin_unlock_irqrestore(&vdev->priv_lock, flags);
- spin_unlock(&vdev->priv_lock);
- spin_unlock(&the_controller->lock);
+ spin_lock_irqsave(&the_controller->lock, flags);
+ usb_hcd_unlink_urb_from_ep(vhci_to_hcd(the_controller), urb);
+ spin_unlock_irqrestore(&the_controller->lock, flags);
usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb,
urb->status);
- spin_lock(&the_controller->lock);
- spin_lock(&vdev->priv_lock);
-
kfree(unlink);
+
+ spin_lock_irqsave(&vdev->priv_lock, flags);
}
- spin_unlock(&vdev->priv_lock);
- spin_unlock(&the_controller->lock);
+ spin_unlock_irqrestore(&vdev->priv_lock, flags);
}
/*
@@ -860,8 +854,9 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
static void vhci_device_reset(struct usbip_device *ud)
{
struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
+ unsigned long flags;
- spin_lock(&ud->lock);
+ spin_lock_irqsave(&ud->lock, flags);
vdev->speed = 0;
vdev->devid = 0;
@@ -876,14 +871,15 @@ static void vhci_device_reset(struct usbip_device *ud)
}
ud->status = VDEV_ST_NULL;
- spin_unlock(&ud->lock);
+ spin_unlock_irqrestore(&ud->lock, flags);
}
static void vhci_device_unusable(struct usbip_device *ud)
{
- spin_lock(&ud->lock);
+ unsigned long flags;
+ spin_lock_irqsave(&ud->lock, flags);
ud->status = VDEV_ST_ERROR;
- spin_unlock(&ud->lock);
+ spin_unlock_irqrestore(&ud->lock, flags);
}
static void vhci_device_init(struct vhci_device *vdev)
@@ -1105,17 +1101,18 @@ static int vhci_hcd_suspend(struct platform_device *pdev, pm_message_t state)
int rhport = 0;
int connected = 0;
int ret = 0;
+ unsigned long flags;
hcd = platform_get_drvdata(pdev);
- spin_lock(&the_controller->lock);
+ spin_lock_irqsave(&the_controller->lock, flags);
for (rhport = 0; rhport < VHCI_NPORTS; rhport++)
if (the_controller->port_status[rhport] &
USB_PORT_STAT_CONNECTION)
connected += 1;
- spin_unlock(&the_controller->lock);
+ spin_unlock_irqrestore(&the_controller->lock, flags);
if (connected > 0) {
dev_info(&pdev->dev, "We have %d active connection%s. Do not "
diff --git a/drivers/staging/usbip/vhci_rx.c b/drivers/staging/usbip/vhci_rx.c
index f0eaf04..ec91306 100644
--- a/drivers/staging/usbip/vhci_rx.c
+++ b/drivers/staging/usbip/vhci_rx.c
@@ -68,11 +68,10 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
{
struct usbip_device *ud = &vdev->ud;
struct urb *urb;
- unsigned long flags;
- spin_lock(&vdev->priv_lock);
+ spin_lock_irq(&vdev->priv_lock);
urb = pickup_urb_and_free_priv(vdev, pdu->base.seqnum);
- spin_unlock(&vdev->priv_lock);
+ spin_unlock_irq(&vdev->priv_lock);
if (!urb) {
pr_err("cannot find a urb of seqnum %u\n", pdu->base.seqnum);
@@ -101,9 +100,9 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
usbip_dbg_vhci_rx("now giveback urb %p\n", urb);
- spin_lock_irqsave(&the_controller->lock, flags);
+ spin_lock_irq(&the_controller->lock);
usb_hcd_unlink_urb_from_ep(vhci_to_hcd(the_controller), urb);
- spin_unlock_irqrestore(&the_controller->lock, flags);
+ spin_unlock_irq(&the_controller->lock);
usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb, urb->status);
@@ -115,9 +114,10 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
static struct vhci_unlink *dequeue_pending_unlink(struct vhci_device *vdev,
struct usbip_header *pdu)
{
- struct vhci_unlink *unlink, *tmp;
+ struct vhci_unlink *unlink = NULL;
+ struct vhci_unlink *tmp;
- spin_lock(&vdev->priv_lock);
+ spin_lock_irq(&vdev->priv_lock);
list_for_each_entry_safe(unlink, tmp, &vdev->unlink_rx, list) {
pr_info("unlink->seqnum %lu\n", unlink->seqnum);
@@ -126,14 +126,13 @@ static struct vhci_unlink *dequeue_pending_unlink(struct vhci_device *vdev,
unlink->seqnum);
list_del(&unlink->list);
- spin_unlock(&vdev->priv_lock);
- return unlink;
+ break;
}
}
- spin_unlock(&vdev->priv_lock);
+ spin_unlock_irq(&vdev->priv_lock);
- return NULL;
+ return unlink;
}
static void vhci_recv_ret_unlink(struct vhci_device *vdev,
@@ -141,7 +140,6 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
{
struct vhci_unlink *unlink;
struct urb *urb;
- unsigned long flags;
usbip_dump_header(pdu);
@@ -152,9 +150,9 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
return;
}
- spin_lock(&vdev->priv_lock);
+ spin_lock_irq(&vdev->priv_lock);
urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
- spin_unlock(&vdev->priv_lock);
+ spin_unlock_irq(&vdev->priv_lock);
if (!urb) {
/*
@@ -171,9 +169,9 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
urb->status = pdu->u.ret_unlink.status;
pr_info("urb->status %d\n", urb->status);
- spin_lock_irqsave(&the_controller->lock, flags);
+ spin_lock_irq(&the_controller->lock);
usb_hcd_unlink_urb_from_ep(vhci_to_hcd(the_controller), urb);
- spin_unlock_irqrestore(&the_controller->lock, flags);
+ spin_unlock_irq(&the_controller->lock);
usb_hcd_giveback_urb(vhci_to_hcd(the_controller), urb,
urb->status);
@@ -186,9 +184,9 @@ static int vhci_priv_tx_empty(struct vhci_device *vdev)
{
int empty = 0;
- spin_lock(&vdev->priv_lock);
+ spin_lock_irq(&vdev->priv_lock);
empty = list_empty(&vdev->priv_rx);
- spin_unlock(&vdev->priv_lock);
+ spin_unlock_irq(&vdev->priv_lock);
return empty;
}
diff --git a/drivers/staging/usbip/vhci_sysfs.c b/drivers/staging/usbip/vhci_sysfs.c
index c66e9c0..f992512 100644
--- a/drivers/staging/usbip/vhci_sysfs.c
+++ b/drivers/staging/usbip/vhci_sysfs.c
@@ -32,10 +32,11 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
{
char *s = out;
int i = 0;
+ unsigned long flags;
BUG_ON(!the_controller || !out);
- spin_lock(&the_controller->lock);
+ spin_lock_irqsave(&the_controller->lock, flags);
/*
* output example:
@@ -70,7 +71,7 @@ static ssize_t show_status(struct device *dev, struct device_attribute *attr,
spin_unlock(&vdev->ud.lock);
}
- spin_unlock(&the_controller->lock);
+ spin_unlock_irqrestore(&the_controller->lock, flags);
return out - s;
}
@@ -80,32 +81,31 @@ static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
static int vhci_port_disconnect(__u32 rhport)
{
struct vhci_device *vdev;
+ unsigned long flags;
+ int ret = 0;
usbip_dbg_vhci_sysfs("enter\n");
/* lock */
- spin_lock(&the_controller->lock);
+ spin_lock_irqsave(&the_controller->lock, flags);
vdev = port_to_vdev(rhport);
spin_lock(&vdev->ud.lock);
+
if (vdev->ud.status == VDEV_ST_NULL) {
pr_err("not connected %d\n", vdev->ud.status);
-
- /* unlock */
- spin_unlock(&vdev->ud.lock);
- spin_unlock(&the_controller->lock);
-
- return -EINVAL;
+ ret = -EINVAL;
}
/* unlock */
spin_unlock(&vdev->ud.lock);
- spin_unlock(&the_controller->lock);
+ spin_unlock_irqrestore(&the_controller->lock, flags);
- usbip_event_add(&vdev->ud, VDEV_EVENT_DOWN);
+ if (ret == 0)
+ usbip_event_add(&vdev->ud, VDEV_EVENT_DOWN);
- return 0;
+ return ret;
}
static ssize_t store_detach(struct device *dev, struct device_attribute *attr,
@@ -170,6 +170,7 @@ static int valid_args(__u32 rhport, enum usb_device_speed speed)
static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ unsigned long flags;
struct vhci_device *vdev;
struct socket *socket;
int sockfd = 0;
@@ -199,14 +200,14 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
/* now need lock until setting vdev status as used */
/* begin a lock */
- spin_lock(&the_controller->lock);
+ spin_lock_irqsave(&the_controller->lock, flags);
vdev = port_to_vdev(rhport);
spin_lock(&vdev->ud.lock);
if (vdev->ud.status != VDEV_ST_NULL) {
/* end of the lock */
spin_unlock(&vdev->ud.lock);
- spin_unlock(&the_controller->lock);
+ spin_unlock_irqrestore(&the_controller->lock, flags);
fput(socket->file);
@@ -223,7 +224,7 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
vdev->ud.status = VDEV_ST_NOTASSIGNED;
spin_unlock(&vdev->ud.lock);
- spin_unlock(&the_controller->lock);
+ spin_unlock_irqrestore(&the_controller->lock, flags);
/* end the lock */
vdev->ud.tcp_rx = kthread_get_run(vhci_rx_loop, &vdev->ud, "vhci_rx");
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
