[PATCH] usb: phy: Restore deferred probing path

[PATCH] usb: phy: Restore deferred probing path

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
failure") broke platforms that rely on deferred probing to order probing
of PHY and host controller drivers. The reason is that the commit simply
propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
PHY has been registered yet for a given device tree node. The only case
in which -EPROBE_DEFER would now be returned is if try_module_get() did
fail, which does not make sense.

The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
registered yet. The only condition under which it makes sense to return
-ENODEV is if the device tree node representing the PHY has been
disabled (via the status property) because in that case the PHY will
never be registered.

This patch addresses the problem by making __of_usb_find_phy() return an
appropriate error code while keeping in line with the above-mentioned
commit to propagate error codes rather than overwriting them. At the
same time the check for a valid PHY is decoupled from the check for the
try_module_get() call and a separate error code is returned if the
latter fails.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/usb/phy/phy.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index b4066a001ba0..2f9735b35338 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -59,6 +59,9 @@ static struct usb_phy *__of_usb_find_phy(struct device_node *node)
 {
 	struct usb_phy  *phy;
 
+	if (!of_device_is_available(node))
+		return ERR_PTR(-ENODEV);
+
 	list_for_each_entry(phy, &phy_list, head) {
 		if (node != phy->dev->of_node)
 			continue;
@@ -66,7 +69,7 @@ static struct usb_phy *__of_usb_find_phy(struct device_node *node)
 		return phy;
 	}
 
-	return ERR_PTR(-ENODEV);
+	return ERR_PTR(-EPROBE_DEFER);
 }
 
 static void devm_usb_phy_release(struct device *dev, void *res)
@@ -190,10 +193,13 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 	spin_lock_irqsave(&phy_lock, flags);
 
 	phy = __of_usb_find_phy(node);
-	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
-		if (!IS_ERR(phy))
-			phy = ERR_PTR(-EPROBE_DEFER);
+	if (IS_ERR(phy)) {
+		devres_free(ptr);
+		goto err1;
+	}
 
+	if (!try_module_get(phy->dev->driver->owner)) {
+		phy = ERR_PTR(-ENODEV);
 		devres_free(ptr);
 		goto err1;
 	}
-- 
2.1.3

Re: [PATCH] usb: phy: Restore deferred probing path

[Sergei Shtylyov]

Hello.

On 12/4/2014 3:06 PM, Thierry Reding wrote:

> From: Thierry Reding <treding@nvidia.com>

> Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> failure") broke platforms that rely on deferred probing to order probing
> of PHY and host controller drivers. The reason is that the commit simply
> propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> PHY has been registered yet for a given device tree node. The only case
> in which -EPROBE_DEFER would now be returned is if try_module_get() did
> fail, which does not make sense.

> The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> registered yet. The only condition under which it makes sense to return
> -ENODEV is if the device tree node representing the PHY has been
> disabled (via the status property) because in that case the PHY will
> never be registered.

> This patch addresses the problem by making __of_usb_find_phy() return an
> appropriate error code while keeping in line with the above-mentioned
> commit to propagate error codes rather than overwriting them. At the
> same time the check for a valid PHY is decoupled from the check for the
> try_module_get() call and a separate error code is returned if the
> latter fails.

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/usb/phy/phy.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)

> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> index b4066a001ba0..2f9735b35338 100644
> --- a/drivers/usb/phy/phy.c
> +++ b/drivers/usb/phy/phy.c
[...]
> @@ -190,10 +193,13 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
>   	spin_lock_irqsave(&phy_lock, flags);
>
>   	phy = __of_usb_find_phy(node);
> -	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
> -		if (!IS_ERR(phy))
> -			phy = ERR_PTR(-EPROBE_DEFER);
> +	if (IS_ERR(phy)) {
> +		devres_free(ptr);
> +		goto err1;
> +	}
>
> +	if (!try_module_get(phy->dev->driver->owner)) {
> +		phy = ERR_PTR(-ENODEV);
>   		devres_free(ptr);
>   		goto err1;
>   	}

    I think devres_free() should now be called in one place, under the new 
'err2' label.

WBR, Sergei

Re: [PATCH] usb: phy: Restore deferred probing path

[Thierry Reding]

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

