lkml.org 
[lkml]   [2010]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] USB: g_serial: Allow to override the default VID/PID
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


\
 
 \ /
  Last update: 2010-11-30 18:51    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site