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 <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Reported-by: [email protected]
Signed-off-by: Eugeniu Rosca <[email protected]>
---
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
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 <[email protected]>
> Cc: Alan Stern <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Reported-by: [email protected]
> Signed-off-by: Eugeniu Rosca <[email protected]>
> ---
> 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
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/[email protected]
>
> 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
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/[email protected]
>
> >
> > 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
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
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
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/[email protected]/
--
Best Regards
Eugeniu Rosca