On Thu, Dec 04, 2014 at 03:23:06PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/4/2014 3:06 PM, Thierry Reding wrote:
> 
> >From: Thierry Reding <treding@nvidia.com>
> 
> >Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> >failure") broke platforms that rely on deferred probing to order probing
> >of PHY and host controller drivers. The reason is that the commit simply
> >propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> >PHY has been registered yet for a given device tree node. The only case
> >in which -EPROBE_DEFER would now be returned is if try_module_get() did
> >fail, which does not make sense.
> 
> >The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> >registered yet. The only condition under which it makes sense to return
> >-ENODEV is if the device tree node representing the PHY has been
> >disabled (via the status property) because in that case the PHY will
> >never be registered.
> 
> >This patch addresses the problem by making __of_usb_find_phy() return an
> >appropriate error code while keeping in line with the above-mentioned
> >commit to propagate error codes rather than overwriting them. At the
> >same time the check for a valid PHY is decoupled from the check for the
> >try_module_get() call and a separate error code is returned if the
> >latter fails.
> 
> >Signed-off-by: Thierry Reding <treding@nvidia.com>
> >---
> >  drivers/usb/phy/phy.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> >diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> >index b4066a001ba0..2f9735b35338 100644
> >--- a/drivers/usb/phy/phy.c
> >+++ b/drivers/usb/phy/phy.c
> [...]
> >@@ -190,10 +193,13 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
> >  	spin_lock_irqsave(&phy_lock, flags);
> >
> >  	phy = __of_usb_find_phy(node);
> >-	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
> >-		if (!IS_ERR(phy))
> >-			phy = ERR_PTR(-EPROBE_DEFER);
> >+	if (IS_ERR(phy)) {
> >+		devres_free(ptr);
> >+		goto err1;
> >+	}
> >
> >+	if (!try_module_get(phy->dev->driver->owner)) {
> >+		phy = ERR_PTR(-ENODEV);
> >  		devres_free(ptr);
> >  		goto err1;
> >  	}
> 
>    I think devres_free() should now be called in one place, under the new
> 'err2' label.

That'd make things very confusing, though. devres_free() should only be
called on error, whereas both err1 and err0 are shared with the success
case as well. If we were to do what you suggest we'd end up with
something like this:

		...

		phy = __of_usb_find_phy(node);
		if (IS_ERR(phy))
			goto err2;

		if (!try_module_get(...)) {
			phy = ERR_PTR(-ENODEV);
			goto err2;
		}

		*ptr = phy;
		devres_add(dev, ptr);

		get_device(phy->dev);

		goto err1;

	err2:
		devres_free(ptr);

	err1:
		spin_unlock_irqrestore(&phy_lock, flags);

	err0:
		of_node_put(node);

		return phy;
	}

That's pretty spaghetti-like.

Thierry

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

Re: [PATCH] usb: phy: Restore deferred probing path

[Sergei Shtylyov]

On 12/4/2014 4:22 PM, Thierry Reding wrote:

>>> From: Thierry Reding <treding@nvidia.com>

>>> Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
>>> failure") broke platforms that rely on deferred probing to order probing
>>> of PHY and host controller drivers. The reason is that the commit simply
>>> propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
>>> PHY has been registered yet for a given device tree node. The only case
>>> in which -EPROBE_DEFER would now be returned is if try_module_get() did
>>> fail, which does not make sense.

>>> The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
>>> registered yet. The only condition under which it makes sense to return
>>> -ENODEV is if the device tree node representing the PHY has been
>>> disabled (via the status property) because in that case the PHY will
>>> never be registered.

>>> This patch addresses the problem by making __of_usb_find_phy() return an
>>> appropriate error code while keeping in line with the above-mentioned
>>> commit to propagate error codes rather than overwriting them. At the
>>> same time the check for a valid PHY is decoupled from the check for the
>>> try_module_get() call and a separate error code is returned if the
>>> latter fails.

>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>   drivers/usb/phy/phy.c | 14 ++++++++++----
>>>   1 file changed, 10 insertions(+), 4 deletions(-)

>>> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
>>> index b4066a001ba0..2f9735b35338 100644
>>> --- a/drivers/usb/phy/phy.c
>>> +++ b/drivers/usb/phy/phy.c
>> [...]
>>> @@ -190,10 +193,13 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
>>>   	spin_lock_irqsave(&phy_lock, flags);
>>>
>>>   	phy = __of_usb_find_phy(node);
>>> -	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
>>> -		if (!IS_ERR(phy))
>>> -			phy = ERR_PTR(-EPROBE_DEFER);
>>> +	if (IS_ERR(phy)) {
>>> +		devres_free(ptr);
>>> +		goto err1;
>>> +	}
>>>
>>> +	if (!try_module_get(phy->dev->driver->owner)) {
>>> +		phy = ERR_PTR(-ENODEV);
>>>   		devres_free(ptr);
>>>   		goto err1;
>>>   	}

