LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Eugeniu Rosca <erosca@de.adit-jv.com>
Cc: linux-usb@vger.kernel.org, <linux-renesas-soc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	"Lee, Chiasheng" <chiasheng.lee@intel.com>,
	Mathieu Malaterre <malat@debian.org>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Hardik Gajjar <hgajjar@de.adit-jv.com>, <scan-admin@coverity.com>,
	Eugeniu Rosca <roscaeugeniu@gmail.com>
Subject: Re: [PATCH] usb: hub: Fix unhandled return value of usb_autopm_get_interface()
Date: Tue, 25 Feb 2020 14:39:23 -0500 (EST)
Message-ID: <Pine.LNX.4.44L0.2002251419120.1485-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <20200225191241.GA32410@lxhi-065.adit-jv.com>

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


  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-25 13:08 Eugeniu Rosca
2020-02-25 15:32 ` Alan Stern
2020-02-25 19:12   ` Eugeniu Rosca
2020-02-25 19:39     ` Alan Stern [this message]
2020-02-25 20:22       ` Eugeniu Rosca
2020-02-25 20:54         ` Alan Stern
2020-02-26 18:10           ` Eugeniu Rosca

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.44L0.2002251419120.1485-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=Thinh.Nguyen@synopsys.com \
    --cc=chiasheng.lee@intel.com \
    --cc=erosca@de.adit-jv.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hgajjar@de.adit-jv.com \
    --cc=kai.heng.feng@canonical.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=malat@debian.org \
    --cc=roscaeugeniu@gmail.com \
    --cc=scan-admin@coverity.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git