linux-kernel - Re: [PATCH] usb:phy: propagate __of_usb_find

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 24 Nov 2014 09:37:54 -0600
From:	Felipe Balbi <balbi@...com>
To:	Felipe Balbi <balbi@...com>
CC:	Thierry Reding <thierry.reding@...il.com>,
	Arjun Sreedharan <arjun024@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-next@...r.kernel.org>,
	<kernel-build-reports@...ts.linaro.org>
Subject: Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure

Hi again,

On Mon, Nov 24, 2014 at 09:36:21AM -0600, Felipe Balbi wrote:
> > > > > When __of_usb_find_phy() fails, it returns -ENODEV - its
> > > > > error code has to be returned by devm_usb_get_phy_by_phandle().
> > > > > Only when the former function succeeds and try_module_get()
> > > > > fails should -EPROBE_DEFER be returned.
> > > > > 
> > > > > Signed-off-by: Arjun Sreedharan <arjun024@...il.com>
> > > > > ---
> > > > >  drivers/usb/phy/phy.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > This causes a boot regression on at least NVIDIA Dalmore (I boot over
> > > > NFS using a USB network adapter).
> > > > 
> > > > The commit message is somewhat insufficient because while it explains
> > > > what the code does and asserts that it is the right thing to do, it
> > > > fails to explain why.
> > > 
> > > you also fail to explain it causes a regressions with Dalmore.
> > 
> > I thought my explanation below was sufficient, but maybe I should say it
> > in other words: __of_usb_find_phy() returns -ENODEV if no PHY was found
> > to be registered for a given phandle. That causes the driver to abort
> > probing with a -ENODEV error and does not trigger the probe deferral
> > that'd be necessary to get the host controller to find the PHY the next
> > time it was triggered.
> 
> right, and before $subject dev_usb_get_phy_by_phandle() was overwriting
> whatever error code passed by __of_usb_find_phy() to -EPROBE_DEFER.
> 
> > > This is really the correct patch, we shouldn't be overwritting the
> > > error passed in by upper layers.
> > 
> > No, it's very obviously not the correct patch if it causes a regression.
> 
> or it exposes a bug elsewhere :-)

still, if you send your patch as a proper patch, I'll queue it as it
definitely makes sense to not return -ENODEV when we have a phandle.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)