On Mon, 19 Apr 2004, David Brownell wrote: > Alan Stern wrote: > > > > Several issues of varying importance came up while I was working on this. > > > > (1) The locking in devices.c needs to be fixed. The USB subsystem > > rwsem should be held over a much larger part of the code. That should be > > pretty easy to do. I don't think it's necessary to use usb_get_dev or to > > lock usbdev->serialize, but someone should verify this. > > I'm thinking the plan of record for this one is that dev->serialize > should be held when accessing dev->children[] ... though that'll take > a while to phase in.
Yes. The change needed in devices.c will be quite simple; the changes to khubd may be a little more complicated -- I haven't looked at it yet. > > (2) The exact relationships among usbdev->state, usbdev->actconfig, > > and the actual state of the physical device aren't clear. Various error > > scenarios can leave ->state disagreeing with the device or ->state equal > > to USB_STATE_CONFIGURED even though ->actconfig is NULL. This needs more > > thought. > > Are you sure about CONFIGURED without an actconfig? It certainly > doesn't need to be that way; the current 2.6.6 code doesn't have > that problem, it updates state to ADDRESSED when clearing actconfig > if the original state is CONFIGURED. As you say, it doesn't have to be that way and it's easy to change. I just wasn't sure what was the right thing to do. If the request fails, we can simply set the state to ADDRESSED. In fact there's a line of code that already does that; it can just be moved up to before the message is sent. > > (3) I decided that it wasn't really necessary to make a deep copy of > > the altsetting and endpoint structures for the dynamic interfaces (those > > structures are read-only), so this patch just copies some pointers. > > Unfortunately this opens up a hole: A user program can pin an interface > > attribute file and use it to try reading an altsetting structure even > > after the device has been disconnected and the structure deallocated. I > > added a test to the attribute read method to prevent dereferencing a NULL > > pointer, but there remained the possibility of a race between using the > > pointer and setting it to NULL. So I added another semaphore to handle > > _that_. All this makes those attribute methods a lot bigger than they > > used to be. > > Seems like you resolved this to your satisfaction in as246c, using > the kref instead. That's right. It became clear that this was the only viable option after Greg added the usb_get_intf and usb_put_intf routines. The only remaining drawback (a pretty small one) is that the array of interface_cache pointers has a fixed length of 32, like the array of interface pointers. For most devices this will end up wasting over 100 bytes per config. I can live with that. (Or we could reduce the USB_MAXINTERFACES value.) > Other than testing, and (1) above which is deeper khubd surgery, > it looks like those issues are now gone. Any other issues? Not that I've come across. It works okay in light testing and it does avoid certain Badness messages the current kernel can get when interfaces are reused. > I skimmed as246c and didn't notice any particular issues as I > skimmed. There were a few whitespace changes bulking it up, but > those weren't going to make trouble! Just more comments and kerneldoc. Actually as246b had a rather large whitespace change (reduced indentation level) that I decided not to include in as246c. Alan Stern ------------------------------------------------------- This SF.Net email is sponsored by: IBM Linux Tutorials Free Linux tutorial presented by Daniel Robbins, President and CEO of GenToo technologies. Learn everything from fundamentals to system administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
