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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 24 Nov 2014 08:36:46 -0600
From:	Felipe Balbi <balbi@...com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	Arjun Sreedharan <arjun024@...il.com>, Felipe Balbi <balbi@...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,

On Mon, Nov 24, 2014 at 02:10:41PM +0100, Thierry Reding wrote:
> On Thu, Nov 20, 2014 at 09:23:36PM +0530, Arjun Sreedharan 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. This is
really the correct patch, we shouldn't be overwritting the error passed
in by upper layers.

> > diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> > index 045cd30..0310112 100644
> > --- a/drivers/usb/phy/phy.c
> > +++ b/drivers/usb/phy/phy.c
> > @@ -191,7 +191,9 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
> >  
> >  	phy = __of_usb_find_phy(node);
> >  	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
> > -		phy = ERR_PTR(-EPROBE_DEFER);
> > +		if (!IS_ERR(phy))
> > +			phy = ERR_PTR(-EPROBE_DEFER);
> 
> If we look at this closer, __of_usb_find_phy() return a valid pointer if
> a PHY was found or ERR_PTR(-ENODEV) otherwise. But since the phandle has
> already been validated, the only reason why __of_usb_find_phy() fails is
> because the PHY that the phandle refers to hasn't been registered yet.
> 
> Returning -EPROBE_DEFER is the correct thing to do in this situation
> because it gives the PHY driver an opportunity to register and the USB
> host controller to try probing again. I suppose one could argue that
> __of_usb_find_phy() should return ERR_PTR(-EPROBE_DEFER) on failure
> instead of ERR_PTR(-ENODEV), since evidently the device does exist, it
> just hasn't been registered yet. On the other hand it could happen that
> the phandle refers to a device tree node that's status = "disabled", in
> which case ERR_PTR(-ENODEV) might be appropriate.
> 
> Also, -EPROBE_DEFER isn't really the proper error for try_module_get()
> failure. Other functions (usb_get_phy() and usb_get_phy_dev()) return
> -ENODEV instead, so it'd be more consistent to stick with that. Hence I
> propose something like the below instead.

I don't mind patch below, but I want to know why Dalmore regressed with
$subject.

-- 
balbi

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