Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On Mon, Nov 24, 2014 at 08:36:46AM -0600, Felipe Balbi wrote:
> 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@gmail.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.

> 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.

> > > 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.

Note that this isn't only an issue specific to Dalmore. This affects
every device that uses a USB PHY and where the PHY is registered after
the first probe of the USB host controller.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]