Hi Greg, this is the first of a series of patches that replace the
per-file semaphore ps->devsem with the per-device semaphore
ps->dev->serialize. The role of devsem was to protect against
device disconnection. This can be done equally well using
ps->dev->serialize. On the other hand, ps->dev->serialize
protects against configuration and other changes, and has
already been introduced into usbfs in several places. Using
just one semaphore simplifies the code and removes some
remaining race conditions. It should also fix the oopses some
people have been seeing. In this first patch, a reference is
taken to the usb device as long as the usbfs file is open. That
way we can use ps->dev->serialize for as long as ps exists.
devio.c | 27 ++++++++++++++++-----------
inode.c | 3 ---
2 files changed, 16 insertions(+), 14 deletions(-)
diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c Wed Apr 14 12:15:29 2004
+++ b/drivers/usb/core/devio.c Wed Apr 14 12:15:29 2004
@@ -60,6 +60,11 @@
struct urb *urb;
};
+static inline int connected (struct usb_device *dev)
+{
+ return dev->state != USB_STATE_NOTATTACHED;
+}
+
static loff_t usbdev_lseek(struct file *file, loff_t offset, int orig)
{
loff_t ret;
@@ -94,7 +99,7 @@
pos = *ppos;
down_read(&ps->devsem);
- if (!ps->dev) {
+ if (!connected(ps->dev)) {
ret = -ENODEV;
goto err;
} else if (pos < 0) {
@@ -343,8 +348,6 @@
* all pending I/O requests; 2.6 does that.
*/
- /* prevent new I/O requests */
- ps->dev = 0;
clear_bit(intf->cur_altsetting->desc.bInterfaceNumber, &ps->ifclaimed);
usb_set_intfdata (intf, NULL);
@@ -365,7 +368,7 @@
struct usb_interface *iface;
int err;
- if (intf >= 8*sizeof(ps->ifclaimed) || !dev
+ if (intf >= 8*sizeof(ps->ifclaimed)
|| intf >= dev->actconfig->desc.bNumInterfaces)
return -EINVAL;
/* already claimed */
@@ -506,7 +509,7 @@
lock_kernel();
ret = -ENOENT;
- dev = inode->u.generic_ip;
+ dev = usb_get_dev(inode->u.generic_ip);
if (!dev) {
kfree(ps);
goto out;
@@ -540,13 +543,15 @@
lock_kernel();
list_del_init(&ps->list);
- if (ps->dev) {
+ if (connected(ps->dev)) {
for (i = 0; ps->ifclaimed && i < 8*sizeof(ps->ifclaimed); i++)
if (test_bit(i, &ps->ifclaimed))
releaseintf(ps, i);
+ destroy_all_async(ps);
}
unlock_kernel();
- destroy_all_async(ps);
+ usb_put_dev(ps->dev);
+ ps->dev = NULL;
kfree(ps);
return 0;
}
@@ -1015,7 +1020,7 @@
int ret;
add_wait_queue(&ps->wait, &wait);
- while (ps->dev) {
+ while (connected(ps->dev)) {
__set_current_state(TASK_INTERRUPTIBLE);
if ((as = async_getcompleted(ps)))
break;
@@ -1125,7 +1130,7 @@
}
}
- if (!ps->dev) {
+ if (!connected(ps->dev)) {
if (buf)
kfree(buf);
return -ENODEV;
@@ -1195,7 +1200,7 @@
if (!(file->f_mode & FMODE_WRITE))
return -EPERM;
down_read(&ps->devsem);
- if (!ps->dev) {
+ if (!connected(ps->dev)) {
up_read(&ps->devsem);
return -ENODEV;
}
@@ -1293,7 +1298,7 @@
poll_wait(file, &ps->wait, wait);
if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed))
mask |= POLLOUT | POLLWRNORM;
- if (!ps->dev)
+ if (!connected(ps->dev))
mask |= POLLERR | POLLHUP;
return mask;
}
diff -Nru a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
--- a/drivers/usb/core/inode.c Wed Apr 14 12:15:29 2004
+++ b/drivers/usb/core/inode.c Wed Apr 14 12:15:29 2004
@@ -717,9 +717,6 @@
while (!list_empty(&dev->filelist)) {
ds = list_entry(dev->filelist.next, struct dev_state, list);
list_del_init(&ds->list);
- down_write(&ds->devsem);
- ds->dev = NULL;
- up_write(&ds->devsem);
if (ds->discsignr) {
sinfo.si_signo = SIGPIPE;
sinfo.si_errno = EPIPE;
On Wednesday 14 April 2004 12:29, Duncan Sands wrote:
> Hi Greg, this is the first of a series of patches that replace the
> per-file semaphore ps->devsem with the per-device semaphore
> ps->dev->serialize. The role of devsem was to protect against
> device disconnection. This can be done equally well using
> ps->dev->serialize. On the other hand, ps->dev->serialize
> protects against configuration and other changes, and has
> already been introduced into usbfs in several places. Using
> just one semaphore simplifies the code and removes some
> remaining race conditions. It should also fix the oopses some
> people have been seeing. In this first patch, a reference is
> taken to the usb device as long as the usbfs file is open. That
> way we can use ps->dev->serialize for as long as ps exists.
Perhaps I should explain why something like this is needed.
Take a random subroutine in devio.c. I just made a random
choice (yes, really) and got proc_submiturb. It starts like
this:
if (copy_from_user(&uurb, arg, sizeof(uurb)))
return -EFAULT;
if (uurb.flags & ~(USBDEVFS_URB_ISO_ASAP|USBDEVFS_URB_SHORT_NOT_OK|
URB_NO_FSBR|URB_ZERO_PACKET))
return -EINVAL;
if (!uurb.buffer)
return -EINVAL;
if (uurb.signr != 0 && (uurb.signr < SIGRTMIN || uurb.signr > SIGRTMAX))
return -EINVAL;
if (!(uurb.type == USBDEVFS_URB_TYPE_CONTROL && (uurb.endpoint & ~USB_EN
DPOINT_DIR_MASK) == 0)) {
if ((intf = findintfep(ps->dev, uurb.endpoint)) < 0)
return intf;
if ((ret = checkintf(ps, intf)))
return ret;
}
Notice the calls to findintfep and to checkintf? The first scans through interfaces to
find which one we want, the second looks up the interface and claims it. What is to
stop the interfaces changing while we are doing that (due to a configuration change
for example)? Answer: nothing. Almost all subroutines in devio.c have calls like
these. The ps->dev->serialize semaphore needs to be taken to protect against
changes, but this simply isn't down right now. Normal USB drivers don't suffer from
this problem: they claim interfaces via their probe method (during which dev->serialize
is held for them by the core), and get disconnected by the core before any configuration
changes are performed. But usbfs is different: it goes around claiming interfaces all
over the place. For example, proc_submiturb like most other routines can be called
on an interface that hasn't been claimed - it will "helpfully" claim it (checkintf does it).
This means mapping an interface number to struct usb_interface - so we need to take
dev->serialize. If we removed this autoclaiming of interfaces (which would be a good
thing to do IMHO) then things would already be a lot better - but that would break
at least one user space program I know of, so surely others too. Anyway, checkintf
isn't the only problem here - devio.c is riddled with races of this kind. The simplest
solution is to systematically take ps->dev->serialize when entering the usbfs
routines, which is what my patches do. This should be regarded as a first step: it
gives correctness, but at the cost of a probable performance hit. In later steps we can
(1) turn dev->serialize into a rwsem
(2) push the acquisition of dev->serialize down to the lower levels as they are fixed up.
All the best,
Duncan.
> riddled with races of this kind. The simplest solution is to
> systematically take ps->dev->serialize when entering the usbfs routines,
> which is what my patches do. This should be regarded as a first step: it
What is the alternative?
> gives correctness, but at the cost of a probable performance hit. In later
> steps we can (1) turn dev->serialize into a rwsem
Rwsems are _slower_ in the normal case of no contention.
> (2) push the acquisition of dev->serialize down to the lower levels as they
> are fixed up.
Why?
Regards
Oliver
On Saturday 17 April 2004 22:33, Oliver Neukum wrote:
> > riddled with races of this kind. The simplest solution is to
> > systematically take ps->dev->serialize when entering the usbfs routines,
> > which is what my patches do. This should be regarded as a first step: it
>
> What is the alternative?
The alternative is to start at the lower levels and work up (while here I propose
starting at the top levels and working down): trying to lock small regions in many
places. I rejected this as too error prone (remember that usbfs is a bit of a mess).
Anyway, if done correctly the end result would be much the same as applying this
patch and doing step (2).
> > gives correctness, but at the cost of a probable performance hit. In
> > later steps we can (1) turn dev->serialize into a rwsem
>
> Rwsems are _slower_ in the normal case of no contention.
Right, but remember that dev->serialize is per device, not per interface. So if two
programs grab different interfaces of the same device using usbfs, or if multiple
threads in the same program beat on the same interface, then they could lose time
fighting for dev->serialize when in fact they could run in parallel. Personally I doubt
it matters much, but since most of usbfs only requires read access to the data structures
protected by dev->serialize, it seems logical to use a rwsem.
> > (2) push the acquisition of dev->serialize down to the lower levels as
> > they are fixed up.
>
> Why?
Efficiency. The main reason is that the copy to/from user calls are inside the locked region :)
As for the other places where the lock could be dropped, I guess measurement is required to
see if it gains anything.
Ciao,
Duncan.
> > > gives correctness, but at the cost of a probable performance hit. In
> > > later steps we can (1) turn dev->serialize into a rwsem
> >
> > Rwsems are _slower_ in the normal case of no contention.
>
> Right, but remember that dev->serialize is per device, not per interface.
> So if two programs grab different interfaces of the same device using
> usbfs, or if multiple threads in the same program beat on the same
> interface, then they could lose time fighting for dev->serialize when in
> fact they could run in parallel. Personally I doubt it matters much, but
> since most of usbfs only requires read access to the data structures
> protected by dev->serialize, it seems logical to use a rwsem.
Multiple interfaces are uncommon. Devices with several interfaces bound
to usbfs are uncommoner. Concurrent use is still uncommoner. You are
slowing the common case.
> > > (2) push the acquisition of dev->serialize down to the lower levels as
> > > they are fixed up.
> >
> > Why?
>
> Efficiency. The main reason is that the copy to/from user calls are inside
> the locked region :) As for the other places where the lock could be
> dropped, I guess measurement is required to see if it gains anything.
OK. I see. But IMHO usbfs is not written for speed anyway, so don't
worry too much.
Regards
Oliver
Hi Oliver,
> Multiple interfaces are uncommon. Devices with several interfaces bound
> to usbfs are uncommoner. Concurrent use is still uncommoner. You are
> slowing the common case.
The slowdown is probably negligeable though. The speedup may be big for
the rare cases where it matters (though I doubt anyone is ever going to care
one way or the other).
> > > > (2) push the acquisition of dev->serialize down to the lower levels
> > > > as they are fixed up.
> > >
> > > Why?
> >
> > Efficiency. The main reason is that the copy to/from user calls are
> > inside the locked region :) As for the other places where the lock could
> > be dropped, I guess measurement is required to see if it gains anything.
>
> OK. I see. But IMHO usbfs is not written for speed anyway, so don't
> worry too much.
I'm not worrying! It's more a matter of hygiene :)
Duncan.
On Sun, 18 Apr 2004, Duncan Sands wrote:
> > > gives correctness, but at the cost of a probable performance hit. In
> > > later steps we can (1) turn dev->serialize into a rwsem
> >
> > Rwsems are _slower_ in the normal case of no contention.
>
> Right, but remember that dev->serialize is per device, not per interface. So if two
> programs grab different interfaces of the same device using usbfs, or if multiple
> threads in the same program beat on the same interface, then they could lose time
> fighting for dev->serialize when in fact they could run in parallel. Personally I doubt
> it matters much, but since most of usbfs only requires read access to the data structures
> protected by dev->serialize, it seems logical to use a rwsem.
There was a lengthy discussion about this a few months ago. On the whole,
people felt that using an rwsem wasn't a good idea.
Personally, I think that contention for a single device will be very rare,
so we don't need to consider it and can leave things as they are.
Alan Stern
Heh ... I like this, getting rid of more of locks from usbcore.
Duncan Sands wrote:
> In later steps we can
> (1) turn dev->serialize into a rwsem
That doesn't bother me (unlike some folk!) but I do
agree it should wait until the rest shakes out, and
until we can observe benefits. Hardly any code paths
need complete mutual exclusion (writelock).
> (2) push the acquisition of dev->serialize down to the lower levels as they are fixed up.
Actually at least some of those lower levels are getting
fixed by pushing the lock acquisition up.
The reason usbfs needs to get involved in that stuff is
that it's going around the standard usb driver structure.
One of the other things that "usbfs2" should do is act a
lot more like a "normal" device driver, so it plays better
with more of the normal usbcore mechanisms.
- Dave
On Wed, Apr 14, 2004 at 12:29:26PM +0200, Duncan Sands wrote:
> Hi Greg, this is the first of a series of patches that replace the
> per-file semaphore ps->devsem with the per-device semaphore
> ps->dev->serialize. The role of devsem was to protect against
> device disconnection. This can be done equally well using
> ps->dev->serialize. On the other hand, ps->dev->serialize
> protects against configuration and other changes, and has
> already been introduced into usbfs in several places. Using
> just one semaphore simplifies the code and removes some
> remaining race conditions. It should also fix the oopses some
> people have been seeing. In this first patch, a reference is
> taken to the usb device as long as the usbfs file is open. That
> way we can use ps->dev->serialize for as long as ps exists.
Nice, I've applied all 9 patches here (with the updated patch 8
version). Feel free to send me an update for the warning issue you and
Oliver talked about if you want to.
thanks,
greg k-h
> Nice, I've applied all 9 patches here (with the updated patch 8
> version). Feel free to send me an update for the warning issue you and
> Oliver talked about if you want to.
Hi Greg, thanks for applying the patches. The following patch goes on top.
Hopefully it will make Oliver happy!
Ciao,
Duncan.
Be assertive.
devio.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
+++ b/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
@@ -350,8 +350,8 @@
* all pending I/O requests; 2.6 does that.
*/
- if (ifnum < 8*sizeof(ps->ifclaimed))
- clear_bit(ifnum, &ps->ifclaimed);
+ BUG_ON(ifnum >= 8*sizeof(ps->ifclaimed));
+ clear_bit(ifnum, &ps->ifclaimed);
usb_set_intfdata (intf, NULL);
/* force async requests to complete */
On Mon, Apr 26, 2004 at 04:05:17PM +0200, Duncan Sands wrote:
> diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> --- a/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
> +++ b/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
> @@ -350,8 +350,8 @@
> * all pending I/O requests; 2.6 does that.
> */
>
> - if (ifnum < 8*sizeof(ps->ifclaimed))
> - clear_bit(ifnum, &ps->ifclaimed);
> + BUG_ON(ifnum >= 8*sizeof(ps->ifclaimed));
I've changed that to a WARN_ON(). Yeah, writing over memory is bad, but
oopsing is worse. Let's be a bit nicer than that.
thanks,
greg k-h
Am Dienstag, 27. April 2004 00:14 schrieb Greg KH:
> On Mon, Apr 26, 2004 at 04:05:17PM +0200, Duncan Sands wrote:
> > diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > --- a/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
> > +++ b/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
> > @@ -350,8 +350,8 @@
> > * all pending I/O requests; 2.6 does that.
> > */
> >
> > - if (ifnum < 8*sizeof(ps->ifclaimed))
> > - clear_bit(ifnum, &ps->ifclaimed);
> > + BUG_ON(ifnum >= 8*sizeof(ps->ifclaimed));
>
> I've changed that to a WARN_ON(). Yeah, writing over memory is bad, but
> oopsing is worse. Let's be a bit nicer than that.
You aren't nice that way. An oops has localised consequences. Scribbling
over memory can cause anything.
Regards
Oliver
On Tuesday 27 April 2004 10:58, Oliver Neukum wrote:
> Am Dienstag, 27. April 2004 00:14 schrieb Greg KH:
> > On Mon, Apr 26, 2004 at 04:05:17PM +0200, Duncan Sands wrote:
> > > diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > --- a/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
> > > +++ b/drivers/usb/core/devio.c Mon Apr 26 13:48:28 2004
> > > @@ -350,8 +350,8 @@
> > > * all pending I/O requests; 2.6 does that.
> > > */
> > >
> > > - if (ifnum < 8*sizeof(ps->ifclaimed))
> > > - clear_bit(ifnum, &ps->ifclaimed);
> > > + BUG_ON(ifnum >= 8*sizeof(ps->ifclaimed));
> >
> > I've changed that to a WARN_ON(). Yeah, writing over memory is bad, but
> > oopsing is worse. Let's be a bit nicer than that.
>
> You aren't nice that way. An oops has localised consequences. Scribbling
> over memory can cause anything.
Hi Greg, if won't accept a BUG_ON, how about the following?
--- usb-2.6/drivers/usb/core/devio.c.orig 2004-04-30 11:36:17.000000000 +0200
+++ usb-2.6/drivers/usb/core/devio.c 2004-04-30 12:01:37.000000000 +0200
@@ -350,8 +350,11 @@
* all pending I/O requests; 2.6 does that.
*/
- WARN_ON(ifnum >= 8*sizeof(ps->ifclaimed));
- clear_bit(ifnum, &ps->ifclaimed);
+ if (likely(ifnum < 8*sizeof(ps->ifclaimed)))
+ clear_bit(ifnum, &ps->ifclaimed);
+ else
+ warn("interface number %u out of range", ifnum);
+
usb_set_intfdata (intf, NULL);
/* force async requests to complete */