Am Freitag, 11. Oktober 2002 01:35 schrieb Greg KH:
> On Fri, Oct 11, 2002 at 01:13:58AM +0200, Oliver Neukum wrote:
> > Hi,
> >
> > this version, though probably horribly broken, should have all features
> > I intended it to have. So could you have a look at this ?
>
> Um, if you know it's broken, do you still want comments?
I strongly suspect it to be broken.
And yes I want comments.
> > diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > --- a/drivers/usb/core/hub.c Fri Oct 11 01:08:39 2002
> > +++ b/drivers/usb/core/hub.c Fri Oct 11 01:08:39 2002
> > @@ -1236,7 +1236,7 @@
> > return 1;
> > }
> >
> > - ret = usb_set_configuration(dev, dev->actconfig->bConfigurationValue);
> > + ret = do_set_conf(dev, dev->actconfig->bConfigurationValue);
>
> Why call do_set_conf() here, instead of usb_set_configuration()?
So that no attaching of drivers to do interfaces happens prematurely.
> > diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > --- a/drivers/usb/core/message.c Fri Oct 11 01:08:39 2002
> > +++ b/drivers/usb/core/message.c Fri Oct 11 01:08:39 2002
> > @@ -838,6 +838,43 @@
> > }
> >
> > /**
> > + * do_set_conf - send the actual message changing configuration
>
> That name has got to change (along with the other do_* name. You're
> poluting the global namespace with this function, either make it start
> with usb_ or make it static (hint, I think you can do the latter.)
OK.
> > + * @dev: the device whose configuration is being updated
> > + * @configuration: the configuration being chosen.
> > + * Context: !in_interrupt ()
> > + */
> > +int do_set_conf(struct usb_device *dev, int configuration)
> > +{
> > + int r,i;
> > + struct usb_config_descriptor *cp;
> > +printk("Setting configuration.\n");
>
> dbg() works well for these kinds of things :)
Sorry. They were left in accidentally.
> > + for (i=0; i<dev->descriptor.bNumConfigurations; i++) {
> > + if (dev->config[i].bConfigurationValue == configuration) {
> > + cp = &dev->config[i];
> > + goto found;
> > + }
> > + }
> > +
> > + return -EINVAL;
> > +
> > +found:
> > +
> > + r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> > + USB_REQ_SET_CONFIGURATION, 0, configuration, 0,
> > + NULL, 0, HZ * USB_CTRL_SET_TIMEOUT);
> > + if (r)
> > + return r;
> > +
> > + dev->actconfig = cp;
> > + dev->toggle[0] = 0;
> > + dev->toggle[1] = 0;
> > + usb_set_maxpacket(dev);
> > +
> > + return 0;
>
> You have 3 ways to get out of this function, try to narrow that down to
> 1.
OK. I like goto ;-)
> > +
> > +}
> > +
> > +/**
> > * usb_set_configuration - Makes a particular device setting be current
> > * @dev: the device whose configuration is being updated
> > * @configuration: the configuration being chosen.
> > @@ -871,7 +908,7 @@
> > {
> > int i, ret;
> > struct usb_config_descriptor *cp = NULL;
> > -
> > +
> > for (i=0; i<dev->descriptor.bNumConfigurations; i++) {
> > if (dev->config[i].bConfigurationValue == configuration) {
> > cp = &dev->config[i];
> > @@ -883,17 +920,29 @@
> > return -EINVAL;
> > }
> >
> > - if ((ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> > - USB_REQ_SET_CONFIGURATION, 0, configuration, 0,
> > - NULL, 0, HZ * USB_CTRL_SET_TIMEOUT)) < 0)
> > - return ret;
> > + down(&dev->serialize);
> >
> > - dev->actconfig = cp;
> > - dev->toggle[0] = 0;
> > - dev->toggle[1] = 0;
> > - usb_set_maxpacket(dev);
> > + for (i = 0; i < USB_MAXCHILDREN; i++) {
> > + struct usb_device **child = dev->children + i;
> > + if (*child) {
> > + ret = -EBUSY;
> > + goto err; /* refuse if children were harmed */
> > + }
> > + }
> > +printk("Got lock in usb_set_configuration.\n");
> >
> > - return 0;
> > + usb_reap_interfaces(dev); /* get rid of all interfaces */
> > +
> > + if ((ret = do_set_conf(dev, configuration)))
> > + goto err;
> > +
> > + dev->desired_conf = configuration; /* for pm */
> > +
> > + ret = do_logical_register_dev(dev); /* reevaluate device */
>
> Nope, you just registered the main usb device a second time, you only
> want to register the interfaces, not the main device.
>
> Yes, it's a bit complicated the way USB and the driver model works
> together, if people want, I can try to document it better.
Oh, yes, please. I didn't really know what I was doing there.
> > +printk("Got driver lock.\n");
> > + down (&udev->serialize);
>
> Why grab the main device lock for the interface probe? I don't see how
> it protects you here, and it's starting to get a bit messy...
If this is not taken there's a race with usb_set_configuration().
Not only must we remove all drivers from the interfaces, but
no new attachments of drivers to interfaces of the device must
happen until the configuration has changed.
Regards
Oliver
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
