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

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

[Jef Driesen]

Override the default VID/PID if custom values are supplied through the
idVendor and idProduct kernel module parameters.

Signed-off-by: Jef Driesen <jefdriesen@telenet.be>
---
 drivers/usb/gadget/serial.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/serial.c b/drivers/usb/gadget/serial.c
index f46a609..77b410e 100644
--- a/drivers/usb/gadget/serial.c
+++ b/drivers/usb/gadget/serial.c
@@ -271,6 +271,11 @@ static int __init init(void)
 	}
 	strings_dev[STRING_DESCRIPTION_IDX].s = serial_config_driver.label;
 
+	if (idVendor)
+		device_desc.idVendor = idVendor;
+	if (idProduct)
+		device_desc.idProduct = idProduct;
+
 	return usb_composite_register(&gserial_driver);
 }
 module_init(init);
-- 
1.7.1

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

[Greg KH]

On Sat, Nov 20, 2010 at 12:32:34AM +0100, Jef Driesen wrote:
> Override the default VID/PID if custom values are supplied through the
> idVendor and idProduct kernel module parameters.
> 
> Signed-off-by: Jef Driesen <jefdriesen@telenet.be>

So this patch resolves the bug found in
1ab83238740ff1e1773d5c13ecac43c60cf4aec4 which showed up in .35-rc1,
right?

Robert, any objection to this as it was your patch that broke things?

thanks,

greg k-h
> ---
>  drivers/usb/gadget/serial.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/serial.c b/drivers/usb/gadget/serial.c
> index f46a609..77b410e 100644
> --- a/drivers/usb/gadget/serial.c
> +++ b/drivers/usb/gadget/serial.c
> @@ -271,6 +271,11 @@ static int __init init(void)
>  	}
>  	strings_dev[STRING_DESCRIPTION_IDX].s = serial_config_driver.label;
>  
> +	if (idVendor)
> +		device_desc.idVendor = idVendor;
> +	if (idProduct)
> +		device_desc.idProduct = idProduct;
> +
>  	return usb_composite_register(&gserial_driver);
>  }
>  module_init(init);
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jef Driesen]

On 20/11/10 01:12, Greg KH wrote:
> On Sat, Nov 20, 2010 at 12:32:34AM +0100, Jef Driesen wrote:
>> Override the default VID/PID if custom values are supplied through the
>> idVendor and idProduct kernel module parameters.
>>
>> Signed-off-by: Jef Driesen<jefdriesen@telenet.be>
>
> So this patch resolves the bug found in
> 1ab83238740ff1e1773d5c13ecac43c60cf4aec4 which showed up in .35-rc1,
> right?

Yes, although I'm really sure it is the best way to fix the problem. The way I 
understand this code is that before that commit, the 
idVendor/idProduct/bcdDevice module parameters where set after the bind(). Thus 
whatever was set as the default there got replaced with the values of the module 
parameters. Exactly what I would consider the correct behavior. After the 
commit, they get set before bind(), where they are changed back to the hardcoded 
values, which I think is wrong. My patch sets them back to the right values, but 
maybe it makes more sense to not set the default values in the first place (if 
there are already values in place of course). But I didn't knew how to 
accomplish that.

There might be similar problems for the other gadget drivers as well. I haven't 
checked that.

>> ---
>>   drivers/usb/gadget/serial.c |    5 +++++
>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/serial.c b/drivers/usb/gadget/serial.c
>> index f46a609..77b410e 100644
>> --- a/drivers/usb/gadget/serial.c
>> +++ b/drivers/usb/gadget/serial.c
>> @@ -271,6 +271,11 @@ static int __init init(void)
>>   	}
>>   	strings_dev[STRING_DESCRIPTION_IDX].s = serial_config_driver.label;
>>
>> +	if (idVendor)
>> +		device_desc.idVendor = idVendor;
>> +	if (idProduct)
>> +		device_desc.idProduct = idProduct;
>> +
>>   	return usb_composite_register(&gserial_driver);
>>   }
>>   module_init(init);
>> --
>> 1.7.1

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

[Robert Lukassen]

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.

Robert 

