[linux-usb-devel] [PATCH 1/9] USB usbfs: take a reference to the usb device

[linux-usb-devel] [PATCH 1/9] USB usbfs: take a reference to the usb device

Post by Duncan San » Mon, 19 Apr 2004 05:30:26


n Wednesday 14 April 2004 12:29, Duncan Sands wrote:

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 
 
 

[linux-usb-devel] [PATCH 1/9] USB usbfs: take a reference to the usb device

Post by Oliver Neu » Mon, 19 Apr 2004 05:40:18


What is the alternative?


Rwsems are _slower_ in the normal case of no contention.


Why?

Regards
Oliver

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/

 
 
 

[linux-usb-devel] [PATCH 1/9] USB usbfs: take a reference to the usb device

Post by Duncan San » Mon, 19 Apr 2004 18:40:12


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).


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.


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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/
 
 
 

[linux-usb-devel] [PATCH 1/9] USB usbfs: take a reference to the usb device

Post by Oliver Neu » Mon, 19 Apr 2004 23:10:10


Multiple interfaces are uncommon. Devices with several interfaces bound
to usbfs are uncommoner. Concurrent use is still uncommoner. You are
slowing the common case.


OK. I see. But IMHO usbfs is not written for speed anyway, so don't
worry too much.

Regards
Oliver

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/
 
 
 

[linux-usb-devel] [PATCH 1/9] USB usbfs: take a reference to the usb device

Post by Duncan San » Mon, 19 Apr 2004 23:10:12

Hi Oliver,


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).


I'm not worrying! It's more a matter of hygiene :)

Duncan.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to XXXX@XXXXX.COM
More majordomo info at http://www.yqcomputer.com/
Please read the FAQ at http://www.yqcomputer.com/