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 <[email protected]> --- 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