> -----Original Message-----
> From: Jef Driesen [mailto:jefdriesen@telenet.be] 
> Sent: Sunday, November 21, 2010 12:03 AM
> To: Greg KH
> Cc: Robert Lukassen; David Brownell; Greg Kroah-Hartman; 
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] USB: g_serial: Allow to override the 
> default VID/PID
> 
> On 20/11/10 01:12, Greg KH wrote:
> > On Sat, Nov 20, 2010 at 12:32:34AM +0100, Jef Driesen wrote:
> >> Override the default VID/PID if custom values are supplied through 
> >> the idVendor and idProduct kernel module parameters.
> >>
> >> Signed-off-by: Jef Driesen<jefdriesen@telenet.be>
> >
> > So this patch resolves the bug found in
> > 1ab83238740ff1e1773d5c13ecac43c60cf4aec4 which showed up in 
> .35-rc1, 
> > right?
> 
> Yes, although I'm really sure it is the best way to fix the 
> problem. The way I understand this code is that before that 
> commit, the idVendor/idProduct/bcdDevice module parameters 
> where set after the bind(). Thus whatever was set as the 
> default there got replaced with the values of the module 
> parameters. Exactly what I would consider the correct 
> behavior. After the commit, they get set before bind(), where 
> they are changed back to the hardcoded values, which I think 
> is wrong. My patch sets them back to the right values, but 
> maybe it makes more sense to not set the default values in 
> the first place (if there are already values in place of 
> course). But I didn't knew how to accomplish that.
> 
> There might be similar problems for the other gadget drivers 
> as well. I haven't checked that.
> 
> >> ---
> >>   drivers/usb/gadget/serial.c |    5 +++++
> >>   1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/serial.c 
> >> b/drivers/usb/gadget/serial.c index f46a609..77b410e 100644
> >> --- a/drivers/usb/gadget/serial.c
> >> +++ b/drivers/usb/gadget/serial.c
> >> @@ -271,6 +271,11 @@ static int __init init(void)
> >>   	}
> >>   	strings_dev[STRING_DESCRIPTION_IDX].s = 
> >> serial_config_driver.label;
> >>
> >> +	if (idVendor)
> >> +		device_desc.idVendor = idVendor;
> >> +	if (idProduct)
> >> +		device_desc.idProduct = idProduct;
> >> +
> >>   	return usb_composite_register(&gserial_driver);
> >>   }
> >>   module_init(init);
> >> --
> >> 1.7.1
> 

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

[Greg KH]

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

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

[Jef Driesen]

On 30/11/10 18:46, Greg KH wrote:
> 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.

If the original commit is reverted, the idVendor and idProduct module parameters 
can still be accessed through the global variables directly (like I did in my 
own pathc). Since access to those values appears to be the main goal of the 
patch, I think no functionality will be lost by reverting the commit.

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

[Robert Lukassen]

Hi,

Yes, providing access to those module parameters was the original goal. I agree that my patch should be reverted and I'll try to come up with a method of accessing these values without violating the information hiding principles too much. I don't like accessing globals.

Robert

> -----Original Message-----
> From: Jef Driesen [mailto:jefdriesen@telenet.be] 
> Sent: Friday, December 03, 2010 11:10 PM
> To: Greg KH
> Cc: Robert Lukassen; David Brownell; Greg Kroah-Hartman; 
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] USB: g_serial: Allow to override the 
> default VID/PID
> 
> On 30/11/10 18:46, Greg KH wrote:
> > 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.
> 
> If the original commit is reverted, the idVendor and 
> idProduct module parameters can still be accessed through the 
> global variables directly (like I did in my own pathc). Since 
> access to those values appears to be the main goal of the 
> patch, I think no functionality will be lost by reverting the commit.
> 


This e-mail message contains information which is confidential and may be privileged. It is intended for use by the addressee only. If you are not the intended addressee, we request that you notify the sender immediately and delete or destroy this e-mail message and any attachment(s), without copying, saving, forwarding, disclosing or using its contents in any other way. TomTom N.V., TomTom International BV or any other company belonging to the TomTom group of companies will not be liable for damage relating to the communication by e-mail of data, documents or any other information.

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

[Jef Driesen]

On 06/12/10 09:09, Robert Lukassen wrote:
> Yes, providing access to those module parameters was the original goal. I
> agree that my patch should be reverted and I'll try to come up with a method
> of accessing these values without violating the information hiding principles
> too much. I don't like accessing globals.

Do I have to take any action to make sure the original commit [1] gets reverted?

[1] 1ab83238740ff1e1773d5c13ecac43c60cf4aec4

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

[Greg KH]

On Wed, Dec 15, 2010 at 08:57:47AM +0100, Jef Driesen wrote:
> On 06/12/10 09:09, Robert Lukassen wrote:
> >Yes, providing access to those module parameters was the original goal. I
> >agree that my patch should be reverted and I'll try to come up with a method
> >of accessing these values without violating the information hiding principles
> >too much. I don't like accessing globals.
> 
> Do I have to take any action to make sure the original commit [1] gets reverted?
> 
> [1] 1ab83238740ff1e1773d5c13ecac43c60cf4aec4

It's now reverted.

thanks,

greg k-h