Re: [PATCH] USB: g_serial: Allow to override the default VID/PID

[Greg KH <greg@kroah.com>]

On Mon, Nov 22, 2010 at 11:20:55AM +0100, Robert Lukassen wrote:
> Hi,
> 
> I just revisited the patch I submitted and I can see that it needs 
> fixing. The intention of the patch was to provide the idVendor & 
> idProduct kernel module parameter overrides to the individual 
> functions at 'bind()' time. Prior to the patch, the overrides were 
> done after the bind, making it hard for individual functions to 
> determine the correct value for idVendor (some functions may need 
> to use that value, e.g. USB audio when it registers itself
> as an ALSA device; it would want to use the same Vendor id).
> 
> The patch makes sure that that happens, however, right after the bind()
> the patched values are lost due to the copying of the gadget specific
> device descriptor. Here's the snippet from 2.6.34:
> 
>  999        usb_gadget_set_selfpowered(gadget);
> 1000
> 
> 1001        /* interface and string IDs start at zero via kzalloc.
> 1002         * we force endpoints to start unassigned; few controller
> 1003         * drivers will zero ep->driver_data.
> 1004         */
> 1005        usb_ep_autoconfig_reset(cdev->gadget);
> 1006
> 1007        /* composite gadget needs to assign strings for whole device (like
> 1008         * serial number), register function drivers, potentially update
> 1009         * power state and consumption, etc
> 1010         */
> 1011        status = composite->bind(cdev);
> 1012        if (status < 0)
> 1013                goto fail;
> 1014
> 1015        cdev->desc = *composite->dev;
> 1016        cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket;
> 1017
> 1018        /* standardized runtime overrides for device ID data */
> 1019        if (idVendor)
> 1020                cdev->desc.idVendor = cpu_to_le16(idVendor);
> 1021        if (idProduct)
> 1022                cdev->desc.idProduct = cpu_to_le16(idProduct);
> 1023        if (bcdDevice)
> 1024                cdev->desc.bcdDevice = cpu_to_le16(bcdDevice);
> 1025
> 
> In line 1015, the composite device descriptor is copied to the cdev->desc
> field. Then the values get patched.
> 
> My patch just moved lines 1018-1024, so that the functions could access
> the patched values. The ill side effect is that these patched values
> are lost when the *composite->dev (== device descriptor) is copied 
> in line 1015.
> 
> It would seem that the patch should also have moved lines 1015-1016 in front 
> of the composite->bind(cdev) call.
> 
> It would require all composite gadgets (such as serial.c) and all functions
> (such as f_acm.c) to do any modifications to the device descriptor as passed 
> to them in the 'bind' call. For functions that is natural, as they should be
> unaware of the gadget that uses them. 
> 
> The composite gadgets such as 'serial' seem to deal with it differently.
> The serial.c gadget refers to the device descriptor as 'device_desc', and it
> is a static struct. It references it in its 'init' function (which is ok, it is
> prior to the registration point) and in the gs_bind() call. At that point it should
> probably not be using 'device_desc', but cdev->desc.
> 
> I think we can go one of two ways. I'd be happy to go over the composite gadgets and 
> change their use of their device descriptors, such that they use cdev->desc at
> bind() time. Combining that with the moving the copying of the *composite->dev
> to before calling 'composite->bind()' would make things consistent.
> 
> The other way is reverting my original patch. For those functions that need access
> to the overridden idVendor/idProduct code, we'll need to work out another
> (generic) solution.

So, do you want me to revert your original patch and wait for you to
come up with a "better" solution?

That seems like the correct thing to do at the moment.

thanks,

greg k-h