Here is my try of lock_kernel()/unlock_kernel() removal from usblp.c.
Reason for the removal is simply the depreciation of the BLK, it may
vanish from the kernel some day.
I could remove it only by adding driver specific lock like in usb-skeleton.c.
Also the locking order had to be changed to be correct (with lock_kernel()
it doesnt matter ?). It may be that it could still be simplified, but basically
open() needs to be protected against removal of struct usblp and then
struct usblp field accesses need to be protected, so current locking model
seems reasonable.
If somebody knows locking is USB well, it would be nice to know if this
patch is OK ?
------------ 03_usblp_no_BKL.patch ---------------
--- linux-2.5.50-usb_02/drivers/usb/class/usblp.c Thu Dec 12 23:21:44 2002
+++ linux-2.5.50-usb/drivers/usb/class/usblp.c Fri Dec 13 00:03:31 2002
@@ -188,6 +188,9 @@
extern devfs_handle_t usb_devfs_handle; /* /dev/usb dir. */
+/* lock to protect driver access */
+static DECLARE_MUTEX (usblp_mutex);
+
/* Quirks: various printer quirks are handled by this table & its flags. */
struct quirk_printer_struct {
@@ -328,23 +331,25 @@
struct usb_interface *intf;
int retval;
- if (minor < 0 || minor >= USBLP_MINORS)
- return -ENODEV;
-
- lock_kernel();
+ down (&usblp_mutex);
retval = -ENODEV;
intf = usb_find_interface(&usblp_driver, mk_kdev(USB_MAJOR,minor));
- if (!intf) {
+ if (!intf)
goto out;
- }
+
usblp = dev_get_drvdata (&intf->dev);
if (!usblp || !usblp->dev)
goto out;
- retval = -EBUSY;
- if (usblp->used)
- goto out;
+ down (&usblp->sem);
+ up (&usblp_mutex);
+
+ if (usblp->used) {
+ up (&usblp->sem);
+ return -EBUSY;
+ }
+
/*
* TODO: need to implement LP_ABORTOPEN + O_NONBLOCK as in drivers/char/lp.c
???
@@ -354,15 +359,11 @@
#if 0
if ((retval = usblp_check_status(usblp, 0))) {
retval = retval > 1 ? -EIO : -ENOSPC;
- goto out;
+ up (&usblp->sem);
+ return retval;
}
-#else
- retval = 0;
#endif
- usblp->used = 1;
- file->private_data = usblp;
-
usblp->writeurb->transfer_buffer_length = 0;
usblp->writeurb->status = 0;
usblp->wcomplete = 1; /* we begin writeable */
@@ -372,13 +373,17 @@
usblp->readcount = 0;
usblp->readurb->dev = usblp->dev;
if (usb_submit_urb(usblp->readurb, GFP_KERNEL) < 0) {
- retval = -EIO;
- usblp->used = 0;
- file->private_data = NULL;
+ up (&usblp->sem);
+ return -EIO;
}
}
+ usblp->used = 1;
+ file->private_data = usblp;
+
+ up (&usblp->sem);
+ return 0;
out:
- unlock_kernel();
+ up (&usblp_mutex);
return retval;
}
@@ -410,19 +415,18 @@
{
struct usblp *usblp = file->private_data;
+ down (&usblp_mutex);
down (&usblp->sem);
- lock_kernel();
usblp->used = 0;
if (usblp->dev) {
usblp_unlink_urbs(usblp);
up(&usblp->sem);
} else /* finish cleanup from disconnect */
usblp_cleanup (usblp);
- unlock_kernel();
+ up (&usblp_mutex);
return 0;
}
-/* No kernel lock - fine */
static unsigned int usblp_poll(struct file *file, struct poll_table_struct *wait)
{
struct usblp *usblp = file->private_data;
@@ -1115,13 +1119,14 @@
/* remove device id to disable open() */
intf->kdev = NODEV;
+ down (&usblp_mutex);
+
if (!usblp || !usblp->dev) {
err("bogus disconnect");
BUG ();
}
down (&usblp->sem);
- lock_kernel();
usblp->dev = NULL;
dev_set_drvdata (&intf->dev, NULL);
@@ -1131,7 +1136,8 @@
usblp_cleanup (usblp);
else /* cleanup later, on close */
up (&usblp->sem);
- unlock_kernel();
+
+ up (&usblp_mutex);
}
static struct usb_device_id usblp_ids [] = {
------------------------
Also I would like comment if any locking is missing from
function usb_find_interface(), which should be in another
thread ("[FYI] 2.5 changes in usb core") in this list too.
I think it went into the core a bit faster than I expected.
That function is iterating through devices list inside driver
structure, and Im not sure if it is safe without any lock.
Are locks needed for reading lists ? Actually it is now
protected in the sample drivers (usblp and scanner)
with the drivers lock, but I dont know if that
is enough ?
I was really hoping that only one lock would be needed. If
the list reading does not need a lock, then perhaps lock
in device local store could be moved to struct usb_interface,
so the same lock would be available in open() and also in the
driver (needs pointer to usb_interface stored to local data
structure in probe()). Anyway the changes wont happen fast,
needs still a lot thinking, implementation and testing too.
I just wonder if it is fundamentally posssible at all ? And is
it really useful enough to be tried ?
----
Kari H�meenaho
-------------------------------------------------------
This sf.net email is sponsored by:
With Great Power, Comes Great Responsibility
Learn to use your power at OSDN's High Performance Computing Channel
http://hpc.devchannel.org/
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