>>     I think devres_free() should now be called in one place, under the new
>> 'err2' label.

> That'd make things very confusing, though. devres_free() should only be
> called on error, whereas both err1 and err0 are shared with the success
> case as well. If we were to do what you suggest we'd end up with
> something like this:

[...]

> That's pretty spaghetti-like.

    Oh, sorry, I wasn't looking attentively enough. :-/

> Thierry

WBR, Sergei

Re: [PATCH] usb: phy: Restore deferred probing path

[Thierry Reding]

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

On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> failure") broke platforms that rely on deferred probing to order probing
> of PHY and host controller drivers. The reason is that the commit simply
> propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> PHY has been registered yet for a given device tree node. The only case
> in which -EPROBE_DEFER would now be returned is if try_module_get() did
> fail, which does not make sense.
> 
> The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> registered yet. The only condition under which it makes sense to return
> -ENODEV is if the device tree node representing the PHY has been
> disabled (via the status property) because in that case the PHY will
> never be registered.
> 
> This patch addresses the problem by making __of_usb_find_phy() return an
> appropriate error code while keeping in line with the above-mentioned
> commit to propagate error codes rather than overwriting them. At the
> same time the check for a valid PHY is decoupled from the check for the
> try_module_get() call and a separate error code is returned if the
> latter fails.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/usb/phy/phy.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

Ping?

Thierry

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

Re: [PATCH] usb: phy: Restore deferred probing path

[Greg Kroah-Hartman]

On Wed, Dec 17, 2014 at 09:30:03AM +0100, Thierry Reding wrote:
> On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> > failure") broke platforms that rely on deferred probing to order probing
> > of PHY and host controller drivers. The reason is that the commit simply
> > propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> > PHY has been registered yet for a given device tree node. The only case
> > in which -EPROBE_DEFER would now be returned is if try_module_get() did
> > fail, which does not make sense.
> > 
> > The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> > registered yet. The only condition under which it makes sense to return
> > -ENODEV is if the device tree node representing the PHY has been
> > disabled (via the status property) because in that case the PHY will
> > never be registered.
> > 
> > This patch addresses the problem by making __of_usb_find_phy() return an
> > appropriate error code while keeping in line with the above-mentioned
> > commit to propagate error codes rather than overwriting them. At the
> > same time the check for a valid PHY is decoupled from the check for the
> > try_module_get() call and a separate error code is returned if the
> > latter fails.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/usb/phy/phy.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> Ping?

It's the middle of the merge window, we can't do anything until 3.19-rc1
is out at the earliest.

And then my vacation kicks in, so it will be a week or so more after
that :)

greg k-h

Re: [PATCH] usb: phy: Restore deferred probing path

[Thierry Reding]

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

On Wed, Dec 17, 2014 at 09:34:48AM -0800, Greg Kroah-Hartman wrote:
> On Wed, Dec 17, 2014 at 09:30:03AM +0100, Thierry Reding wrote:
> > On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> > > failure") broke platforms that rely on deferred probing to order probing
> > > of PHY and host controller drivers. The reason is that the commit simply
> > > propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> > > PHY has been registered yet for a given device tree node. The only case
> > > in which -EPROBE_DEFER would now be returned is if try_module_get() did
> > > fail, which does not make sense.
> > > 
> > > The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> > > registered yet. The only condition under which it makes sense to return
> > > -ENODEV is if the device tree node representing the PHY has been
> > > disabled (via the status property) because in that case the PHY will
> > > never be registered.
> > > 
> > > This patch addresses the problem by making __of_usb_find_phy() return an
> > > appropriate error code while keeping in line with the above-mentioned
> > > commit to propagate error codes rather than overwriting them. At the
> > > same time the check for a valid PHY is decoupled from the check for the
> > > try_module_get() call and a separate error code is returned if the
> > > latter fails.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  drivers/usb/phy/phy.c | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > Ping?
> 
> It's the middle of the merge window, we can't do anything until 3.19-rc1
> is out at the earliest.

This fixes a regression that was introduced in commit 1290a958d48e which
has been merged for v3.19-rc1, so this really should go in as soon as
possible. I brought this up around the time of v3.18-rc5 and it's a
shame it's been broken for so long.

