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 arjun...@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.
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
diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 2b1039e0f1ac..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,8 +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)) {
- 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;
}
pgpo5S0FH40Jp.pgp
Description: PGP signature
Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure
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 arjun...@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. 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
signature.asc
Description: Digital signature
Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure
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 arjun...@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
pgpMqhroiAnGt.pgp
Description: PGP signature
Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure
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 arjun...@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.
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
signature.asc
Description: Digital signature
Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure
Hi again, On Mon, Nov 24, 2014 at 09:36:21AM -0600, Felipe Balbi 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 arjun...@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. 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 :-) still, if you send your patch as a proper patch, I'll queue it as it definitely makes sense to not return -ENODEV when we have a phandle. -- balbi signature.asc Description: Digital signature
Re: [PATCH] usb:phy: propagate __of_usb_find_phy()'s error on failure
On Fri, Nov 21, 2014 at 12:19:42PM +0530, Arjun Sreedharan wrote:
On 21 November 2014 01:09, Felipe Balbi ba...@ti.com 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 arjun...@gmail.com
---
drivers/usb/phy/phy.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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);
+
trailing whitespace here. I'll fix it myself this time, but next time be
more careful.
sorry about that. my bloody vi. thanks
add these to your .vimrc :-)
Highlight redundant whitespaces
highlight RedundantWhitespace ctermbg=red guibg=red
match RedundantWhitespace /\s\+$\| \+\ze\t/
--
balbi
signature.asc
Description: Digital signature
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 arjun...@gmail.com
---
drivers/usb/phy/phy.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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);
+
trailing whitespace here. I'll fix it myself this time, but next time be
more careful.
cheers
--
balbi
signature.asc
Description: Digital signature
