From: Michal Nazarewicz <[email protected]>
The usb_create_sysfs_intf_files() function always returned zero even
if it failed to create sysfs fails. Since this is a desired behaviour
there is no need to return return code at all. This commit changes
function's return type (form int) to void.
Signed-off-by: Michal Nazarewicz <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
---
drivers/usb/core/sysfs.c | 7 +++----
drivers/usb/core/usb.c | 3 +--
drivers/usb/core/usb.h | 2 +-
3 files changed, 5 insertions(+), 7 deletions(-)
The below patch has not been tested (not even compiled), just a quick scatch.
> On Thu, 24 Mar 2011, Sergey Senozhatsky wrote:
>> I just noticed that usb_create_sysfs_intf_files() ignores
>> device_create_file() return code and sets intf->sysfs_files_created
>> to 1, even if sysfs_add_file_mode() returned -ENOMEM (or later
>> sysfs_add_one() returned -EEXIST).
>>
>> Shouldn't we check retval for 0 before setting
>> intf->sysfs_files_created?
On Thu, 24 Mar 2011 15:55:08 +0100, Alan Stern <[email protected]> wrote:
> No. We want this routine to succeed even if the sysfs files can't be
> created. The interface string attribute is more or less optional.
>
> It would be okay to add a comment explaining this, so that other people
> don't make the same mistake (which has already happened -- you're not
> the first).
Can we also drop the return value completely then?
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 6781c36..e729480 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -842,11 +842,10 @@ const struct attribute_group *usb_interface_groups[] = {
NULL
};
-int usb_create_sysfs_intf_files(struct usb_interface *intf)
+void usb_create_sysfs_intf_files(struct usb_interface *intf)
{
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_host_interface *alt = intf->cur_altsetting;
- int retval;
if (intf->sysfs_files_created || intf->unregistering)
return 0;
@@ -855,9 +854,9 @@ int usb_create_sysfs_intf_files(struct usb_interface *intf)
!(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
alt->string = usb_cache_string(udev, alt->desc.iInterface);
if (alt->string)
- retval = device_create_file(&intf->dev, &dev_attr_interface);
+ /* We don't care if the function actually fails. */
+ device_create_file(&intf->dev, &dev_attr_interface);
intf->sysfs_files_created = 1;
- return 0;
}
void usb_remove_sysfs_intf_files(struct usb_interface *intf)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 079cb57..08b74f8 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -948,8 +948,7 @@ static int usb_bus_notify(struct notifier_block *nb, unsigned long action,
if (dev->type == &usb_device_type)
(void) usb_create_sysfs_dev_files(to_usb_device(dev));
else if (dev->type == &usb_if_device_type)
- (void) usb_create_sysfs_intf_files(
- to_usb_interface(dev));
+ usb_create_sysfs_intf_files(to_usb_interface(dev));
break;
case BUS_NOTIFY_DEL_DEVICE:
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index a9cf484..d51ab6a 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -4,7 +4,7 @@
extern int usb_create_sysfs_dev_files(struct usb_device *dev);
extern void usb_remove_sysfs_dev_files(struct usb_device *dev);
-extern int usb_create_sysfs_intf_files(struct usb_interface *intf);
+extern void usb_create_sysfs_intf_files(struct usb_interface *intf);
extern void usb_remove_sysfs_intf_files(struct usb_interface *intf);
extern int usb_create_ep_devs(struct device *parent,
struct usb_host_endpoint *endpoint,
--
1.7.3.1
On Thu, 24 Mar 2011, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <[email protected]>
>
> The usb_create_sysfs_intf_files() function always returned zero even
> if it failed to create sysfs fails. Since this is a desired behaviour
> there is no need to return return code at all. This commit changes
> function's return type (form int) to void.
>
> Signed-off-by: Michal Nazarewicz <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> ---
> drivers/usb/core/sysfs.c | 7 +++----
> drivers/usb/core/usb.c | 3 +--
> drivers/usb/core/usb.h | 2 +-
> 3 files changed, 5 insertions(+), 7 deletions(-)
>
> The below patch has not been tested (not even compiled), just a quick scatch.
If you had compiled it, you would have seen why it's not a good idea to
remove retval. :-)
On the other hand, changing the function's return type to void is okay.
I think the reason it returns int is historical; there used to be a
failure mode but it got removed.
Alan Stern
> On Thu, 24 Mar 2011, Michal Nazarewicz wrote:
>> The usb_create_sysfs_intf_files() function always returned zero even
>> if it failed to create sysfs fails. Since this is a desired behaviour
>> there is no need to return return code at all. This commit changes
>> function's return type (form int) to void.
[...]
>> The below patch has not been tested (not even compiled), just a quick
>> scatch.
On Thu, 24 Mar 2011 16:52:36 +0100, Alan Stern wrote:
> If you had compiled it, you would have seen why it's not a good idea to
> remove retval. :-)
I must be getting blind... I explicitly checked if device_create_file()
has __must_check attribute but somehow missed it.
> On the other hand, changing the function's return type to void is okay.
> I think the reason it returns int is historical; there used to be a
> failure mode but it got removed.
In that case I can prepare a v2 later today (unless Sergey feels like
doing that).
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--
From: Michal Nazarewicz <[email protected]>
The usb_create_sysfs_intf_files() function always returned zero even
if it failed to create sysfs fails. Since this is a desired behaviour
there is no need to return return code at all. This commit changes
function's return type (form int) to void.
Signed-off-by: Michal Nazarewicz <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
---
drivers/usb/core/sysfs.c | 13 +++++--------
drivers/usb/core/usb.c | 3 +--
drivers/usb/core/usb.h | 2 +-
3 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 6781c36..f5b7b1c 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -842,22 +842,19 @@ const struct attribute_group *usb_interface_groups[] = {
NULL
};
-int usb_create_sysfs_intf_files(struct usb_interface *intf)
+void usb_create_sysfs_intf_files(struct usb_interface *intf)
{
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_host_interface *alt = intf->cur_altsetting;
- int retval;
if (intf->sysfs_files_created || intf->unregistering)
- return 0;
+ return;
- if (alt->string == NULL &&
- !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
+ if (!alt->string && !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
alt->string = usb_cache_string(udev, alt->desc.iInterface);
- if (alt->string)
- retval = device_create_file(&intf->dev, &dev_attr_interface);
+ if (alt->string && device_create_file(&intf->dev, &dev_attr_interface))
+ /* We don't actually care if the function fails. */;
intf->sysfs_files_created = 1;
- return 0;
}
void usb_remove_sysfs_intf_files(struct usb_interface *intf)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 079cb57..08b74f8 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -948,8 +948,7 @@ static int usb_bus_notify(struct notifier_block *nb, unsigned long action,
if (dev->type == &usb_device_type)
(void) usb_create_sysfs_dev_files(to_usb_device(dev));
else if (dev->type == &usb_if_device_type)
- (void) usb_create_sysfs_intf_files(
- to_usb_interface(dev));
+ usb_create_sysfs_intf_files(to_usb_interface(dev));
break;
case BUS_NOTIFY_DEL_DEVICE:
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index a9cf484..d51ab6a 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -4,7 +4,7 @@
extern int usb_create_sysfs_dev_files(struct usb_device *dev);
extern void usb_remove_sysfs_dev_files(struct usb_device *dev);
-extern int usb_create_sysfs_intf_files(struct usb_interface *intf);
+extern void usb_create_sysfs_intf_files(struct usb_interface *intf);
extern void usb_remove_sysfs_intf_files(struct usb_interface *intf);
extern int usb_create_ep_devs(struct device *parent,
struct usb_host_endpoint *endpoint,
--
1.7.3.1
On Fri, 25 Mar 2011, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <[email protected]>
>
> The usb_create_sysfs_intf_files() function always returned zero even
> if it failed to create sysfs fails. Since this is a desired behaviour
> there is no need to return return code at all. This commit changes
> function's return type (form int) to void.
>
> Signed-off-by: Michal Nazarewicz <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> ---
> drivers/usb/core/sysfs.c | 13 +++++--------
> drivers/usb/core/usb.c | 3 +--
> drivers/usb/core/usb.h | 2 +-
> 3 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index 6781c36..f5b7b1c 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -842,22 +842,19 @@ const struct attribute_group *usb_interface_groups[] = {
> NULL
> };
>
> -int usb_create_sysfs_intf_files(struct usb_interface *intf)
> +void usb_create_sysfs_intf_files(struct usb_interface *intf)
> {
> struct usb_device *udev = interface_to_usbdev(intf);
> struct usb_host_interface *alt = intf->cur_altsetting;
> - int retval;
>
> if (intf->sysfs_files_created || intf->unregistering)
> - return 0;
> + return;
>
> - if (alt->string == NULL &&
> - !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
> + if (!alt->string && !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
> alt->string = usb_cache_string(udev, alt->desc.iInterface);
> - if (alt->string)
> - retval = device_create_file(&intf->dev, &dev_attr_interface);
> + if (alt->string && device_create_file(&intf->dev, &dev_attr_interface))
> + /* We don't actually care if the function fails. */;
My style preference is to put the empty statement before the comment.
It's unusual to have a comment appearing before a statement on the same
line. For example:
; /* We don't care if the function fails */
Alan Stern
On Fri, 25 Mar 2011, Michal Nazarewicz wrote:
> The usb_create_sysfs_intf_files() function always returned zero even
> if it failed to create sysfs fails. Since this is a desired behaviour
> there is no need to return return code at all. This commit changes
> function's return type (form int) to void.
>
> Signed-off-by: Michal Nazarewicz <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>
> ---
> drivers/usb/core/sysfs.c | 13 +++++--------
> drivers/usb/core/usb.c | 3 +--
> drivers/usb/core/usb.h | 2 +-
> 3 files changed, 7 insertions(+), 11 deletions(-)
>
> On Fri, 25 Mar 2011 16:33:12 +0100, Alan Stern <[email protected]> wrote:
> > My style preference is to put the empty statement before the comment.
> > It's unusual to have a comment appearing before a statement on the same
> > line. For example:
> > ; /* We don't care if the function fails */
>
> Changed. And I agree, it looks better now. :)
Acked-by: Alan Stern <[email protected]>
On Fri, Mar 25, 2011 at 01:03:12PM -0400, Alan Stern wrote:
> On Fri, 25 Mar 2011, Michal Nazarewicz wrote:
>
> > The usb_create_sysfs_intf_files() function always returned zero even
> > if it failed to create sysfs fails. Since this is a desired behaviour
> > there is no need to return return code at all. This commit changes
> > function's return type (form int) to void.
> >
> > Signed-off-by: Michal Nazarewicz <[email protected]>
> > Cc: Sergey Senozhatsky <[email protected]>
> > ---
> > drivers/usb/core/sysfs.c | 13 +++++--------
> > drivers/usb/core/usb.c | 3 +--
> > drivers/usb/core/usb.h | 2 +-
> > 3 files changed, 7 insertions(+), 11 deletions(-)
> >
> > On Fri, 25 Mar 2011 16:33:12 +0100, Alan Stern <[email protected]> wrote:
> > > My style preference is to put the empty statement before the comment.
> > > It's unusual to have a comment appearing before a statement on the same
> > > line. For example:
> > > ; /* We don't care if the function fails */
> >
> > Changed. And I agree, it looks better now. :)
>
> Acked-by: Alan Stern <[email protected]>
I can't find this version of the patch in my email archive anywhere.
Michal, can you resend it so I can apply it?
thanks,
greg k-h
The usb_create_sysfs_intf_files() function always returned zero even
if it failed to create sysfs fails. Since this is a desired behaviour
there is no need to return return code at all. This commit changes
function's return type (form int) to void.
Signed-off-by: Michal Nazarewicz <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
---
drivers/usb/core/sysfs.c | 13 +++++--------
drivers/usb/core/usb.c | 3 +--
drivers/usb/core/usb.h | 2 +-
3 files changed, 7 insertions(+), 11 deletions(-)
On Thu, 14 Apr 2011 01:23:15 +0200, Greg KH <[email protected]> wrote:
> I can't find this version of the patch in my email archive anywhere.
>
> Michal, can you resend it so I can apply it?
Here it is.
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 6781c36..cf05b97 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -842,22 +842,19 @@ const struct attribute_group *usb_interface_groups[] = {
NULL
};
-int usb_create_sysfs_intf_files(struct usb_interface *intf)
+void usb_create_sysfs_intf_files(struct usb_interface *intf)
{
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_host_interface *alt = intf->cur_altsetting;
- int retval;
if (intf->sysfs_files_created || intf->unregistering)
- return 0;
+ return;
- if (alt->string == NULL &&
- !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
+ if (!alt->string && !(udev->quirks & USB_QUIRK_CONFIG_INTF_STRINGS))
alt->string = usb_cache_string(udev, alt->desc.iInterface);
- if (alt->string)
- retval = device_create_file(&intf->dev, &dev_attr_interface);
+ if (alt->string && device_create_file(&intf->dev, &dev_attr_interface))
+ ; /* We don't actually care if the function fails. */
intf->sysfs_files_created = 1;
- return 0;
}
void usb_remove_sysfs_intf_files(struct usb_interface *intf)
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index d9d4b16..8706fc9 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -953,8 +953,7 @@ static int usb_bus_notify(struct notifier_block *nb, unsigned long action,
if (dev->type == &usb_device_type)
(void) usb_create_sysfs_dev_files(to_usb_device(dev));
else if (dev->type == &usb_if_device_type)
- (void) usb_create_sysfs_intf_files(
- to_usb_interface(dev));
+ usb_create_sysfs_intf_files(to_usb_interface(dev));
break;
case BUS_NOTIFY_DEL_DEVICE:
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index d450b74..d44d4b7 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -4,7 +4,7 @@
extern int usb_create_sysfs_dev_files(struct usb_device *dev);
extern void usb_remove_sysfs_dev_files(struct usb_device *dev);
-extern int usb_create_sysfs_intf_files(struct usb_interface *intf);
+extern void usb_create_sysfs_intf_files(struct usb_interface *intf);
extern void usb_remove_sysfs_intf_files(struct usb_interface *intf);
extern int usb_create_ep_devs(struct device *parent,
struct usb_host_endpoint *endpoint,
--
1.7.3.1