The iManufatcurer, iProduct and iSerialNumber composite module
parameters were only used when the gadget driver registers
strings for manufacturer, product and serial number. If the
gadget never bothered to set corresponding fields in USB device
descriptors those module parameters are ignored.
This commit makes the parameters work even if the strings ID
have not been assigned. It also changes the way IDs are
overridden -- what IDs are overridden is now saved in
usb_composite_dev structure -- which makes it unnecessary to
modify the string tables the way previous code did.
The commit also adds a iProduct and iManufatcurer fields to the
usb_composite_device structure. If they are set, appropriate
strings are reserved and added to device descriptor. This makes
it unnecessary for gadget drivers to maintain code for setting
those. If iProduct is not set it defaults to
usb_composite_device::name; if iManufatcurer is not set
a default "<system> <release> with <gadget-name>" is used.
The last thing is that if needs_serial field of
usb_composite_device is set and user failed to provided
iSerialNumber parameter a warning is issued.
Signed-off-by: Michal Nazarewicz <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
---
Hope this time everyone will be happy. :)
Delta to the previous version:
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index ae66889..38f7982 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -645,11 +645,12 @@ static int get_string(struct usb_composite_dev *cdev,
> * check if the string has not been overridden.
> */
> if (cdev->manufacturer_override == id)
> - str = iManufacturer ?: composite_manufacturer;
> + str = iManufacturer ?: composite->iManufacturer ?:
> + composite_manufacturer;
> else if (cdev->product_override == id)
> str = iProduct ?: composite->iProduct;
> else if (cdev->serial_override == id)
> - str = iSerialNumber ?: "0123456789AB";
> + str = iSerialNumber;
> else
> str = NULL;
> if (str) {
> @@ -1116,7 +1117,7 @@ static int composite_bind(struct usb_gadget *gadget)
>
> /* stirng overrides */
> if (iManufacturer || !cdev->desc.iManufacturer) {
> - if (!cdev->desc.iManufacturer &&
> + if (!iManufacturer && !composite->iManufacturer &&
> !*composite_manufacturer)
> snprintf(composite_manufacturer,
> sizeof composite_manufacturer,
> @@ -1133,14 +1134,15 @@ static int composite_bind(struct usb_gadget *gadget)
> cdev->product_override =
> override_id(cdev, &cdev->desc.iProduct);
>
> - if (iSerialNumber ||
> - (composite->needs_serial && !cdev->desc.iSerialNumber)) {
> - if (!iSerialNumber)
> - WARNING(cdev, "userspace failed to provide iSerialNumber, using fake one\n");
> + if (iSerialNumber)
> cdev->serial_override =
> override_id(cdev, &cdev->desc.iSerialNumber);
> - }
>
> + /* has userspace failed to provide a serial number? */
> + if (composite->needs_serial && !cdev->desc.iSerialNumber)
> + WARNING(cdev, "userspace failed to provide iSerialNumber\n");
> +
> + /* finish up */
> status = device_create_file(&gadget->dev, &dev_attr_suspended);
> if (status)
> goto fail;
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index c7bcc48..9eaadce 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -239,13 +239,15 @@ int usb_add_config(struct usb_composite_dev *,
> * @name: For diagnostics, identifies the driver.
> * @iProduct: Used as iProduct override if @dev->iProduct is not set.
> * If NULL value of @name is taken.
> + * @iProduct: Used as iManufacturer override if @dev->iManufacturer is
> + * not set. If NULL value a default "<system> <release> with <udc>"
> + * will be used.
> * @dev: Template descriptor for the device, including default device
> * identifiers.
> * @strings: tables of strings, keyed by identifiers assigned during bind()
> * and language IDs provided in control requests
> * @needs_serial: set to 1 if the gadget needs userspace to provide
> - * a serial number. If one is not provided, warning will be printed
> - * and fake serial number provided.
> + * a serial number. If one is not provided, warning will be printed.
> * @bind: (REQUIRED) Used to allocate resources that are shared across the
> * whole device, such as string IDs, and add its configurations using
> * @usb_add_config(). This may fail by returning a negative errno
> @@ -271,6 +273,7 @@ int usb_add_config(struct usb_composite_dev *,
> struct usb_composite_driver {
> const char *name;
> const char *iProduct;
> + const char *iManufacturer;
> const struct usb_device_descriptor *dev;
> struct usb_gadget_strings **strings;
> unsigned needs_serial:1;
drivers/usb/gadget/composite.c | 95 ++++++++++++++++++++++++++-------------
include/linux/usb/composite.h | 13 +++++
2 files changed, 76 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index e483f80..38f7982 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -69,6 +69,8 @@ static char *iSerialNumber;
module_param(iSerialNumber, charp, 0);
MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");
+static char composite_manufacturer[50];
+
/*-------------------------------------------------------------------------*/
/**
@@ -599,6 +601,7 @@ static int get_string(struct usb_composite_dev *cdev,
struct usb_configuration *c;
struct usb_function *f;
int len;
+ const char *str;
/* Yes, not only is USB's I18N support probably more than most
* folk will ever care about ... also, it's all supported here.
@@ -638,9 +641,29 @@ static int get_string(struct usb_composite_dev *cdev,
return s->bLength;
}
- /* Otherwise, look up and return a specified string. String IDs
- * are device-scoped, so we look up each string table we're told
- * about. These lookups are infrequent; simpler-is-better here.
+ /* Otherwise, look up and return a specified string. First
+ * check if the string has not been overridden.
+ */
+ if (cdev->manufacturer_override == id)
+ str = iManufacturer ?: composite->iManufacturer ?:
+ composite_manufacturer;
+ else if (cdev->product_override == id)
+ str = iProduct ?: composite->iProduct;
+ else if (cdev->serial_override == id)
+ str = iSerialNumber;
+ else
+ str = NULL;
+ if (str) {
+ struct usb_gadget_strings strings = {
+ .language = language,
+ .strings = &(struct usb_string) { 0xff, str }
+ };
+ return usb_gadget_get_string(&strings, 0xff, buf);
+ }
+
+ /* String IDs are device-scoped, so we look up each string
+ * table we're told about. These lookups are infrequent;
+ * simpler-is-better here.
*/
if (composite->strings) {
len = lookup_string(composite->strings, buf, language, id);
@@ -1025,26 +1048,17 @@ composite_unbind(struct usb_gadget *gadget)
composite = NULL;
}
-static void
-string_override_one(struct usb_gadget_strings *tab, u8 id, const char *s)
+static u8 override_id(struct usb_composite_dev *cdev, u8 *desc)
{
- struct usb_string *str = tab->strings;
-
- for (str = tab->strings; str->s; str++) {
- if (str->id == id) {
- str->s = s;
- return;
- }
+ if (!*desc) {
+ int ret = usb_string_id(cdev);
+ if (unlikely(ret < 0))
+ WARNING(cdev, "failed to override string ID\n");
+ else
+ *desc = ret;
}
-}
-static void
-string_override(struct usb_gadget_strings **tab, u8 id, const char *s)
-{
- while (*tab) {
- string_override_one(*tab, id, s);
- tab++;
- }
+ return *desc;
}
static int composite_bind(struct usb_gadget *gadget)
@@ -1101,19 +1115,34 @@ static int composite_bind(struct usb_gadget *gadget)
cdev->desc = *composite->dev;
cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket;
- /* strings can't be assigned before bind() allocates the
- * releavnt identifiers
- */
- if (cdev->desc.iManufacturer && iManufacturer)
- string_override(composite->strings,
- cdev->desc.iManufacturer, iManufacturer);
- if (cdev->desc.iProduct && iProduct)
- string_override(composite->strings,
- cdev->desc.iProduct, iProduct);
- if (cdev->desc.iSerialNumber && iSerialNumber)
- string_override(composite->strings,
- cdev->desc.iSerialNumber, iSerialNumber);
+ /* stirng overrides */
+ if (iManufacturer || !cdev->desc.iManufacturer) {
+ if (!iManufacturer && !composite->iManufacturer &&
+ !*composite_manufacturer)
+ snprintf(composite_manufacturer,
+ sizeof composite_manufacturer,
+ "%s %s with %s",
+ init_utsname()->sysname,
+ init_utsname()->release,
+ gadget->name);
+
+ cdev->manufacturer_override =
+ override_id(cdev, &cdev->desc.iManufacturer);
+ }
+
+ if (iProduct || (!cdev->desc.iProduct && composite->iProduct))
+ cdev->product_override =
+ override_id(cdev, &cdev->desc.iProduct);
+
+ if (iSerialNumber)
+ cdev->serial_override =
+ override_id(cdev, &cdev->desc.iSerialNumber);
+
+ /* has userspace failed to provide a serial number? */
+ if (composite->needs_serial && !cdev->desc.iSerialNumber)
+ WARNING(cdev, "userspace failed to provide iSerialNumber\n");
+ /* finish up */
status = device_create_file(&gadget->dev, &dev_attr_suspended);
if (status)
goto fail;
@@ -1211,6 +1240,8 @@ int usb_composite_register(struct usb_composite_driver *driver)
if (!driver || !driver->dev || !driver->bind || composite)
return -EINVAL;
+ if (!driver->iProduct)
+ driver->iProduct = driver->name;
if (!driver->name)
driver->name = "composite";
composite_driver.function = (char *) driver->name;
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 890bc14..9eaadce 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -237,10 +237,17 @@ int usb_add_config(struct usb_composite_dev *,
/**
* struct usb_composite_driver - groups configurations into a gadget
* @name: For diagnostics, identifies the driver.
+ * @iProduct: Used as iProduct override if @dev->iProduct is not set.
+ * If NULL value of @name is taken.
+ * @iManufacturer: Used as iManufacturer override if @dev->iManufacturer is
+ * not set. If NULL value a default "<system> <release> with <udc>"
+ * will be used.
* @dev: Template descriptor for the device, including default device
* identifiers.
* @strings: tables of strings, keyed by identifiers assigned during bind()
* and language IDs provided in control requests
+ * @needs_serial: set to 1 if the gadget needs userspace to provide
+ * a serial number. If one is not provided, warning will be printed.
* @bind: (REQUIRED) Used to allocate resources that are shared across the
* whole device, such as string IDs, and add its configurations using
* @usb_add_config(). This may fail by returning a negative errno
@@ -265,8 +272,11 @@ int usb_add_config(struct usb_composite_dev *,
*/
struct usb_composite_driver {
const char *name;
+ const char *iProduct;
+ const char *iManufacturer;
const struct usb_device_descriptor *dev;
struct usb_gadget_strings **strings;
+ unsigned needs_serial:1;
/* REVISIT: bind() functions can be marked __init, which
* makes trouble for section mismatch analysis. See if
@@ -333,6 +343,9 @@ struct usb_composite_dev {
struct list_head configs;
struct usb_composite_driver *driver;
u8 next_string_id;
+ u8 manufacturer_override;
+ u8 product_override;
+ u8 serial_override;
/* the gadget driver won't enable the data pullup
* while the deactivation count is nonzero.
--
1.7.1
This patch FunctionFS, Mass Storage and Multifunction gadgets
use the new features of composite framework. Because it
handles default strings there is no longer the need for the
gadgets drivers to handle many of the strings.
This also adds the "needs_serial" to Mass Storage Gadget and
Multifunction Composite Gadget which makes composite issue
a warning if user space has not provided iSerialNumber
parameter.
Signed-off-by: Michal Nazarewicz <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
---
drivers/usb/gadget/g_ffs.c | 47 +++-------------------
drivers/usb/gadget/mass_storage.c | 78 +++++--------------------------------
drivers/usb/gadget/multi.c | 19 +--------
3 files changed, 19 insertions(+), 125 deletions(-)
diff --git a/drivers/usb/gadget/g_ffs.c b/drivers/usb/gadget/g_ffs.c
index a9474f8..b7287ba 100644
--- a/drivers/usb/gadget/g_ffs.c
+++ b/drivers/usb/gadget/g_ffs.c
@@ -53,9 +53,6 @@ MODULE_AUTHOR("Michal Nazarewicz");
MODULE_LICENSE("GPL");
-static unsigned short gfs_vendor_id = 0x0525; /* XXX NetChip */
-static unsigned short gfs_product_id = 0xa4ac; /* XXX */
-
static struct usb_device_descriptor gfs_dev_desc = {
.bLength = sizeof gfs_dev_desc,
.bDescriptorType = USB_DT_DEVICE,
@@ -63,14 +60,8 @@ static struct usb_device_descriptor gfs_dev_desc = {
.bcdUSB = cpu_to_le16(0x0200),
.bDeviceClass = USB_CLASS_PER_INTERFACE,
- /* Vendor and product id can be overridden by module parameters. */
- /* .idVendor = cpu_to_le16(gfs_vendor_id), */
- /* .idProduct = cpu_to_le16(gfs_product_id), */
- /* .bcdDevice = f(hardware) */
- /* .iManufacturer = DYNAMIC */
- /* .iProduct = DYNAMIC */
- /* NO SERIAL NUMBER */
- .bNumConfigurations = 1,
+ .idVendor = cpu_to_le16(0x0525),
+ .idProduct = cpu_to_le16(0xa4ac),
};
#define GFS_MODULE_PARAM_DESC(name, field) \
@@ -82,10 +73,6 @@ module_param_named(usb_subclass, gfs_dev_desc.bDeviceSubClass, byte, 0644);
GFS_MODULE_PARAM_DESC(usb_subclass, bDeviceSubClass);
module_param_named(usb_protocol, gfs_dev_desc.bDeviceProtocol, byte, 0644);
GFS_MODULE_PARAM_DESC(usb_protocol, bDeviceProtocol);
-module_param_named(usb_vendor, gfs_vendor_id, ushort, 0644);
-GFS_MODULE_PARAM_DESC(usb_vendor, idVendor);
-module_param_named(usb_product, gfs_product_id, ushort, 0644);
-GFS_MODULE_PARAM_DESC(usb_product, idProduct);
@@ -105,19 +92,7 @@ static const struct usb_descriptor_header *gfs_otg_desc[] = {
/* string IDs are assigned dynamically */
-enum {
- GFS_STRING_MANUFACTURER_IDX,
- GFS_STRING_PRODUCT_IDX,
- GFS_STRING_FIRST_CONFIG_IDX,
-};
-
-static char gfs_manufacturer[50];
-static const char gfs_driver_desc[] = DRIVER_DESC;
-static const char gfs_short_name[] = DRIVER_NAME;
-
static struct usb_string gfs_strings[] = {
- [GFS_STRING_MANUFACTURER_IDX].s = gfs_manufacturer,
- [GFS_STRING_PRODUCT_IDX].s = gfs_driver_desc,
#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
{ .s = "FunctionFS + RNDIS" },
#endif
@@ -168,11 +143,12 @@ static int gfs_unbind(struct usb_composite_dev *cdev);
static int gfs_do_config(struct usb_configuration *c);
static struct usb_composite_driver gfs_driver = {
- .name = gfs_short_name,
+ .name = DRIVER_NAME,
.dev = &gfs_dev_desc,
.strings = gfs_dev_strings,
.bind = gfs_bind,
.unbind = gfs_unbind,
+ .iProduct = DRIVER_DESC,
};
@@ -245,20 +221,10 @@ static int gfs_bind(struct usb_composite_dev *cdev)
if (unlikely(ret < 0))
goto error_quick;
- gfs_dev_desc.idVendor = cpu_to_le16(gfs_vendor_id);
- gfs_dev_desc.idProduct = cpu_to_le16(gfs_product_id);
-
- snprintf(gfs_manufacturer, sizeof gfs_manufacturer, "%s %s with %s",
- init_utsname()->sysname, init_utsname()->release,
- cdev->gadget->name);
-
ret = usb_string_ids_tab(cdev, gfs_strings);
if (unlikely(ret < 0))
goto error;
- gfs_dev_desc.iManufacturer = gfs_strings[GFS_STRING_MANUFACTURER_IDX].id;
- gfs_dev_desc.iProduct = gfs_strings[GFS_STRING_PRODUCT_IDX].id;
-
ret = functionfs_bind(gfs_ffs_data, cdev);
if (unlikely(ret < 0))
goto error;
@@ -266,9 +232,8 @@ static int gfs_bind(struct usb_composite_dev *cdev)
for (i = 0; i < ARRAY_SIZE(gfs_configurations); ++i) {
struct gfs_configuration *c = gfs_configurations + i;
- ret = GFS_STRING_FIRST_CONFIG_IDX + i;
- c->c.label = gfs_strings[ret].s;
- c->c.iConfiguration = gfs_strings[ret].id;
+ c->c.label = gfs_strings[i].s;
+ c->c.iConfiguration = gfs_strings[i].id;
c->c.bind = gfs_do_config;
c->c.bConfigurationValue = 1 + i;
c->c.bmAttributes = USB_CONFIG_ATT_SELFPOWER;
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index 585f255..0354d69 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -75,10 +75,6 @@ static struct usb_device_descriptor msg_device_desc = {
/* Vendor and product id can be overridden by module parameters. */
.idVendor = cpu_to_le16(FSG_VENDOR_ID),
.idProduct = cpu_to_le16(FSG_PRODUCT_ID),
- /* .bcdDevice = f(hardware) */
- /* .iManufacturer = DYNAMIC */
- /* .iProduct = DYNAMIC */
- /* NO SERIAL NUMBER */
.bNumConfigurations = 1,
};
@@ -86,7 +82,8 @@ static struct usb_otg_descriptor otg_descriptor = {
.bLength = sizeof otg_descriptor,
.bDescriptorType = USB_DT_OTG,
- /* REVISIT SRP-only hardware is possible, although
+ /*
+ * REVISIT SRP-only hardware is possible, although
* it would not be called "OTG" ...
*/
.bmAttributes = USB_OTG_SRP | USB_OTG_HNP,
@@ -98,33 +95,6 @@ static const struct usb_descriptor_header *otg_desc[] = {
};
-/* string IDs are assigned dynamically */
-
-#define STRING_MANUFACTURER_IDX 0
-#define STRING_PRODUCT_IDX 1
-#define STRING_CONFIGURATION_IDX 2
-
-static char manufacturer[50];
-
-static struct usb_string strings_dev[] = {
- [STRING_MANUFACTURER_IDX].s = manufacturer,
- [STRING_PRODUCT_IDX].s = DRIVER_DESC,
- [STRING_CONFIGURATION_IDX].s = "Self Powered",
- { } /* end of list */
-};
-
-static struct usb_gadget_strings stringtab_dev = {
- .language = 0x0409, /* en-us */
- .strings = strings_dev,
-};
-
-static struct usb_gadget_strings *dev_strings[] = {
- &stringtab_dev,
- NULL,
-};
-
-
-
/****************************** Configurations ******************************/
static struct fsg_module_parameters mod_data = {
@@ -173,7 +143,6 @@ static struct usb_configuration msg_config_driver = {
.label = "Linux File-Backed Storage",
.bind = msg_do_config,
.bConfigurationValue = 1,
- /* .iConfiguration = DYNAMIC */
.bmAttributes = USB_CONFIG_ATT_SELFPOWER,
};
@@ -184,43 +153,15 @@ static struct usb_configuration msg_config_driver = {
static int __ref msg_bind(struct usb_composite_dev *cdev)
{
- struct usb_gadget *gadget = cdev->gadget;
int status;
- /* Allocate string descriptor numbers ... note that string
- * contents can be overridden by the composite_dev glue.
- */
-
- /* device descriptor strings: manufacturer, product */
- snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
- init_utsname()->sysname, init_utsname()->release,
- gadget->name);
- status = usb_string_id(cdev);
- if (status < 0)
- return status;
- strings_dev[STRING_MANUFACTURER_IDX].id = status;
- msg_device_desc.iManufacturer = status;
-
- status = usb_string_id(cdev);
- if (status < 0)
- return status;
- strings_dev[STRING_PRODUCT_IDX].id = status;
- msg_device_desc.iProduct = status;
-
- status = usb_string_id(cdev);
- if (status < 0)
- return status;
- strings_dev[STRING_CONFIGURATION_IDX].id = status;
- msg_config_driver.iConfiguration = status;
-
- /* register our second configuration */
status = usb_add_config(cdev, &msg_config_driver);
- if (status < 0)
- return status;
-
- dev_info(&gadget->dev, DRIVER_DESC ", version: " DRIVER_VERSION "\n");
- set_bit(0, &msg_registered);
- return 0;
+ if (status >= 0) {
+ dev_info(&cdev->gadget->dev,
+ DRIVER_DESC ", version: " DRIVER_VERSION "\n");
+ set_bit(0, &msg_registered);
+ }
+ return status;
}
@@ -230,8 +171,9 @@ static int __ref msg_bind(struct usb_composite_dev *cdev)
static struct usb_composite_driver msg_driver = {
.name = "g_mass_storage",
.dev = &msg_device_desc,
- .strings = dev_strings,
.bind = msg_bind,
+ .iProduct = DRIVER_DESC,
+ .needs_serial = 1,
};
MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index 795d762..8f7d512 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -121,8 +121,6 @@ static const struct usb_descriptor_header *otg_desc[] = {
enum {
- MULTI_STRING_MANUFACTURER_IDX,
- MULTI_STRING_PRODUCT_IDX,
#ifdef CONFIG_USB_G_MULTI_RNDIS
MULTI_STRING_RNDIS_CONFIG_IDX,
#endif
@@ -131,11 +129,7 @@ enum {
#endif
};
-static char manufacturer[50];
-
static struct usb_string strings_dev[] = {
- [MULTI_STRING_MANUFACTURER_IDX].s = manufacturer,
- [MULTI_STRING_PRODUCT_IDX].s = DRIVER_DESC,
#ifdef CONFIG_USB_G_MULTI_RNDIS
[MULTI_STRING_RNDIS_CONFIG_IDX].s = "Multifunction with RNDIS",
#endif
@@ -314,20 +308,11 @@ static int __ref multi_bind(struct usb_composite_dev *cdev)
device_desc.bcdDevice = cpu_to_le16(0x0300 | 0x0099);
}
- /* allocate string descriptor numbers */
- snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
- init_utsname()->sysname, init_utsname()->release,
- gadget->name);
-
+ /* allocate string IDs */
status = usb_string_ids_tab(cdev, strings_dev);
if (unlikely(status < 0))
goto fail2;
- device_desc.iManufacturer =
- strings_dev[MULTI_STRING_MANUFACTURER_IDX].id;
- device_desc.iProduct =
- strings_dev[MULTI_STRING_PRODUCT_IDX].id;
-
/* register configurations */
status = rndis_config_register(cdev);
if (unlikely(status < 0))
@@ -370,6 +355,8 @@ static struct usb_composite_driver multi_driver = {
.strings = dev_strings,
.bind = multi_bind,
.unbind = __exit_p(multi_unbind),
+ .iProduct = DRIVER_DESC,
+ .needs_serial = 1,
};
--
1.7.1
The "nofua" attribute is used in the File-Storage Gadget but the
functions handling it are defined in storage_common.c (which may be
not a bad thing if anyone will want to port tho "nofua" attribute to
mass storage function) which causes the following warnings:
drivers/usb/gadget/storage_common.c:718: \
warning: ‘fsg_show_nofua’ defined but not used
drivers/usb/gadget/storage_common.c:782: \
warning: ‘fsg_store_nofua’ defined but not used
Adding __maybe_unused fixes this issue.
Signed-off-by: Michal Nazarewicz <[email protected]>
Cc: Andy Shevchenko <[email protected]>
---
drivers/usb/gadget/storage_common.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 484acfb..3379cc3 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -715,8 +715,8 @@ static ssize_t fsg_show_ro(struct device *dev, struct device_attribute *attr,
: curlun->initially_ro);
}
-static ssize_t fsg_show_nofua(struct device *dev, struct device_attribute *attr,
- char *buf)
+static ssize_t __maybe_unused
+fsg_show_nofua(struct device *dev, struct device_attribute *attr, char *buf)
{
struct fsg_lun *curlun = fsg_lun_from_dev(dev);
@@ -779,9 +779,9 @@ static ssize_t fsg_store_ro(struct device *dev, struct device_attribute *attr,
return rc;
}
-static ssize_t fsg_store_nofua(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
+static ssize_t __maybe_unused
+fsg_store_nofua(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
struct fsg_lun *curlun = fsg_lun_from_dev(dev);
unsigned long nofua;
--
1.7.1
On Wed, Jul 28, 2010 at 3:13 PM, Michal Nazarewicz
<[email protected]> wrote:
> The "nofua" attribute is used in the File-Storage Gadget but the
> functions handling it are defined in storage_common.c (which may be
> not a bad thing if anyone will want to port tho "nofua" attribute to
> mass storage function) which causes the following warnings:
>
> drivers/usb/gadget/storage_common.c:718: \
> warning: ‘fsg_show_nofua’ defined but not used
> drivers/usb/gadget/storage_common.c:782: \
> warning: ‘fsg_store_nofua’ defined but not used
>
> Adding __maybe_unused fixes this issue.
It's a bit odd.
Why the warning happens for those two only?
There are two more parameters (four methods) to handle 'ro' and 'file'
which are done in the same way.
--
With Best Regards,
Andy Shevchenko
On Wed, 28 Jul 2010 14:25:59 +0200, Andy Shevchenko <[email protected]> wrote:
> On Wed, Jul 28, 2010 at 3:13 PM, Michal Nazarewicz
> <[email protected]> wrote:
>> The "nofua" attribute is used in the File-Storage Gadget but the
>> functions handling it are defined in storage_common.c (which may be
>> not a bad thing if anyone will want to port tho "nofua" attribute to
>> mass storage function) which causes the following warnings:
>>
>> drivers/usb/gadget/storage_common.c:718: \
>> warning: ‘fsg_show_nofua’ defined but not used
>> drivers/usb/gadget/storage_common.c:782: \
>> warning: ‘fsg_store_nofua’ defined but not used
>>
>> Adding __maybe_unused fixes this issue.
> It's a bit odd.
>
> Why the warning happens for those two only?
> There are two more parameters (four methods) to handle 'ro' and 'file'
> which are done in the same way.
Those are the only ones that are not used by the mass storage function.
The warning is issued when drivers using mass storage function are built.
File-Storage Gadget compiles fine.
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
2010/7/28 Michał Nazarewicz <[email protected]>:
>> Why the warning happens for those two only?
>> There are two more parameters (four methods) to handle 'ro' and 'file'
>> which are done in the same way.
>
> Those are the only ones that are not used by the mass storage function.
Now I got it.
I'm just wondering if it somehow useful to append similar parameter to
that function.
--
With Best Regards,
Andy Shevchenko
On Wed, 28 Jul 2010 15:42:33 +0200, Andy Shevchenko <[email protected]> wrote:
> 2010/7/28 Michał Nazarewicz <[email protected]>:
>>> Why the warning happens for those two only?
>>> There are two more parameters (four methods) to handle 'ro' and 'file'
>>> which are done in the same way.
>>
>> Those are the only ones that are not used by the mass storage function.
> Now I got it.
>
> I'm just wondering if it somehow useful to append similar parameter to
> that function.
I intend to add it later on when I'll have bit more time. For now just
a quick fix and an entry in my TODO. ;)
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--- On Wed, 7/28/10, Michal Nazarewicz <[email protected]> wrote:
> From: Michal Nazarewicz <[email protected]>
> Subject: [PATCHv5 2/3] USB: gadget: Use new composite features in some gadgets
NAK
Let's have one patch per gadget or function driver.
WITH a good explanation of what's changed in each.
What I see is a whole lot of random changes that
don't make ANY sense as one combined patch. Plus,
some look quite dubious...
> use the new features of composite framework.? Because
> it
> handles default strings there is no longer the need for
> the
> gadgets drivers to handle many of the strings.
The gadgets should always identify the same, and
thus handle their strings -- *unless* module params
are applied by users to override those defaults.
The reason to use the module params is because a
product wants to be a *different* gadget, which
must be possible but won't be routine. It
suffices to be different instances (serial #s)
in most routine usage.
>
> This also adds the "needs_serial" to Mass Storage
> Gadget and
When the mass-storage only patch gets sent, I'll
want to see Alan's ack.
> Multifunction Composite Gadget which makes composite issue
> a warning if user space has not provided iSerialNumber parameter.
>
>
>
> -static unsigned short gfs_vendor_id? ? =
> 0x0525;??? /* XXX NetChip */
> -static unsigned short gfs_product_id???=
> 0xa4ac;??? /* XXX */
Look -- you can't assign NetChip numbers!!! I
personally have a handful of them, and if I didn't
assign them, they CANNOT be used. That XXX
makes me think you (or someone) just randomly
picked a (broken) number. The original file
storage gadget had a correctly assigned number.
If you're using anything else, fix it; there are
numbers Greg can assign from Linux Foundation's
USB-IF membership.
Comments like "XXX" need explanations, too...
if the intent is to seem like the file storage
gadget, just say so.
>
> -??? /* Vendor and product id can be
> overridden by module parameters.? */
> -??? /* .idVendor???
> ??? = cpu_to_le16(gfs_vendor_id), */
> -??? /* .idProduct???
> ??? = cpu_to_le16(gfs_product_id), */
Again, screwey. Use the standard IDs unless
they get overridden. Don't require them to be
overridden ... they were assigned in the first
place to be safe. Module overrides are for folk
who put out their own products and want them to be
visibly different from the generic Linux ones,
and thus need to manage their own USB-IF vid/pid
codes
What you're doing is changing the whole model so
there's no longer a standard "this is what Linux
does by default" -- and *requiring* a lot more
pain and suffering from folk configuring gadgets.
PLUS ... almost ensuring they'll get it wrong. Not
anything vaguely like an improvement.
> -??? /* .bcdDevice???
> ??? = f(hardware) */
> -??? /* .iManufacturer??? =
> DYNAMIC */
> -??? /* .iProduct???
> ??? = DYNAMIC */
> -??? /* NO SERIAL NUMBER */
> -??? .bNumConfigurations??? =
> 1,
> +??? .idVendor???
> ??? = cpu_to_le16(0x0525),
> +??? .idProduct???
> ??? = cpu_to_le16(0xa4ac),
Same as above. You've broken ID management.
Use the (correct) symbols, not magic numbers.
Do you not understand how fundamental proper
management of vendor and product IDs is????
> --- On Wed, 7/28/10, Michal Nazarewicz <[email protected]> wrote:
>> use the new features of composite framework. Because it
>> handles default strings there is no longer the need for
>> the gadgets drivers to handle many of the strings.
On Fri, 30 Jul 2010 00:21:18 +0200, David Brownell <[email protected]> wrote:
> The gadgets should always identify the same, and
> thus handle their strings -- *unless* module params
> are applied by users to override those defaults.
This is not how many gadgets seem to work at least as
far as the iManufacturer is concerned which is usually built
at run-time as "Linux <version> with <gadget-name>".
The patch does not change the behaviour of the gadget since all
it does is take advantage of code put in composite.c in the 1/2
patch. Therefore, modified gadgets will still use the same
strings only code that handles string ID reservation is placed in
composite.c now (since it needs to be there to fix the module
parameters anyway).
>> -static unsigned short gfs_vendor_id = 0x0525; /* XXX NetChip */
>> -static unsigned short gfs_product_id = 0xa4ac; /* XXX */
> Look -- you can't assign NetChip numbers!!!
/me ashamed
Obviously, you're absolutely right. I left the XXX mark to remember
to clear the situation with the IDs but then completely forgot about
it after g_multi got pulled. (It's especially bad since there is a
conflict with hid.c).
So, to fix the situation, I need to ask Greg for the IDs?
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
On Fri, Jul 30, 2010 at 06:48:41PM +0200, Michał Nazarewicz wrote:
> >>-static unsigned short gfs_vendor_id = 0x0525; /* XXX NetChip */
> >>-static unsigned short gfs_product_id = 0xa4ac; /* XXX */
>
> >Look -- you can't assign NetChip numbers!!!
>
> /me ashamed
>
> Obviously, you're absolutely right. I left the XXX mark to remember
> to clear the situation with the IDs but then completely forgot about
> it after g_multi got pulled. (It's especially bad since there is a
> conflict with hid.c).
>
> So, to fix the situation, I need to ask Greg for the IDs?
Yes you do. We shouldn't be using _any_ netchip ids anymore now that we
have our own vendor id assigned to us.
thanks,
greg k-h
>
> We shouldn't be using _any_ netchip
> ids anymore now that we
> have our own vendor id assigned to us.
That's too extreme; the original handful of
NetChip IDs were (and are!!) correctly assigned,
and there's no reason to change them. In fact,
there's a lot of reason to continue using them
while config files and drivers expect to see
those specific IDs (as is reasonable). That's
to avoid breaking working configs...
For new IDs, yes: use Linux-foundation VIDS/PIDs.
- Dave
On Fri, Jul 30, 2010 at 11:57:35AM -0700, David Brownell wrote:
>
> >
> > We shouldn't be using _any_ netchip
> > ids anymore now that we
> > have our own vendor id assigned to us.
>
> That's too extreme; the original handful of
> NetChip IDs were (and are!!) correctly assigned,
> and there's no reason to change them. In fact,
> there's a lot of reason to continue using them
> while config files and drivers expect to see
> those specific IDs (as is reasonable). That's
> to avoid breaking working configs...
True, I was thinking that for the class-type devices, we could use the
linux foundation vid, as changing them shouldn't matter right?
Or does Windows really care about the vid/pid for a class device
somehow?
thanks,
greg k-h
> > That's too extreme; the original handful of
> > NetChip IDs were (and are!!) correctly assigned,
> > and there's no reason to change them. In fact,
> > there's a lot of reason to continue using them
> > while config files and drivers expect to see
> > those specific IDs (as is reasonable). That's
> > to avoid breaking working configs...
>
> True, I was thinking that for the class-type devices, we
> could use the
> linux foundation vid, as changing them shouldn't
> matter right?
There are INF files using NetChip IDs, so
changing them would break stuff.
Newer interfaces using new VIDS/PIDS? Fine.
>
> Or does Windows really care about the vid/pid for
> a class device somehow?
My understanding is that it does, since it really
doesn't have a good concept of "class" being the
generic thing. INF files embed VIDS/PIDS even
for drivers implementing class specs. MSFT was
being stupid again...
- Dave
On Fri, Jul 30, 2010 at 03:01:50PM -0700, David Brownell wrote:
>
> > > That's too extreme; the original handful of
> > > NetChip IDs were (and are!!) correctly assigned,
> > > and there's no reason to change them.? In fact,
> > > there's a lot of reason to continue using them
> > > while config files and drivers expect to see
> > > those specific IDs (as is reasonable).? That's
> > > to avoid breaking working configs...
> >
> > True, I was thinking that for the class-type devices, we
> > could use the
> > linux foundation vid, as changing them shouldn't
> > matter right?
>
> There are INF files using NetChip IDs, so
> changing them would break stuff.
>
> Newer interfaces using new VIDS/PIDS? Fine.
> >
> > Or does Windows really care about the vid/pid for
> > a class device somehow?
>
> My understanding is that it does, since it really
> doesn't have a good concept of "class" being the
> generic thing. INF files embed VIDS/PIDS even
> for drivers implementing class specs. MSFT was
> being stupid again...
Ok, fair enough.
thanks,
greg k-h
On Sat, Jul 31, 2010 at 6:01 AM, David Brownell <[email protected]> wrote:
>> Or does Windows really care about the vid/pid for
>> a class device somehow?
>
> My understanding is that it does, since it really
> doesn't have a good concept of "class" being the
> generic thing. ?INF files embed VIDS/PIDS even
> for drivers implementing class specs. ?MSFT was
> being stupid again...
>
It depends. For Mass Storage device and HID device,
no INF file is needed. So the VID/PID really does not
matter for them. For CDC-ACM or USB RNDIS,
an INF is needed.
--
Xiaofan
> Hope this time everyone will be happy. :)
NAK. You're still hard-wiring an un-managed
serial number. How many times do I have to say
I will never accept such a broken policy??
On Sun, 01 Aug 2010 21:05:33 +0200, David Brownell <[email protected]> wrote:
> NAK. You're still hard-wiring an un-managed serial number.
Uh? I'm not. Current code overrides the serial number only when the
iSerial module parameter is given:
+ if (iSerialNumber)
+ cdev->serial_override =
+ override_id(cdev, &cdev->desc.iSerialNumber);
and when host requests string which ID is saved as a overridden for
serial number the module parameter is taken.
+ else if (cdev->serial_override == id)
+ str = iSerialNumber;
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
> > NAK. You're still hard-wiring an un-managed
> serial number.
>
> Uh? I'm not.
Are too. THere's a line assigning such an
un-managed number. Get rid of it instead of
denying its existence (and then justifying it).
On Mon, 02 Aug 2010 13:26:05 +0200, David Brownell <[email protected]> wrote:
>>> NAK. You're still hard-wiring an un-managed
>>> serial number.
>> Uh? I'm not.
> Are too. There's a line assigning such an
> un-managed number. Get rid of it instead of
> denying its existence (and then justifying it).
Could you point that line to me, please? I fail to see it in this
patch... And it wasn't my intention to allow unmanaged serial number
with this patch, so if there is such code then I must have missed it
somehow.
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
> On Fri, Jul 30, 2010 at 06:48:41PM +0200, Michał Nazarewicz wrote:
>> So, to fix the situation, I need to ask Greg for the IDs?
On Fri, 30 Jul 2010 18:54:50 +0200, Greg KH <[email protected]> wrote:
> Yes you do.
Could I ask for two IDs then. One for the Multifunction Composite
Gadget (g_multi) and another for FunctionFS Gadget (g_ffs). Or is
there some kind of procedure for that?
Again, sorry for all the confusion. As soon as the gadgets will
get their own IDs I'll repost the patches with fixed IDs and David's
suggestions taken into consideration.
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
On Mon, Aug 02, 2010 at 07:14:08PM +0200, Michał Nazarewicz wrote:
> >On Fri, Jul 30, 2010 at 06:48:41PM +0200, Michał Nazarewicz wrote:
> >>So, to fix the situation, I need to ask Greg for the IDs?
>
> On Fri, 30 Jul 2010 18:54:50 +0200, Greg KH <[email protected]> wrote:
> >Yes you do.
>
> Could I ask for two IDs then. One for the Multifunction Composite
> Gadget (g_multi)
You now have device id 0104 reserved for this.
> and another for FunctionFS Gadget (g_ffs).
You now have device id 0105 reserved for this.
> Or is there some kind of procedure for that?
Heh, nope, you just have to ask and have code that needs it.
To keep everyone straight, here's the currently reserved device ids
managed by us:
1d6b Linux Foundation
0001 1.1 root hub
0002 2.0 root hub
0003 3.0 root hub
0100 PTP Gadget
0101 Audio Gadget
0102 EEM Gadget
0103 NCM (Ethernet) Gadget
0104 Multifunction Composite Gadget
0105 FunctionFS Gadget
thanks,
greg k-h
> On Mon, Aug 02, 2010 at 07:14:08PM +0200, Michał Nazarewicz wrote:
>> One for the Multifunction Composite Gadget (g_multi)
Greg KH <[email protected]> writes:
> You now have device id 0104 reserved for this.
>> and another for FunctionFS Gadget (g_ffs).
> You now have device id 0105 reserved for this.
Great! Thanks. I'll send patches changing the IDs tomorrow.
--
Best regards, _ _
.o. | Liege of Serenly Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +--<mina86-tlen.pl>--<jid:mina86-jabber.org>--ooO--(_)--Ooo--