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. 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
Attachment:
signature.asc
Description: Digital signature
