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?
> 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()?
> 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.)
> + * @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 :)
> + 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.
> +
> +}
> +
> +/**
> * 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.
> +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...
thanks,
greg k-h
-------------------------------------------------------
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
