The below fixes a warning generated by GCC:
CC drivers/usb/core/sysfs.o
drivers/usb/core/sysfs.c: In function 'usb_create_sysfs_intf_files':
drivers/usb/core/sysfs.c:889:6: warning: variable 'retval' set but not used
Let me know if these are totally wrong etc.. And I'll try
my best to resend properly..
Signed-off-by: Justin P. Mattock <[email protected]>
---
drivers/usb/core/sysfs.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 448f5b4..af56b3e 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -886,7 +886,6 @@ int 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;
@@ -895,7 +894,7 @@ 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);
+ device_create_file(&intf->dev, &dev_attr_interface);
intf->sysfs_files_created = 1;
return 0;
}
--
1.7.1.rc1.21.gf3bd6
The below fixes a build warning from GCC:
drivers/usb/host/ehci-hub.c: In function 'create_companion_file':
drivers/usb/host/ehci-hub.c:533:6: warning: variable 'i' set but not used
Let me know if these are totally wrong etc.. And I'll try
my best to resend properly..
Signed-off-by: Justin P. Mattock <[email protected]>
---
drivers/usb/host/ehci-hub.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index e7d3d8d..dc7290f 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -530,11 +530,10 @@ static DEVICE_ATTR(companion, 0644, show_companion, store_companion);
static inline void create_companion_file(struct ehci_hcd *ehci)
{
- int i;
/* with integrated TT there is no companion! */
if (!ehci_is_TDI(ehci))
- i = device_create_file(ehci_to_hcd(ehci)->self.controller,
+ device_create_file(ehci_to_hcd(ehci)->self.controller,
&dev_attr_companion);
}
--
1.7.1.rc1.21.gf3bd6
On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> if (alt->string)
> - retval = device_create_file(&intf->dev, &dev_attr_interface);
> + device_create_file(&intf->dev, &dev_attr_interface);
> intf->sysfs_files_created = 1;
> return 0;
What should the code do if device_create_file() manages to fail? Yes, ignoring
the return value is one option, but is it the best one? 'return ret;' might be
another one. Somebody who understands this code and has more caffeine than me
should look this over.
(Nothing personal Justin - it's just my opinion that *anytime* we have a patch
that remove a check for a return code, it needs to justify that ignoring the
return code is appropriate).
On 08/03/2010 04:28 AM, [email protected] wrote:
> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
>> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>
>> if (alt->string)
>> - retval = device_create_file(&intf->dev,&dev_attr_interface);
>> + device_create_file(&intf->dev,&dev_attr_interface);
>> intf->sysfs_files_created = 1;
>> return 0;
>
> What should the code do if device_create_file() manages to fail? Yes, ignoring
> the return value is one option, but is it the best one? 'return ret;' might be
> another one. Somebody who understands this code and has more caffeine than me
> should look this over.
>
ignoring the return value is one option, but is it the best one?
probably not. As for return ret; the option did cross my mind, but
figured to do what I had done by removing the retval
> (Nothing personal Justin - it's just my opinion that *anytime* we have a patch
> that remove a check for a return code, it needs to justify that ignoring the
> return code is appropriate).
>
nothing personal taken.. in fact I agree with that whole paragraph.
looking back I should of really explained why I was removing this code
besides a warning message.
Thanks for the response and info on this..
Justin P. Mattock
On Tue, 3 Aug 2010 [email protected] wrote:
> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
> > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>
> > if (alt->string)
> > - retval = device_create_file(&intf->dev, &dev_attr_interface);
> > + device_create_file(&intf->dev, &dev_attr_interface);
> > intf->sysfs_files_created = 1;
> > return 0;
Justin, did you try compiling your new code? Those unused values are
there because device_create_file is declared as __must_check.
> What should the code do if device_create_file() manages to fail? Yes, ignoring
> the return value is one option, but is it the best one? 'return ret;' might be
> another one. Somebody who understands this code and has more caffeine than me
> should look this over.
Failure to create a file in sysfs is almost never fatal and usually not
even dangerous. Ignoring the error is generally better than failing
the entire operation.
Alan Stern
On 08/03/2010 07:29 AM, Alan Stern wrote:
> On Tue, 3 Aug 2010 [email protected] wrote:
>
>> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
>>> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>>
>>> if (alt->string)
>>> - retval = device_create_file(&intf->dev,&dev_attr_interface);
>>> + device_create_file(&intf->dev,&dev_attr_interface);
>>> intf->sysfs_files_created = 1;
>>> return 0;
>
> Justin, did you try compiling your new code? Those unused values are
> there because device_create_file is declared as __must_check.
>
I went as far as compiling, once I saw no warning then figured o.k
I'll send out what I have for feedback then go from there.
(and just for the record I want to thank those who took the time to go
through and give feedback).
>> What should the code do if device_create_file() manages to fail? Yes, ignoring
>> the return value is one option, but is it the best one? 'return ret;' might be
>> another one. Somebody who understands this code and has more caffeine than me
>> should look this over.
>
> Failure to create a file in sysfs is almost never fatal and usually not
> even dangerous. Ignoring the error is generally better than failing
> the entire operation.
>
> Alan Stern
>
>
ahh.. you made this more clear for me.. cool thanks!
Justin P. Mattock
On Tue, 03 Aug 2010 10:29:52 EDT, Alan Stern said:
> On Tue, 3 Aug 2010 [email protected] wrote:
>
> > On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
> > > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> >
> > > if (alt->string)
> > > - retval = device_create_file(&intf->dev, &dev_attr_interface);
> > > + device_create_file(&intf->dev, &dev_attr_interface);
> > > intf->sysfs_files_created = 1;
> > > return 0;
>
> Justin, did you try compiling your new code? Those unused values are
> there because device_create_file is declared as __must_check.
>
> > What should the code do if device_create_file() manages to fail? Yes, ignoring
> > the return value is one option, but is it the best one? 'return ret;' might be
> > another one. Somebody who understands this code and has more caffeine than me
> > should look this over.
>
> Failure to create a file in sysfs is almost never fatal and usually not
> even dangerous. Ignoring the error is generally better than failing
> the entire operation.
Then why the __must_check attribute if it's usually ignorable? I thought
that was reserved for functions that you damned sight better well check
for errors because bad things are afoot otherwise?
On Tue, 3 Aug 2010 [email protected] wrote:
> > Failure to create a file in sysfs is almost never fatal and usually not
> > even dangerous. Ignoring the error is generally better than failing
> > the entire operation.
>
> Then why the __must_check attribute if it's usually ignorable? I thought
> that was reserved for functions that you damned sight better well check
> for errors because bad things are afoot otherwise?
That's a good question. Perhaps Greg KH knows the answer.
Alan Stern
On Tue, 3 Aug 2010, Justin P. Mattock wrote:
> On 08/03/2010 07:29 AM, Alan Stern wrote:
> > On Tue, 3 Aug 2010 [email protected] wrote:
> >
> >> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
> >>> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> >>
> >>> if (alt->string)
> >>> - retval = device_create_file(&intf->dev,&dev_attr_interface);
> >>> + device_create_file(&intf->dev,&dev_attr_interface);
> >>> intf->sysfs_files_created = 1;
> >>> return 0;
> >
> > Justin, did you try compiling your new code? Those unused values are
> > there because device_create_file is declared as __must_check.
> >
>
> I went as far as compiling, once I saw no warning then figured o.k
> I'll send out what I have for feedback then go from there.
> (and just for the record I want to thank those who took the time to go
> through and give feedback).
It's a little surprising that you didn't get any warning. I guess you
don't have CONFIG_ENABLE_MUST_CHECK turned on.
Alan Stern
On Tue, Aug 03, 2010 at 11:34:26AM -0400, Alan Stern wrote:
> On Tue, 3 Aug 2010 [email protected] wrote:
>
> > > Failure to create a file in sysfs is almost never fatal and usually not
> > > even dangerous. Ignoring the error is generally better than failing
> > > the entire operation.
> >
> > Then why the __must_check attribute if it's usually ignorable? I thought
> > that was reserved for functions that you damned sight better well check
> > for errors because bad things are afoot otherwise?
>
> That's a good question. Perhaps Greg KH knows the answer.
You should check the return value for that function. To not do that is
a bug. This one looks like it snuck through. Fixing it properly is the
correct thing to do.
thanks,
greg k-h
On 08/03/2010 08:36 AM, Alan Stern wrote:
> On Tue, 3 Aug 2010, Justin P. Mattock wrote:
>
>> On 08/03/2010 07:29 AM, Alan Stern wrote:
>>> On Tue, 3 Aug 2010 [email protected] wrote:
>>>
>>>> On Mon, 02 Aug 2010 21:26:28 PDT, "Justin P. Mattock" said:
>>>>> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
>>>>
>>>>> if (alt->string)
>>>>> - retval = device_create_file(&intf->dev,&dev_attr_interface);
>>>>> + device_create_file(&intf->dev,&dev_attr_interface);
>>>>> intf->sysfs_files_created = 1;
>>>>> return 0;
>>>
>>> Justin, did you try compiling your new code? Those unused values are
>>> there because device_create_file is declared as __must_check.
>>>
>>
>> I went as far as compiling, once I saw no warning then figured o.k
>> I'll send out what I have for feedback then go from there.
>> (and just for the record I want to thank those who took the time to go
>> through and give feedback).
>
> It's a little surprising that you didn't get any warning. I guess you
> don't have CONFIG_ENABLE_MUST_CHECK turned on.
>
> Alan Stern
>
>
no nothing.. just the original warning.. as for MUST_CHECK
yeah your right it's off.
Justin P. Mattock
On Tue, 3 Aug 2010, Greg KH wrote:
> On Tue, Aug 03, 2010 at 11:34:26AM -0400, Alan Stern wrote:
> > On Tue, 3 Aug 2010 [email protected] wrote:
> >
> > > > Failure to create a file in sysfs is almost never fatal and usually not
> > > > even dangerous. Ignoring the error is generally better than failing
> > > > the entire operation.
> > >
> > > Then why the __must_check attribute if it's usually ignorable? I thought
> > > that was reserved for functions that you damned sight better well check
> > > for errors because bad things are afoot otherwise?
> >
> > That's a good question. Perhaps Greg KH knows the answer.
>
> You should check the return value for that function. To not do that is
> a bug. This one looks like it snuck through. Fixing it properly is the
> correct thing to do.
In other words, usb_set_configuration() should fail if
usb_create_sysfs_intf_files() is unable to create the attribute file
containing an interface's string descriptor?
And likewise, ehci_run() should fail if the "companion" and debugging
files can't be created?
Alan Stern
On Tue, Aug 03, 2010 at 01:09:31PM -0400, Alan Stern wrote:
> On Tue, 3 Aug 2010, Greg KH wrote:
>
> > On Tue, Aug 03, 2010 at 11:34:26AM -0400, Alan Stern wrote:
> > > On Tue, 3 Aug 2010 [email protected] wrote:
> > >
> > > > > Failure to create a file in sysfs is almost never fatal and usually not
> > > > > even dangerous. Ignoring the error is generally better than failing
> > > > > the entire operation.
> > > >
> > > > Then why the __must_check attribute if it's usually ignorable? I thought
> > > > that was reserved for functions that you damned sight better well check
> > > > for errors because bad things are afoot otherwise?
> > >
> > > That's a good question. Perhaps Greg KH knows the answer.
> >
> > You should check the return value for that function. To not do that is
> > a bug. This one looks like it snuck through. Fixing it properly is the
> > correct thing to do.
>
> In other words, usb_set_configuration() should fail if
> usb_create_sysfs_intf_files() is unable to create the attribute file
> containing an interface's string descriptor?
>
> And likewise, ehci_run() should fail if the "companion" and debugging
> files can't be created?
Ah, yeah, now I recall why we didn't do anything with those values :)
It's best to continue on. Perhaps we should just emit a warning to the
system log if the file fails to be created and move on. That would
properly handle the return value and keep gcc, and everyone else, happy.
thanks,
greg k-h