On Wed, 13 Jan 2010, Oliver Neukum wrote:
> From a100d2f19f255219faf14c9ffb16615f6204bfff Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@xxxxxxxxxx>
> Date: Wed, 13 Jan 2010 09:53:40 +0100
> Subject: [PATCH 1/5] usb:Remove BKL from poll()
>
> Replace BKL with usbfs_mutex to protect a global counter
> and a per file data structure
>
> Signed-off-by: Oliver Neukum <oliver@xxxxxxxxxx>
> ---
> drivers/usb/core/devices.c | 28 +++++++++-------------------
> 1 files changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
> index 355dffc..175529f 100644
> --- a/drivers/usb/core/devices.c
> +++ b/drivers/usb/core/devices.c
> @@ -118,6 +118,7 @@ static const char *format_endpt =
> */
>
> static DECLARE_WAIT_QUEUE_HEAD(deviceconndiscwq);
> +/* guarded by usbfs_mutex */
> static unsigned int conndiscevcnt;
>
> /* this struct stores the poll state for <mountpoint>/devices pollers */
> @@ -156,7 +157,9 @@ static const struct class_info clas_info[] =
>
> void usbfs_conn_disc_event(void)
> {
> + mutex_lock(&usbfs_mutex);
> conndiscevcnt++;
> + mutex_unlock(&usbfs_mutex);
> wake_up(&deviceconndiscwq);
> }
Could you simplify this by making conndiscevcnt an atomic_t?
> @@ -629,42 +632,29 @@ static ssize_t usb_device_read(struct file *file, char __user *buf,
> static unsigned int usb_device_poll(struct file *file,
> struct poll_table_struct *wait)
> {
> - struct usb_device_status *st = file->private_data;
> + struct usb_device_status *st;
> unsigned int mask = 0;
>
> - lock_kernel();
> + mutex_lock(&usbfs_mutex);
> + st = file->private_data;
> if (!st) {
> st = kmalloc(sizeof(struct usb_device_status), GFP_KERNEL);
> -
> - /* we may have dropped BKL -
> - * need to check for having lost the race */
> - if (file->private_data) {
> - kfree(st);
> - st = file->private_data;
> - goto lost_race;
> - }
> - /* we haven't lost - check for allocation failure now */
> if (!st) {
> - unlock_kernel();
> + mutex_unlock(&usbfs_mutex);
> return POLLIN;
> }
>
> - /*
> - * need to prevent the module from being unloaded, since
> - * proc_unregister does not call the release method and
> - * we would have a memory leak
> - */
> st->lastev = conndiscevcnt;
> file->private_data = st;
> mask = POLLIN;
> }
> -lost_race:
> +
> if (file->f_mode & FMODE_READ)
> poll_wait(file, &deviceconndiscwq, wait);
> if (st->lastev != conndiscevcnt)
> mask |= POLLIN;
> st->lastev = conndiscevcnt;
> - unlock_kernel();
> + mutex_unlock(&usbfs_mutex);
> return mask;
> }
And could this be simplified by storing the count directly in
file->private_data rather than allocating it separately?
If there's a question as to whether or not private_data has already
been set, you could make the counter's low-order bit always 1
(increment it by 2 each time).
Alan Stern
--
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
