Franck Bui-Huu wrote:
>
> In devices.c, it's used by usb_device_poll() and usb_device_lseek()
> functions.
>
> usb_device_poll():
>
> It seems to use it only to protect usb_device_status
> structure. Can't we use a simple mutex here ?
actually I don't think we need a lock at all here, if the
usb_device_status structure is allocated per process.
Moreover this function seems to be broken because it almost always
raises the POLLIN bit. I tested it and poll(2) always returns with
POLLIN bit set although no event happens on the bus...is this
behaviour expected ?
>
> usb_device_lseek():
>
> can we use the inode's mutex instead ?
>
> In devio.c, it's used by usbdev_open() and usbdev_lseek()
> functions. In both function can't we also use the inode's mutex
> instead ?
this patch below does the following:
usb_device_poll() -> no more lock needed because usb_device_status
structure is allocated when opening the file. It also fixes the
potential bug described previously
usb_device_lseek() -> drop the BKL and use the inode's mutex
usbdev_lseek() -> drop the BKL and use the inode's mutex
usbdev_open() -> drop the BKL. I don't know why it was used...
-- >8 --
diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index c0f3734..d07f3ac 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -590,50 +590,34 @@ static ssize_t usb_device_read(struct fi
return total_written;
}
-/* Kernel lock for "lastev" protection */
static unsigned int usb_device_poll(struct file *file, struct
poll_table_struct *wait)
{
- struct usb_device_status *st = (struct usb_device_status
*)file->private_data;
+ struct usb_device_status *st;
unsigned int mask = 0;
- lock_kernel();
- if (!st) {
- st = kmalloc(sizeof(struct usb_device_status), GFP_KERNEL);
- if (!st) {
- unlock_kernel();
- return POLLIN;
- }
-
- /* 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;
- }
-
- /*
- * 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);
+
+ st = (struct usb_device_status *)file->private_data;
if (st->lastev != conndiscevcnt)
mask |= POLLIN;
st->lastev = conndiscevcnt;
- unlock_kernel();
+
return mask;
}
static int usb_device_open(struct inode *inode, struct file *file)
{
- file->private_data = NULL;
- return 0;
+ struct usb_device_status *st;
+ int rv = -ENOMEM;
+
+ st = kmalloc(sizeof(struct usb_device_status), GFP_KERNEL);
+ if (st) {
+ st->lastev = conndiscevcnt;
+ file->private_data = st;
+ rv = 0;
+ }
+ return rv;
}
static int usb_device_release(struct inode *inode, struct file *file)
@@ -647,8 +631,7 @@ static loff_t usb_device_lseek(struct fi
{
loff_t ret;
- lock_kernel();
-
+ mutex_lock(&file->f_dentry->d_inode->i_mutex);
switch (orig) {
case 0:
file->f_pos = offset;
@@ -662,8 +645,7 @@ static loff_t usb_device_lseek(struct fi
default:
ret = -EINVAL;
}
-
- unlock_kernel();
+ mutex_unlock(&file->f_dentry->d_inode->i_mutex);
return ret;
}
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 545da37..dfe54d5 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -94,8 +94,7 @@ static loff_t usbdev_lseek(struct file *
{
loff_t ret;
- lock_kernel();
-
+ mutex_lock(&file->f_dentry->d_inode->i_mutex);
switch (orig) {
case 0:
file->f_pos = offset;
@@ -109,8 +108,7 @@ static loff_t usbdev_lseek(struct file *
default:
ret = -EINVAL;
}
-
- unlock_kernel();
+ mutex_unlock(&file->f_dentry->d_inode->i_mutex);
return ret;
}
@@ -539,15 +537,10 @@ static int usbdev_open(struct inode *ino
struct dev_state *ps;
int ret;
- /*
- * no locking necessary here, as chrdev_open has the kernel lock
- * (still acquire the kernel lock for safety)
- */
ret = -ENOMEM;
if (!(ps = kmalloc(sizeof(struct dev_state), GFP_KERNEL)))
- goto out_nolock;
+ goto out;
- lock_kernel();
ret = -ENOENT;
/* check if we are called from a real node or usbfs */
if (imajor(inode) == USB_DEVICE_MAJOR)
@@ -576,8 +569,6 @@ static int usbdev_open(struct inode *ino
list_add_tail(&ps->list, &dev->filelist);
file->private_data = ps;
out:
- unlock_kernel();
- out_nolock:
return ret;
}
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
