* [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
@ 2020-02-25 13:08 Eugeniu Rosca
2020-02-25 15:32 ` Alan Stern
0 siblings, 1 reply; 7+ messages in thread
From: Eugeniu Rosca @ 2020-02-25 13:08 UTC (permalink / raw)
To: linux-usb, linux-renesas-soc, linux-kernel
Cc: Alan Stern, Greg Kroah-Hartman, Thinh Nguyen, Lee, Chiasheng,
Mathieu Malaterre, Kai-Heng Feng, Eugeniu Rosca, Hardik Gajjar,
scan-admin
Address below Coverity complaint (Feb 25, 2020, 8:06 AM CET):
*** CID 1458999: Error handling issues (CHECKED_RETURN)
/drivers/usb/core/hub.c: 1869 in hub_probe()
1863
1864 if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
1865 hub->quirk_check_port_auto_suspend = 1;
1866
1867 if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) {
1868 hub->quirk_disable_autosuspend = 1;
>>> CID 1458999: Error handling issues (CHECKED_RETURN)
>>> Calling "usb_autopm_get_interface" without checking return value (as is done elsewhere 97 out of 111 times).
1869 usb_autopm_get_interface(intf);
1870 }
1871
1872 if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
1873 return 0;
1874
Fixes: 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub")
Cc: Hardik Gajjar <hgajjar@de.adit-jv.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: scan-admin@coverity.com
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
drivers/usb/core/hub.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 1d212f82c69b..ff04ca28970d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1865,8 +1865,12 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
hub->quirk_check_port_auto_suspend = 1;
if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) {
- hub->quirk_disable_autosuspend = 1;
- usb_autopm_get_interface(intf);
+ int r = usb_autopm_get_interface(intf);
+
+ if (!r)
+ hub->quirk_disable_autosuspend = 1;
+ else
+ dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r);
}
if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
--
2.25.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
2020-02-25 13:08 [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface() Eugeniu Rosca
@ 2020-02-25 15:32 ` Alan Stern
2020-02-25 19:12 ` Eugeniu Rosca
0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2020-02-25 15:32 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: linux-usb, linux-renesas-soc, linux-kernel, Greg Kroah-Hartman,
Thinh Nguyen, Lee, Chiasheng, Mathieu Malaterre, Kai-Heng Feng,
Hardik Gajjar, scan-admin
On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> Address below Coverity complaint (Feb 25, 2020, 8:06 AM CET):
>
> *** CID 1458999: Error handling issues (CHECKED_RETURN)
> /drivers/usb/core/hub.c: 1869 in hub_probe()
> 1863
> 1864 if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
> 1865 hub->quirk_check_port_auto_suspend = 1;
> 1866
> 1867 if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) {
> 1868 hub->quirk_disable_autosuspend = 1;
> >>> CID 1458999: Error handling issues (CHECKED_RETURN)
> >>> Calling "usb_autopm_get_interface" without checking return value (as is done elsewhere 97 out of 111 times).
> 1869 usb_autopm_get_interface(intf);
> 1870 }
> 1871
> 1872 if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
> 1873 return 0;
> 1874
>
> Fixes: 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub")
> Cc: Hardik Gajjar <hgajjar@de.adit-jv.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reported-by: scan-admin@coverity.com
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> drivers/usb/core/hub.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 1d212f82c69b..ff04ca28970d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1865,8 +1865,12 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
> hub->quirk_check_port_auto_suspend = 1;
>
> if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) {
> - hub->quirk_disable_autosuspend = 1;
> - usb_autopm_get_interface(intf);
> + int r = usb_autopm_get_interface(intf);
> +
> + if (!r)
> + hub->quirk_disable_autosuspend = 1;
> + else
> + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r);
> }
>
> if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
This change is not necessary, because the resume operation cannot fail
at this point (interfaces are always powered-up during probe). A
better solution would be to call usb_autpm_get_interface_no_resume()
instead.
On the other hand, the other places that call
usb_autopm_get_interface() without checking should be audited. Some of
them almost certainly need to be fixed.
Alan Stern
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
2020-02-25 15:32 ` Alan Stern
@ 2020-02-25 19:12 ` Eugeniu Rosca
2020-02-25 19:39 ` Alan Stern
0 siblings, 1 reply; 7+ messages in thread
From: Eugeniu Rosca @ 2020-02-25 19:12 UTC (permalink / raw)
To: Alan Stern
Cc: linux-usb, linux-renesas-soc, linux-kernel, Greg Kroah-Hartman,
Thinh Nguyen, Lee, Chiasheng, Mathieu Malaterre, Kai-Heng Feng,
Hardik Gajjar, scan-admin, Eugeniu Rosca, Eugeniu Rosca
Hi Alan,
Many thanks for the prompt review.
On Tue, Feb 25, 2020 at 10:32:32AM -0500, Alan Stern wrote:
> On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> > + int r = usb_autopm_get_interface(intf);
> > +
> > + if (!r)
> > + hub->quirk_disable_autosuspend = 1;
> > + else
> > + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r);
> > }
> >
> > if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
>
> This change is not necessary, because the resume operation cannot fail
> at this point (interfaces are always powered-up during probe).
Agreed to avoid unneeded complexity.
> A better solution would be to call usb_autpm_get_interface_no_resume()
> instead.
Pushed to https://lore.kernel.org/lkml/20200225183057.31953-1-erosca@de.adit-jv.com
>
> On the other hand, the other places that call
> usb_autopm_get_interface() without checking should be audited. Some of
> them almost certainly need to be fixed.
A quick 'git grep' outputs below list of auditable candidates [1].
With no relevant devices at hand, I would avoid touching these drivers,
since oftentimes even legitimate patches introduce regressions w/o
testing.
If anybody volunteers with testing, I guess it should be quick to
either convert usb_autpm_get_interface to *_no_resume variant or
handle the return value in place in below instances.
[1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\("
drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf);
drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface);
drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface);
drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf);
sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface);
--
Best Regards
Eugeniu Rosca
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
2020-02-25 19:12 ` Eugeniu Rosca
@ 2020-02-25 19:39 ` Alan Stern
2020-02-25 20:22 ` Eugeniu Rosca
0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2020-02-25 19:39 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: linux-usb, linux-renesas-soc, linux-kernel, Greg Kroah-Hartman,
Thinh Nguyen, Lee, Chiasheng, Mathieu Malaterre, Kai-Heng Feng,
Hardik Gajjar, scan-admin, Eugeniu Rosca
On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> Hi Alan,
>
> Many thanks for the prompt review.
>
> On Tue, Feb 25, 2020 at 10:32:32AM -0500, Alan Stern wrote:
> > On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> > > + int r = usb_autopm_get_interface(intf);
> > > +
> > > + if (!r)
> > > + hub->quirk_disable_autosuspend = 1;
> > > + else
> > > + dev_dbg(&intf->dev, "disable autosuspend err=%d\n", r);
> > > }
> > >
> > > if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
> >
> > This change is not necessary, because the resume operation cannot fail
> > at this point (interfaces are always powered-up during probe).
>
> Agreed to avoid unneeded complexity.
>
> > A better solution would be to call usb_autpm_get_interface_no_resume()
> > instead.
>
> Pushed to https://lore.kernel.org/lkml/20200225183057.31953-1-erosca@de.adit-jv.com
>
> >
> > On the other hand, the other places that call
> > usb_autopm_get_interface() without checking should be audited. Some of
> > them almost certainly need to be fixed.
>
> A quick 'git grep' outputs below list of auditable candidates [1].
>
> With no relevant devices at hand, I would avoid touching these drivers,
> since oftentimes even legitimate patches introduce regressions w/o
> testing.
>
> If anybody volunteers with testing, I guess it should be quick to
> either convert usb_autpm_get_interface to *_no_resume variant or
> handle the return value in place in below instances.
>
> [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\("
> drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf);
> drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface);
> drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface);
> drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf);
> sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface);
Your regular expression isn't right because it doesn't match lines
where the usb_autopm_get_interface() is preceded only by one whitespace
character (i.e., a tab). It also will match lines where there are two
space characters between the = sign and the function name. I think
what you want is more like "(^|[^=[:space:]])\s*" at the start of the
RE.
A revised search finds line 997 in drivers/usb/core/hub.c and lines
216, 269 in drivers/usb/core/port.c. (I didn't try looking in any
other directories.) AFAICT all three of these should check the return
value, although a error message in the kernel log probably isn't
needed.
Do you want to fix those instances up also, maybe merging them in with
the existing patch?
Alan Stern
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
2020-02-25 19:39 ` Alan Stern
@ 2020-02-25 20:22 ` Eugeniu Rosca
2020-02-25 20:54 ` Alan Stern
0 siblings, 1 reply; 7+ messages in thread
From: Eugeniu Rosca @ 2020-02-25 20:22 UTC (permalink / raw)
To: Alan Stern
Cc: linux-usb, linux-renesas-soc, linux-kernel, Greg Kroah-Hartman,
Thinh Nguyen, Lee, Chiasheng, Mathieu Malaterre, Kai-Heng Feng,
Hardik Gajjar, scan-admin, Eugeniu Rosca, Eugeniu Rosca
Hi Alan,
On Tue, Feb 25, 2020 at 02:39:23PM -0500, Alan Stern wrote:
> On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> > [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\("
> > drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf);
> > drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface);
> > drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface);
> > drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf);
> > sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface);
>
> Your regular expression isn't right because it doesn't match lines
> where the usb_autopm_get_interface() is preceded only by one whitespace
> character (i.e., a tab). It also will match lines where there are two
> space characters between the = sign and the function name. I think
> what you want is more like "(^|[^=[:space:]])\s*" at the start of the
> RE.
Agreed. My version filters out some true positives.
>
> A revised search finds line 997 in drivers/usb/core/hub.c and lines
> 216, 269 in drivers/usb/core/port.c. (I didn't try looking in any
> other directories.) AFAICT all three of these should check the return
> value, although a error message in the kernel log probably isn't
> needed.
>
> Do you want to fix those instances up also, maybe merging them in with
> the existing patch?
I wonder if squashing all these fixes into one patch is the best option.
There are three commits fixed by the proposed changes in usb core:
- v5.6-rc3 commit 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub")
- v3.9-rc1 commit 971fcd492cebf5 ("usb: add runtime pm support for usb port device")
- v2.6.33-rc1 commit 253e05724f9230 ("USB: add a "remove hardware" sysfs attribute")
I assume a single fix will create some pain when applying it to the
stable branches. Do you have any preference/inputs about that?
--
Best Regards
Eugeniu Rosca
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
2020-02-25 20:22 ` Eugeniu Rosca
@ 2020-02-25 20:54 ` Alan Stern
2020-02-26 18:10 ` Eugeniu Rosca
0 siblings, 1 reply; 7+ messages in thread
From: Alan Stern @ 2020-02-25 20:54 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: linux-usb, linux-renesas-soc, linux-kernel, Greg Kroah-Hartman,
Thinh Nguyen, Lee, Chiasheng, Mathieu Malaterre, Kai-Heng Feng,
Hardik Gajjar, scan-admin, Eugeniu Rosca
On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> Hi Alan,
>
> On Tue, Feb 25, 2020 at 02:39:23PM -0500, Alan Stern wrote:
> > On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> > > [1] (v5.6-rc3) git grep -En "[^=]\s+usb_autopm_get_interface\("
> > > drivers/input/touchscreen/usbtouchscreen.c:1788: usb_autopm_get_interface(intf);
> > > drivers/media/usb/stkwebcam/stk-webcam.c:628: usb_autopm_get_interface(dev->interface);
> > > drivers/net/usb/hso.c:1308: usb_autopm_get_interface(serial->parent->interface);
> > > drivers/net/usb/r8152.c:5231: usb_autopm_get_interface(tp->intf);
> > > sound/usb/usx2y/us122l.c:192: usb_autopm_get_interface(iface);
> >
> > Your regular expression isn't right because it doesn't match lines
> > where the usb_autopm_get_interface() is preceded only by one whitespace
> > character (i.e., a tab). It also will match lines where there are two
> > space characters between the = sign and the function name. I think
> > what you want is more like "(^|[^=[:space:]])\s*" at the start of the
> > RE.
>
> Agreed. My version filters out some true positives.
>
> >
> > A revised search finds line 997 in drivers/usb/core/hub.c and lines
> > 216, 269 in drivers/usb/core/port.c. (I didn't try looking in any
> > other directories.) AFAICT all three of these should check the return
> > value, although a error message in the kernel log probably isn't
> > needed.
> >
> > Do you want to fix those instances up also, maybe merging them in with
> > the existing patch?
>
> I wonder if squashing all these fixes into one patch is the best option.
>
> There are three commits fixed by the proposed changes in usb core:
> - v5.6-rc3 commit 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub")
> - v3.9-rc1 commit 971fcd492cebf5 ("usb: add runtime pm support for usb port device")
> - v2.6.33-rc1 commit 253e05724f9230 ("USB: add a "remove hardware" sysfs attribute")
>
> I assume a single fix will create some pain when applying it to the
> stable branches. Do you have any preference/inputs about that?
If you prefer to split this up into multiple patches, that's fine with
me.
Alan Stern
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
2020-02-25 20:54 ` Alan Stern
@ 2020-02-26 18:10 ` Eugeniu Rosca
0 siblings, 0 replies; 7+ messages in thread
From: Eugeniu Rosca @ 2020-02-26 18:10 UTC (permalink / raw)
To: Alan Stern
Cc: linux-usb, linux-renesas-soc, linux-kernel, Greg Kroah-Hartman,
Thinh Nguyen, Lee, Chiasheng, Mathieu Malaterre, Kai-Heng Feng,
Hardik Gajjar, scan-admin, Eugeniu Rosca, Eugeniu Rosca
Hi Alan,
On Tue, Feb 25, 2020 at 03:54:20PM -0500, Alan Stern wrote:
> On Tue, 25 Feb 2020, Eugeniu Rosca wrote:
> > There are three commits fixed by the proposed changes in usb core:
> > - v5.6-rc3 commit 1208f9e1d758c9 ("USB: hub: Fix the broken detection of USB3 device in SMSC hub")
> > - v3.9-rc1 commit 971fcd492cebf5 ("usb: add runtime pm support for usb port device")
> > - v2.6.33-rc1 commit 253e05724f9230 ("USB: add a "remove hardware" sysfs attribute")
> >
> > I assume a single fix will create some pain when applying it to the
> > stable branches. Do you have any preference/inputs about that?
>
> If you prefer to split this up into multiple patches, that's fine with
> me.
Please, check https://lore.kernel.org/lkml/20200226175036.14946-1-erosca@de.adit-jv.com/
--
Best Regards
Eugeniu Rosca
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-26 18:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25 13:08 [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface() Eugeniu Rosca
2020-02-25 15:32 ` Alan Stern
2020-02-25 19:12 ` Eugeniu Rosca
2020-02-25 19:39 ` Alan Stern
2020-02-25 20:22 ` Eugeniu Rosca
2020-02-25 20:54 ` Alan Stern
2020-02-26 18:10 ` Eugeniu Rosca
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).