Thierry

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

Re: [PATCH] usb: phy: Restore deferred probing path

[Felipe Balbi]

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

On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> failure") broke platforms that rely on deferred probing to order probing
> of PHY and host controller drivers. The reason is that the commit simply
> propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> PHY has been registered yet for a given device tree node. The only case
> in which -EPROBE_DEFER would now be returned is if try_module_get() did
> fail, which does not make sense.
> 
> The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> registered yet. The only condition under which it makes sense to return
> -ENODEV is if the device tree node representing the PHY has been
> disabled (via the status property) because in that case the PHY will
> never be registered.
> 
> This patch addresses the problem by making __of_usb_find_phy() return an
> appropriate error code while keeping in line with the above-mentioned
> commit to propagate error codes rather than overwriting them. At the
> same time the check for a valid PHY is decoupled from the check for the
> try_module_get() call and a separate error code is returned if the
> latter fails.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

you forgot to add the magic "Fixes: foo bar" here, I'll add it this
time, but I've already sent my -rc2 pull request, so this will only show
up on -rc3.

-- 
balbi

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

Re: [PATCH] usb: phy: Restore deferred probing path

[Stephen Warren]

On 12/23/2014 11:36 AM, Felipe Balbi wrote:
> On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
>> failure") broke platforms that rely on deferred probing to order probing
>> of PHY and host controller drivers. The reason is that the commit simply
>> propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
>> PHY has been registered yet for a given device tree node. The only case
>> in which -EPROBE_DEFER would now be returned is if try_module_get() did
>> fail, which does not make sense.
>>
>> The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
>> registered yet. The only condition under which it makes sense to return
>> -ENODEV is if the device tree node representing the PHY has been
>> disabled (via the status property) because in that case the PHY will
>> never be registered.
>>
>> This patch addresses the problem by making __of_usb_find_phy() return an
>> appropriate error code while keeping in line with the above-mentioned
>> commit to propagate error codes rather than overwriting them. At the
>> same time the check for a valid PHY is decoupled from the check for the
>> try_module_get() call and a separate error code is returned if the
>> latter fails.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>
> you forgot to add the magic "Fixes: foo bar" here, I'll add it this
> time, but I've already sent my -rc2 pull request, so this will only show
> up on -rc3.

Presumably also add the following, since the patch fixes a regression in 
a previous kernel release:

Cc: stable # v3.18

?

Re: [PATCH] usb: phy: Restore deferred probing path

[Thierry Reding]

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

On Mon, Jan 05, 2015 at 12:33:51PM -0700, Stephen Warren wrote:
> On 12/23/2014 11:36 AM, Felipe Balbi wrote:
> >On Thu, Dec 04, 2014 at 01:06:07PM +0100, Thierry Reding wrote:
> >>From: Thierry Reding <treding@nvidia.com>
> >>
> >>Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> >>failure") broke platforms that rely on deferred probing to order probing
> >>of PHY and host controller drivers. The reason is that the commit simply
> >>propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> >>PHY has been registered yet for a given device tree node. The only case
> >>in which -EPROBE_DEFER would now be returned is if try_module_get() did
> >>fail, which does not make sense.
> >>
> >>The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> >>registered yet. The only condition under which it makes sense to return
> >>-ENODEV is if the device tree node representing the PHY has been
> >>disabled (via the status property) because in that case the PHY will
> >>never be registered.
> >>
> >>This patch addresses the problem by making __of_usb_find_phy() return an
> >>appropriate error code while keeping in line with the above-mentioned
> >>commit to propagate error codes rather than overwriting them. At the
> >>same time the check for a valid PHY is decoupled from the check for the
> >>try_module_get() call and a separate error code is returned if the
> >>latter fails.
> >>
> >>Signed-off-by: Thierry Reding <treding@nvidia.com>
> >
> >you forgot to add the magic "Fixes: foo bar" here, I'll add it this
> >time, but I've already sent my -rc2 pull request, so this will only show
> >up on -rc3.
> 
> Presumably also add the following, since the patch fixes a regression in a
> previous kernel release:
> 
> Cc: stable # v3.18
> 
> ?

The offending commit was only merged in v3.19-rc1. That's also the
reason why I didn't deem it necessary to include the Fixes: line. Given
that the patch was posted prior to the merge window opening and that it
fixes a regression I had assumed it would be merged in the same pull-
request as the commit causing the regression.

Thierry

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