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

[Felipe Balbi]

Hi,

On Mon, Nov 24, 2014 at 04:16:46PM +0100, Thierry Reding wrote:
> 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@xxxxxxxxx>
> > > > ---
> > > >  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 :-)

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

although, I have been running this for the last few days with
BeagleBoneBlack and AM437x SK and haven't noticed any issues.

-- 
balbi

Attachment: signature.asc
Description: Digital signature