usb:phy: propagate __of_usb_find_phy()'s error on failure
Subsystems (2)
PATCH 1/1 usb:phy: propagate __of_usb_find_phy()'s error on failure
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.
--- drivers/usb/phy/phy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/usb/phy/phy.c +3 -1
@@ -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 ); +devres_free (ptr); goto err1; }
Signature · 1 line
1.7.11.7
Quoted 15 lines · Arjun Sreedharan
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- drivers/usb/phy/phy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/usb/phy/phy.c +0
-0
drivers/usb/phy/phy.c struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, · line 196
Quoted 7 lines
drivers/usb/phy/phy.c +3
-1@@ -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
Signature · 1 line
balbi
A message that is not in the archive (sent off-list or not yet received): CAJFMrCGbUQar751mOZOJ5Fy=HEDb+L7XjQypJxzyE72T=qoDmw@mail.gmail.com
Quoted 27 lines · Arjun Sreedharan
Quoted 25 lines · Felipe Balbi
drivers/usb/phy/phy.c struct usb_phy *devm_usb_get_phy_by_phandle(struct
Quoted 15 lines · Arjun Sreedharan
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- drivers/usb/phy/phy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/usb/phy/phy.c +0
-0@@ -191,7 +191,9 @@ struct usb_phy *devm_usb_get_phy_by_phandle(structdevice *dev,
Quoted 7 lines
drivers/usb/phy/phy.c +3
-1phy =__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 ); +
Quoted 5 lines
Quoted 3 lines
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/
Signature · 1 line
balbi
Quoted 9 lines · Arjun Sreedharan
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- 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.
drivers/usb/phy/phy.c struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, · line 195
Quoted 11 lines
drivers/usb/phy/phy.c +2
-1@@ -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
drivers/usb/phy/phy.c +11 -3
@@ -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; }
Hi,
Quoted 11 lines · Thierry Reding
Quoted 9 lines · Arjun Sreedharan
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- drivers/usb/phy/phy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 6 lines
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.
Quoted 26 lines
drivers/usb/phy/phy.c struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, · line 195
Quoted 11 lines
drivers/usb/phy/phy.c +2
-1@@ -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 orERR_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 returnERR_PTR(-EPROBE_DEFER)on failure instead ofERR_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 caseERR_PTR(-ENODEV)might be appropriate.
Quoted 4 lines
Also, -EPROBE_DEFER isn't really the proper error for
try_module_get()failure. Other functions (usb_get_phy()andusb_get_phy_dev()) return-ENODEVinstead, 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.
Signature · 1 line
balbi
Quoted 14 lines · Felipe Balbi
Hi,
Quoted 11 lines · Thierry Reding
Quoted 9 lines · Arjun Sreedharan
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- drivers/usb/phy/phy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 8 lines
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 26 lines
Quoted 26 lines
drivers/usb/phy/phy.c struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, · line 195
Quoted 11 lines
drivers/usb/phy/phy.c +2
-1@@ -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 orERR_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 returnERR_PTR(-EPROBE_DEFER)on failure instead ofERR_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 caseERR_PTR(-ENODEV)might be appropriate.
Quoted 7 lines
Quoted 4 lines
Also, -EPROBE_DEFER isn't really the proper error for
try_module_get()failure. Other functions (usb_get_phy()andusb_get_phy_dev()) return-ENODEVinstead, 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
Hi,
Quoted 22 lines · Thierry Reding
Quoted 21 lines · Felipe Balbi
Hi,
Quoted 17 lines · Thierry Reding
Quoted 9 lines · Arjun Sreedharan
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- 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.
Quoted 8 lines
Quoted 1 line
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-ENODEVif no PHY was found to be registered for a given phandle. That causes the driver to abort probing with a-ENODEVerror 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.
Quoted 4 lines
Quoted 2 lines
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 :-)
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
drivers/usb/phy/phy.c struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, · line 195
Quoted 11 lines
drivers/usb/phy/phy.c +2
-1@@ -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 orERR_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 returnERR_PTR(-EPROBE_DEFER)on failure instead ofERR_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 caseERR_PTR(-ENODEV)might be appropriate.Also, -EPROBE_DEFER isn't really the proper error for
try_module_get()failure. Other functions (usb_get_phy()andusb_get_phy_dev()) return-ENODEVinstead, so it'd be more consistent to stick with that. Hence I propose something like the below instead.
Quoted 6 lines
Quoted 2 lines
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.
Signature · 1 line
balbi
Hi again,
Quoted 29 lines · Felipe Balbi
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 9 lines
When
__of_usb_find_phy()fails, it returns-ENODEV- its error code has to be returned bydevm_usb_get_phy_by_phandle(). Only when the former function succeeds andtry_module_get()fails should -EPROBE_DEFER be returned.Signed-off-by Arjun Sreedharan <[email protected]>--- 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.
Quoted 8 lines
Quoted 1 line
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-ENODEVif no PHY was found to be registered for a given phandle. That causes the driver to abort probing with a-ENODEVerror 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.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Signature · 1 line
balbi