[PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[John Stultz]

In many cases, documentation around composite drivers suggest
setting the idVendor and other module params as follows:

$ insmod g_ffs.ko idVendor=<ID> iSerialNumber=<string>

However, this won't work if the driver is not compiled in as a
module, as the module_param permissions are S_IRUGO.

Thus this patch changes the composite module_param permissions
to S_IRUGO|S_IWUSR to allow the module_params to be set at
runtime via /sys/modules/<driver>/parameters/

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/usb/composite.h |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index b09c37e..22b1b02 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -399,24 +399,28 @@ struct usb_composite_overwrite {
 #define USB_GADGET_COMPOSITE_OPTIONS()					\
 	static struct usb_composite_overwrite coverwrite;		\
 									\
-	module_param_named(idVendor, coverwrite.idVendor, ushort, S_IRUGO); \
+	module_param_named(idVendor, coverwrite.idVendor, ushort,	\
+			S_IRUGO|S_IWUSR);				\
 	MODULE_PARM_DESC(idVendor, "USB Vendor ID");			\
 									\
-	module_param_named(idProduct, coverwrite.idProduct, ushort, S_IRUGO); \
+	module_param_named(idProduct, coverwrite.idProduct, ushort,	\
+			S_IRUGO|S_IWUSR);				\
 	MODULE_PARM_DESC(idProduct, "USB Product ID");			\
 									\
-	module_param_named(bcdDevice, coverwrite.bcdDevice, ushort, S_IRUGO); \
+	module_param_named(bcdDevice, coverwrite.bcdDevice, ushort,	\
+			S_IRUGO|S_IWUSR);				\
 	MODULE_PARM_DESC(bcdDevice, "USB Device version (BCD)");	\
 									\
 	module_param_named(iSerialNumber, coverwrite.serial_number, charp, \
-			S_IRUGO); \
+			S_IRUGO|S_IWUSR);				\
 	MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");		\
 									\
 	module_param_named(iManufacturer, coverwrite.manufacturer, charp, \
-			S_IRUGO); \
+			S_IRUGO|S_IWUSR);				\
 	MODULE_PARM_DESC(iManufacturer, "USB Manufacturer string");	\
 									\
-	module_param_named(iProduct, coverwrite.product, charp, S_IRUGO); \
+	module_param_named(iProduct, coverwrite.product, charp,		\
+			S_IRUGO|S_IWUSR);				\
 	MODULE_PARM_DESC(iProduct, "USB Product string")
 
 void usb_composite_overwrite_options(struct usb_composite_dev *cdev,
-- 
1.7.10.4

Re: [PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 3449 bytes --]

On Thu, Feb 21 2013, John Stultz wrote:
> In many cases, documentation around composite drivers suggest
> setting the idVendor and other module params as follows:
>
> $ insmod g_ffs.ko idVendor=<ID> iSerialNumber=<string>
>
> However, this won't work if the driver is not compiled in as a
> module, as the module_param permissions are S_IRUGO.
>
> Thus this patch changes the composite module_param permissions
> to S_IRUGO|S_IWUSR to allow the module_params to be set at
> runtime via /sys/modules/<driver>/parameters/

If the driver is not compiled as a module, setting those variables won't
work anyway.  Or am I missing something?

You can, however, pass them on Linux command line (with some prefix
which I can never remember).

If you want to configure things at run-time without having to compile
stuff as modules, you need to wait for the configfs based gadgets.

> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  include/linux/usb/composite.h |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index b09c37e..22b1b02 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -399,24 +399,28 @@ struct usb_composite_overwrite {
>  #define USB_GADGET_COMPOSITE_OPTIONS()					\
>  	static struct usb_composite_overwrite coverwrite;		\
>  									\
> -	module_param_named(idVendor, coverwrite.idVendor, ushort, S_IRUGO); \
> +	module_param_named(idVendor, coverwrite.idVendor, ushort,	\
> +			S_IRUGO|S_IWUSR);				\
>  	MODULE_PARM_DESC(idVendor, "USB Vendor ID");			\
>  									\
> -	module_param_named(idProduct, coverwrite.idProduct, ushort, S_IRUGO); \
> +	module_param_named(idProduct, coverwrite.idProduct, ushort,	\
> +			S_IRUGO|S_IWUSR);				\
>  	MODULE_PARM_DESC(idProduct, "USB Product ID");			\
>  									\
> -	module_param_named(bcdDevice, coverwrite.bcdDevice, ushort, S_IRUGO); \
> +	module_param_named(bcdDevice, coverwrite.bcdDevice, ushort,	\
> +			S_IRUGO|S_IWUSR);				\
>  	MODULE_PARM_DESC(bcdDevice, "USB Device version (BCD)");	\
>  									\
>  	module_param_named(iSerialNumber, coverwrite.serial_number, charp, \
> -			S_IRUGO); \
> +			S_IRUGO|S_IWUSR);				\
>  	MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");		\
>  									\
>  	module_param_named(iManufacturer, coverwrite.manufacturer, charp, \
> -			S_IRUGO); \
> +			S_IRUGO|S_IWUSR);				\
>  	MODULE_PARM_DESC(iManufacturer, "USB Manufacturer string");	\
>  									\
> -	module_param_named(iProduct, coverwrite.product, charp, S_IRUGO); \
> +	module_param_named(iProduct, coverwrite.product, charp,		\
> +			S_IRUGO|S_IWUSR);				\
>  	MODULE_PARM_DESC(iProduct, "USB Product string")
>  
>  void usb_composite_overwrite_options(struct usb_composite_dev *cdev,
> -- 
> 1.7.10.4
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[John Stultz]

On 02/21/2013 02:52 PM, Michal Nazarewicz wrote:
> On Thu, Feb 21 2013, John Stultz wrote:
>> In many cases, documentation around composite drivers suggest
>> setting the idVendor and other module params as follows:
>>
>> $ insmod g_ffs.ko idVendor=<ID> iSerialNumber=<string>
>>
>> However, this won't work if the driver is not compiled in as a
>> module, as the module_param permissions are S_IRUGO.
>>
>> Thus this patch changes the composite module_param permissions
>> to S_IRUGO|S_IWUSR to allow the module_params to be set at
>> runtime via /sys/modules/<driver>/parameters/
> If the driver is not compiled as a module, setting those variables won't
> work anyway.  Or am I missing something?

Huh. It worked in my testing. But maybe that's only the first time its 
set? I'll play around with it some more, but yea, on further thought, 
without unloading the module those values probably shouldn't change. 
Sorry for the confusion on my part here.

> You can, however, pass them on Linux command line (with some prefix
> which I can never remember).
That's right. Yea. g_ffs.idVendor and g_ffs.idProduct works for me. 
Thanks for the reminder!

> If you want to configure things at run-time without having to compile
> stuff as modules, you need to wait for the configfs based gadgets.

Heh. I thought I was just sorting things out between the out-of-tree 
android composite, ccg in staging, and functionfs. And now there's 
*another*?

Any details on configfs based gadget? Is there a git tree somewhere?

thanks
-john

Re: [PATCH] [RFC] usb: gadget: composite: Allow idVendor and other module_params to be writable

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

> On 02/21/2013 02:52 PM, Michal Nazarewicz wrote:
>> If the driver is not compiled as a module, setting those variables
>> won't work anyway.  Or am I missing something?

On Fri, Feb 22 2013, John Stultz wrote:
> Huh. It worked in my testing. But maybe that's only the first time its 
> set? I'll play around with it some more, but yea, on further thought, 
> without unloading the module those values probably shouldn't change. 
> Sorry for the confusion on my part here.

If you are using g_ffs than yes, it's a bit different, since the module
is initialised only after the user space daemon is active, but for all
other gadgets, setting those won't work.

>> If you want to configure things at run-time without having to compile
>> stuff as modules, you need to wait for the configfs based gadgets.
>
> Heh. I thought I was just sorting things out between the out-of-tree 
> android composite, ccg in staging, and functionfs. And now there's 
> *another*?
>
> Any details on configfs based gadget? Is there a git tree somewhere?

There are patches from Andrzej Pietrasiewicz and Sebastian Andrzej
Siewior on the list.  I'm a bit lost in the fate of them and all the
namings.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]