Thread: [usbip-devel] [PATCH 0/6] staging: usbip: Bugfixes and cleanups
Status: Alpha
Brought to you by:
hirofuchi
Menu
▾
▴
usbip-devel
|
From: Bernard B. <b-l...@la...> - 2012-10-21 20:02:26
|
This patch set fixes a number of crashes, deadlocks and resource leaks in usbip. It also makes some changes to improve style and consistency. Bernard Blackham (6): staging: usbip: Don't leak struct file. staging: usbip: Rename dum -> vhci for consistency staging: usbip: Avoid superfluous set HC_STATE_RUNNING in vhci_start staging: usbip: Locking and logic fixes. staging: usbip: Simplify list logic in vhci_tx.c. staging: usbip: Minor cleanups. drivers/staging/usbip/stub_dev.c | 3 +- drivers/staging/usbip/usbip_common.c | 4 +- drivers/staging/usbip/usbip_event.c | 2 +- drivers/staging/usbip/vhci_hcd.c | 261 +++++++++++++++++----------------- drivers/staging/usbip/vhci_rx.c | 46 +++--- drivers/staging/usbip/vhci_sysfs.c | 37 ++--- drivers/staging/usbip/vhci_tx.c | 19 ++- 7 files changed, 187 insertions(+), 185 deletions(-) -- 1.7.10.4 |
|
From: Bernard B. <b-l...@la...> - 2012-10-21 20:02:26
|
HC_STATE_RUNNING is already set by the usb core. Signed-off-by: Bernard Blackham <b-l...@la...> --- drivers/staging/usbip/vhci_hcd.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c index b43fff9..b51e4ee 100644 --- a/drivers/staging/usbip/vhci_hcd.c +++ b/drivers/staging/usbip/vhci_hcd.c @@ -929,7 +929,6 @@ static int vhci_start(struct usb_hcd *hcd) spin_lock_init(&vhci->lock); hcd->power_budget = 0; /* no limit */ - hcd->state = HC_STATE_RUNNING; hcd->uses_new_polling = 1; /* vhci_hcd is now ready to be controlled through sysfs */ -- 1.7.10.4 |
|
From: Bernard B. <b-l...@la...> - 2012-10-21 20:02:26
|
The name dum seems to be historical (copy/paste from dummy driver?).
Rename that to "vhci" to be consistent with usage elsewhere in usbip.
There are no functional changes here.
Signed-off-by: Bernard Blackham <b-l...@la...>
---
drivers/staging/usbip/vhci_hcd.c | 82 +++++++++++++++++++-------------------
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 2e33ded..b43fff9 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -157,7 +157,7 @@ void rh_port_disconnect(int rhport)
usbip_dbg_vhci_rh("rh_port_disconnect %d\n", rhport);
spin_lock_irqsave(&the_controller->lock, flags);
- /* stop_activity(dum, driver); */
+ /* stop_activity(vhci, driver); */
the_controller->port_status[rhport] &= ~USB_PORT_STAT_CONNECTION;
the_controller->port_status[rhport] |=
(1 << USB_PORT_FEAT_C_CONNECTION);
@@ -245,7 +245,7 @@ static inline void hub_descriptor(struct usb_hub_descriptor *desc)
static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
u16 wIndex, char *buf, u16 wLength)
{
- struct vhci_hcd *dum;
+ struct vhci_hcd *vhci;
int retval = 0;
unsigned long flags;
int rhport;
@@ -265,13 +265,13 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
pr_err("invalid port number %d\n", wIndex);
rhport = ((__u8)(wIndex & 0x00ff)) - 1;
- dum = hcd_to_vhci(hcd);
+ vhci = hcd_to_vhci(hcd);
- spin_lock_irqsave(&dum->lock, flags);
+ spin_lock_irqsave(&vhci->lock, flags);
/* store old status and compare now and old later */
if (usbip_dbg_flag_vhci_rh) {
- memcpy(prev_port_status, dum->port_status,
+ memcpy(prev_port_status, vhci->port_status,
sizeof(prev_port_status));
}
@@ -282,31 +282,31 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
case ClearPortFeature:
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
- if (dum->port_status[rhport] & USB_PORT_STAT_SUSPEND) {
+ if (vhci->port_status[rhport] & USB_PORT_STAT_SUSPEND) {
/* 20msec signaling */
- dum->resuming = 1;
- dum->re_timeout =
+ vhci->resuming = 1;
+ vhci->re_timeout =
jiffies + msecs_to_jiffies(20);
}
break;
case USB_PORT_FEAT_POWER:
usbip_dbg_vhci_rh(" ClearPortFeature: "
"USB_PORT_FEAT_POWER\n");
- dum->port_status[rhport] = 0;
- /* dum->address = 0; */
- /* dum->hdev = 0; */
- dum->resuming = 0;
+ vhci->port_status[rhport] = 0;
+ /* vhci->address = 0; */
+ /* vhci->hdev = 0; */
+ vhci->resuming = 0;
break;
case USB_PORT_FEAT_C_RESET:
usbip_dbg_vhci_rh(" ClearPortFeature: "
"USB_PORT_FEAT_C_RESET\n");
- switch (dum->vdev[rhport].speed) {
+ switch (vhci->vdev[rhport].speed) {
case USB_SPEED_HIGH:
- dum->port_status[rhport] |=
+ vhci->port_status[rhport] |=
USB_PORT_STAT_HIGH_SPEED;
break;
case USB_SPEED_LOW:
- dum->port_status[rhport] |=
+ vhci->port_status[rhport] |=
USB_PORT_STAT_LOW_SPEED;
break;
default:
@@ -315,7 +315,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
default:
usbip_dbg_vhci_rh(" ClearPortFeature: default %x\n",
wValue);
- dum->port_status[rhport] &= ~(1 << wValue);
+ vhci->port_status[rhport] &= ~(1 << wValue);
break;
}
break;
@@ -339,40 +339,40 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
/* whoever resets or resumes must GetPortStatus to
* complete it!!
* */
- if (dum->resuming && time_after(jiffies, dum->re_timeout)) {
- dum->port_status[rhport] |=
+ if (vhci->resuming && time_after(jiffies, vhci->re_timeout)) {
+ vhci->port_status[rhport] |=
(1 << USB_PORT_FEAT_C_SUSPEND);
- dum->port_status[rhport] &=
+ vhci->port_status[rhport] &=
~(1 << USB_PORT_FEAT_SUSPEND);
- dum->resuming = 0;
- dum->re_timeout = 0;
- /* if (dum->driver && dum->driver->resume) {
- * spin_unlock (&dum->lock);
- * dum->driver->resume (&dum->gadget);
- * spin_lock (&dum->lock);
+ vhci->resuming = 0;
+ vhci->re_timeout = 0;
+ /* if (vhci->driver && vhci->driver->resume) {
+ * spin_unlock (&vhci->lock);
+ * vhci->driver->resume (&vhci->gadget);
+ * spin_lock (&vhci->lock);
* } */
}
- if ((dum->port_status[rhport] & (1 << USB_PORT_FEAT_RESET)) !=
- 0 && time_after(jiffies, dum->re_timeout)) {
- dum->port_status[rhport] |=
+ if ((vhci->port_status[rhport] & (1 << USB_PORT_FEAT_RESET)) !=
+ 0 && time_after(jiffies, vhci->re_timeout)) {
+ vhci->port_status[rhport] |=
(1 << USB_PORT_FEAT_C_RESET);
- dum->port_status[rhport] &=
+ vhci->port_status[rhport] &=
~(1 << USB_PORT_FEAT_RESET);
- dum->re_timeout = 0;
+ vhci->re_timeout = 0;
- if (dum->vdev[rhport].ud.status ==
+ if (vhci->vdev[rhport].ud.status ==
VDEV_ST_NOTASSIGNED) {
usbip_dbg_vhci_rh(" enable rhport %d "
"(status %u)\n",
rhport,
- dum->vdev[rhport].ud.status);
- dum->port_status[rhport] |=
+ vhci->vdev[rhport].ud.status);
+ vhci->port_status[rhport] |=
USB_PORT_STAT_ENABLE;
}
}
- ((u16 *) buf)[0] = cpu_to_le16(dum->port_status[rhport]);
- ((u16 *) buf)[1] = cpu_to_le16(dum->port_status[rhport] >> 16);
+ ((u16 *) buf)[0] = cpu_to_le16(vhci->port_status[rhport]);
+ ((u16 *) buf)[1] = cpu_to_le16(vhci->port_status[rhport] >> 16);
usbip_dbg_vhci_rh(" GetPortStatus bye %x %x\n", ((u16 *)buf)[0],
((u16 *)buf)[1]);
@@ -391,21 +391,21 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
usbip_dbg_vhci_rh(" SetPortFeature: "
"USB_PORT_FEAT_RESET\n");
/* if it's already running, disconnect first */
- if (dum->port_status[rhport] & USB_PORT_STAT_ENABLE) {
- dum->port_status[rhport] &=
+ if (vhci->port_status[rhport] & USB_PORT_STAT_ENABLE) {
+ vhci->port_status[rhport] &=
~(USB_PORT_STAT_ENABLE |
USB_PORT_STAT_LOW_SPEED |
USB_PORT_STAT_HIGH_SPEED);
/* FIXME test that code path! */
}
/* 50msec reset signaling */
- dum->re_timeout = jiffies + msecs_to_jiffies(50);
+ vhci->re_timeout = jiffies + msecs_to_jiffies(50);
/* FALLTHROUGH */
default:
usbip_dbg_vhci_rh(" SetPortFeature: default %d\n",
wValue);
- dum->port_status[rhport] |= (1 << wValue);
+ vhci->port_status[rhport] |= (1 << wValue);
break;
}
break;
@@ -425,12 +425,12 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
/* Only dump valid port status */
if (rhport >= 0) {
dump_port_status_diff(prev_port_status[rhport],
- dum->port_status[rhport]);
+ vhci->port_status[rhport]);
}
}
usbip_dbg_vhci_rh(" bye\n");
- spin_unlock_irqrestore(&dum->lock, flags);
+ spin_unlock_irqrestore(&vhci->lock, flags);
return retval;
}
--
1.7.10.4
|
|
From: Bernard B. <b-l...@la...> - 2012-10-21 20:02:26
|
We don't need to iterate over a list just to pull off the head.
Signed-off-by: Bernard Blackham <b-l...@la...>
---
drivers/staging/usbip/vhci_tx.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/usbip/vhci_tx.c b/drivers/staging/usbip/vhci_tx.c
index 9b437e7..89c8cb2 100644
--- a/drivers/staging/usbip/vhci_tx.c
+++ b/drivers/staging/usbip/vhci_tx.c
@@ -47,19 +47,18 @@ static void setup_cmd_submit_pdu(struct usbip_header *pdup, struct urb *urb)
static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
{
unsigned long flags;
- struct vhci_priv *priv, *tmp;
+ struct vhci_priv *priv = NULL;
spin_lock_irqsave(&vdev->priv_lock, flags);
- list_for_each_entry_safe(priv, tmp, &vdev->priv_tx, list) {
+ if (!list_empty(&vdev->priv_tx)) {
+ priv = list_first_entry(&vdev->priv_tx, struct vhci_priv, list);
list_move_tail(&priv->list, &vdev->priv_rx);
- spin_unlock_irqrestore(&vdev->priv_lock, flags);
- return priv;
}
spin_unlock_irqrestore(&vdev->priv_lock, flags);
- return NULL;
+ return priv;
}
static int vhci_send_cmd_submit(struct vhci_device *vdev)
@@ -137,19 +136,19 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
static struct vhci_unlink *dequeue_from_unlink_tx(struct vhci_device *vdev)
{
unsigned long flags;
- struct vhci_unlink *unlink, *tmp;
+ struct vhci_unlink *unlink = NULL;
spin_lock_irqsave(&vdev->priv_lock, flags);
- list_for_each_entry_safe(unlink, tmp, &vdev->unlink_tx, list) {
+ if (!list_empty(&vdev->unlink_tx)) {
+ unlink = list_first_entry(&vdev->unlink_tx, struct vhci_unlink,
+ list);
list_move_tail(&unlink->list, &vdev->unlink_rx);
- spin_unlock_irqrestore(&vdev->priv_lock, flags);
- return unlink;
}
spin_unlock_irqrestore(&vdev->priv_lock, flags);
- return NULL;
+ return unlink;
}
static int vhci_send_cmd_unlink(struct vhci_device *vdev)
--
1.7.10.4
|
|
From: Bernard B. <be...@la...> - 2012-10-21 20:02:27
|
usbip takes a reference on a struct file which is passed in via
sysfs. Previously, this reference was never cleaned up, although
the socket it referred to was.
This patch drops the corresponding reference (found with the
socket's ->file backpointer) instead of just closing the socket.
Signed-off-by: Bernard Blackham <b-l...@la...>
---
drivers/staging/usbip/stub_dev.c | 3 ++-
drivers/staging/usbip/usbip_common.c | 4 +++-
drivers/staging/usbip/vhci_hcd.c | 10 +++++++---
drivers/staging/usbip/vhci_sysfs.c | 6 +++++-
4 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/usbip/stub_dev.c b/drivers/staging/usbip/stub_dev.c
index 92ced35..bed4900 100644
--- a/drivers/staging/usbip/stub_dev.c
+++ b/drivers/staging/usbip/stub_dev.c
@@ -18,6 +18,7 @@
*/
#include <linux/device.h>
+#include <linux/file.h>
#include <linux/kthread.h>
#include <linux/module.h>
@@ -199,7 +200,7 @@ static void stub_shutdown_connection(struct usbip_device *ud)
* not touch NULL socket.
*/
if (ud->tcp_socket) {
- sock_release(ud->tcp_socket);
+ fput(ud->tcp_socket->file);
ud->tcp_socket = NULL;
}
diff --git a/drivers/staging/usbip/usbip_common.c b/drivers/staging/usbip/usbip_common.c
index 70f23026..469a4ef 100644
--- a/drivers/staging/usbip/usbip_common.c
+++ b/drivers/staging/usbip/usbip_common.c
@@ -410,8 +410,10 @@ struct socket *sockfd_to_socket(unsigned int sockfd)
inode = file->f_dentry->d_inode;
- if (!inode || !S_ISSOCK(inode->i_mode))
+ if (!inode || !S_ISSOCK(inode->i_mode)) {
+ fput(file);
return NULL;
+ }
socket = SOCKET_I(inode);
diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index dfeb492..2e33ded 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -18,6 +18,7 @@
*/
#include <linux/init.h>
+#include <linux/file.h>
#include <linux/kernel.h>
#include <linux/kthread.h>
#include <linux/module.h>
@@ -822,8 +823,8 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
pr_info("stop threads\n");
/* active connection is closed */
- if (vdev->ud.tcp_socket != NULL) {
- sock_release(vdev->ud.tcp_socket);
+ if (vdev->ud.tcp_socket) {
+ fput(vdev->ud.tcp_socket->file);
vdev->ud.tcp_socket = NULL;
}
pr_info("release socket\n");
@@ -869,7 +870,10 @@ static void vhci_device_reset(struct usbip_device *ud)
usb_put_dev(vdev->udev);
vdev->udev = NULL;
- ud->tcp_socket = NULL;
+ if (ud->tcp_socket) {
+ fput(ud->tcp_socket->file);
+ ud->tcp_socket = NULL;
+ }
ud->status = VDEV_ST_NULL;
spin_unlock(&ud->lock);
diff --git a/drivers/staging/usbip/vhci_sysfs.c b/drivers/staging/usbip/vhci_sysfs.c
index 7ce9c2f..c66e9c0 100644
--- a/drivers/staging/usbip/vhci_sysfs.c
+++ b/drivers/staging/usbip/vhci_sysfs.c
@@ -18,6 +18,7 @@
*/
#include <linux/kthread.h>
+#include <linux/file.h>
#include <linux/net.h>
#include "usbip_common.h"
@@ -189,7 +190,8 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
if (valid_args(rhport, speed) < 0)
return -EINVAL;
- /* check sockfd */
+ /* Extract socket from fd. */
+ /* The correct way to clean this up is to fput(socket->file). */
socket = sockfd_to_socket(sockfd);
if (!socket)
return -EINVAL;
@@ -206,6 +208,8 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr,
spin_unlock(&vdev->ud.lock);
spin_unlock(&the_controller->lock);
+ fput(socket->file);
+
dev_err(dev, "port %d already used\n", rhport);
return -EINVAL;
}
--
1.7.10.4
|
|
From: Bernard B. <b-l...@la...> - 2012-10-21 20:02:32
|
Clean up some debug messages and comments.
Signed-off-by: Bernard Blackham <b-l...@la...>
---
drivers/staging/usbip/usbip_event.c | 2 +-
drivers/staging/usbip/vhci_hcd.c | 23 ++++++++++-------------
drivers/staging/usbip/vhci_rx.c | 12 ++++++------
3 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/usbip/usbip_event.c b/drivers/staging/usbip/usbip_event.c
index d332a34..be94143 100644
--- a/drivers/staging/usbip/usbip_event.c
+++ b/drivers/staging/usbip/usbip_event.c
@@ -53,7 +53,7 @@ static int event_handler(struct usbip_device *ud)
ud->event &= ~USBIP_EH_UNUSABLE;
}
- /* Stop the error handler. */
+ /* Stop the event handler. */
if (ud->event & USBIP_EH_BYE)
return -1;
}
diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index bb54fc5..6568f1f 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -219,8 +219,6 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
}
}
- pr_info("changed %d\n", changed);
-
if (hcd->state == HC_STATE_SUSPENDED)
usb_hcd_resume_root_hub(hcd);
@@ -659,7 +657,7 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
int ret = 0;
int giveback = 0;
- pr_info("dequeue a urb %p\n", urb);
+ pr_debug("dequeue a urb %p\n", urb);
priv = urb->hcpriv;
if (!priv) {
@@ -680,8 +678,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
if (!vdev->ud.tcp_socket) {
/* tcp connection is closed */
+ pr_debug("device %p seems to be disconnected\n", vdev);
- pr_info("device %p seems to be disconnected\n", vdev);
spin_lock(&vdev->priv_lock);
list_del(&priv->list);
spin_unlock(&vdev->priv_lock);
@@ -694,7 +692,7 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
* vhci_rx will receive RET_UNLINK and give back the URB.
* Otherwise, we give back it here.
*/
- pr_info("gives back urb %p\n", urb);
+ pr_debug("gives back urb %p\n", urb);
usb_hcd_unlink_urb_from_ep(hcd, urb);
@@ -714,11 +712,11 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
unlink->seqnum = atomic_inc_return(&the_controller->seqnum);
if (unlink->seqnum == 0xffff)
- pr_info("seqnum max\n");
+ pr_debug("seqnum max\n");
- unlink->unlink_seqnum = priv->seqnum;
+ pr_debug("device %p seems to be still connected\n", vdev);
- pr_info("device %p seems to be still connected\n", vdev);
+ unlink->unlink_seqnum = priv->seqnum;
spin_lock(&vdev->priv_lock);
@@ -763,12 +761,11 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
}
/* 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",
+ pr_debug("the urb (seqnum %lu) was already given back\n",
unlink->unlink_seqnum);
kfree(unlink);
continue;
@@ -814,14 +811,14 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
if (vdev->ud.tcp_tx)
kthread_stop_put(vdev->ud.tcp_tx);
- pr_info("stop threads\n");
+ pr_debug("stop threads\n");
/* active connection is closed */
if (vdev->ud.tcp_socket) {
fput(vdev->ud.tcp_socket->file);
vdev->ud.tcp_socket = NULL;
}
- pr_info("release socket\n");
+ pr_debug("release socket\n");
vhci_device_unlink_cleanup(vdev);
@@ -847,7 +844,7 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
*/
rh_port_disconnect(vdev->rhport);
- pr_info("disconnect device\n");
+ pr_debug("disconnect device\n");
}
diff --git a/drivers/staging/usbip/vhci_rx.c b/drivers/staging/usbip/vhci_rx.c
index ec91306..85f7580 100644
--- a/drivers/staging/usbip/vhci_rx.c
+++ b/drivers/staging/usbip/vhci_rx.c
@@ -75,7 +75,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
if (!urb) {
pr_err("cannot find a urb of seqnum %u\n", pdu->base.seqnum);
- pr_info("max seqnum %d\n",
+ pr_debug("max seqnum %d\n",
atomic_read(&the_controller->seqnum));
usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
return;
@@ -86,11 +86,11 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
/* recv transfer buffer */
if (usbip_recv_xbuff(ud, urb) < 0)
- return;
+ return; /* XXX: unlink/giveback? */
/* recv iso_packet_descriptor */
if (usbip_recv_iso(ud, urb) < 0)
- return;
+ return; /* XXX: unlink/giveback? */
/* restore the padding in iso packets */
usbip_pad_iso(ud, urb);
@@ -120,7 +120,7 @@ static struct vhci_unlink *dequeue_pending_unlink(struct vhci_device *vdev,
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);
+ pr_debug("unlink->seqnum %lu\n", unlink->seqnum);
if (unlink->seqnum == pdu->base.seqnum) {
usbip_dbg_vhci_rx("found pending unlink, %lu\n",
unlink->seqnum);
@@ -160,14 +160,14 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev,
* already received the result of its submit result and gave
* back the URB.
*/
- pr_info("the urb (seqnum %d) was already given back\n",
+ pr_debug("the urb (seqnum %d) was already given back\n",
pdu->base.seqnum);
} else {
usbip_dbg_vhci_rx("now giveback urb %p\n", urb);
/* If unlink is succeed, status is -ECONNRESET */
urb->status = pdu->u.ret_unlink.status;
- pr_info("urb->status %d\n", urb->status);
+ pr_debug("urb->status %d\n", urb->status);
spin_lock_irq(&the_controller->lock);
usb_hcd_unlink_urb_from_ep(vhci_to_hcd(the_controller), urb);
--
1.7.10.4
|
|
From: Bernard B. <b-l...@la...> - 2012-10-21 20:02:33
|
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-l...@la...>
---
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
|
|
From: Greg KH <gr...@kr...> - 2012-10-22 20:40:00
|
On Mon, Oct 22, 2012 at 06:45:36AM +1100, Bernard Blackham wrote: > 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. That's a lot to do all in one patch, making it hard to review. Can you split this up into smaller pieces, one patch per thing you do above, so we can apply it? thanks, greg k-h |
|
From: Bernard B. <be...@la...> - 2012-10-22 21:57:48
|
On 22/10/2012, at 9:39 PM, Greg KH <gr...@kr...> wrote: > On Mon, Oct 22, 2012 at 06:45:36AM +1100, Bernard Blackham wrote: >> 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. > > That's a lot to do all in one patch, making it hard to review. Can you > split this up into smaller pieces, one patch per thing you do above, so > we can apply it? No problem. I'll try to split these up and redo them against usb-next. Cheers, Bernard. |
|
From: Greg KH <gr...@kr...> - 2012-10-22 20:40:38
|
On Mon, Oct 22, 2012 at 06:44:41AM +1100, Bernard Blackham wrote: > This patch set fixes a number of crashes, deadlocks and resource > leaks in usbip. It also makes some changes to improve style and > consistency. I've applied 2 of these, care to redo the rest against my latest usb-next tree and resend them? thanks, greg k-h |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X