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]

Message-ID: <20141124131034.GB4061@ulmo.nvidia.com>
Date:	Mon, 24 Nov 2014 14:10:41 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Arjun Sreedharan <arjun024@...il.com>
Cc:	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

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.

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

Adding linux-next and kernel-build-reports in case anyone's running into
the same issue.

Thierry

View attachment "patch" of type "text/plain" (1102 bytes)

Content of type "application/pgp-signature" skipped