[PATCH] phy: Fix generic_setup_phy return value on power on failure
generic_setup_phy may mask a power on failure due to the return value from generic_phy_exit, typically 0, is being used as return value.
Fix this by ignoring the return value of the generic_phy_exit call, also remove an unnecessary check for -ENOENT.
Fixes: 84e561407a5f ("phy: Add generic_{setup,shutdown}_phy() helpers") Signed-off-by: Jonas Karlman jonas@kwiboo.se --- drivers/phy/phy-uclass.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c index 629ef3aa3de5..afb0e347f76f 100644 --- a/drivers/phy/phy-uclass.c +++ b/drivers/phy/phy-uclass.c @@ -506,24 +506,22 @@ int generic_phy_power_off_bulk(struct phy_bulk *bulk)
int generic_setup_phy(struct udevice *dev, struct phy *phy, int index) { - int ret = 0; + int ret;
if (!phy) return 0;
ret = generic_phy_get_by_index(dev, index, phy); - if (ret) { - if (ret != -ENOENT) - return ret; - } else { - ret = generic_phy_init(phy); - if (ret) - return ret; + if (ret) + return ret;
- ret = generic_phy_power_on(phy); - if (ret) - ret = generic_phy_exit(phy); - } + ret = generic_phy_init(phy); + if (ret) + return ret; + + ret = generic_phy_power_on(phy); + if (ret) + generic_phy_exit(phy);
return ret; }
On 7/12/23 01:48, Jonas Karlman wrote:
generic_setup_phy may mask a power on failure due to the return value from generic_phy_exit, typically 0, is being used as return value.
Fix this by ignoring the return value of the generic_phy_exit call, also remove an unnecessary check for -ENOENT.
Why is it unnecessary ? If I recall it right, the ENOENT check is to not fail if the PHY is not present in DT, which may not be a failure.
[...]
On 2023-07-12 01:59, Marek Vasut wrote:
On 7/12/23 01:48, Jonas Karlman wrote:
generic_setup_phy may mask a power on failure due to the return value from generic_phy_exit, typically 0, is being used as return value.
Fix this by ignoring the return value of the generic_phy_exit call, also remove an unnecessary check for -ENOENT.
Why is it unnecessary ? If I recall it right, the ENOENT check is to not fail if the PHY is not present in DT, which may not be a failure.
In the current state ret is returned any way, so this check is currently pointless.
Looking back at the old originating code for generic_setup_phy the return value was 0 for similar ENOENT check.
Maybe this is something that should be restored or such check be moved to caller of generic_setup_phy.
Regards, Jonas
[...]
On 7/12/23 02:06, Jonas Karlman wrote:
On 2023-07-12 01:59, Marek Vasut wrote:
On 7/12/23 01:48, Jonas Karlman wrote:
generic_setup_phy may mask a power on failure due to the return value from generic_phy_exit, typically 0, is being used as return value.
Fix this by ignoring the return value of the generic_phy_exit call, also remove an unnecessary check for -ENOENT.
Why is it unnecessary ? If I recall it right, the ENOENT check is to not fail if the PHY is not present in DT, which may not be a failure.
In the current state ret is returned any way, so this check is currently pointless.
I don't think so, the test is:
if (ret && ret != -ENOENT) return ret;
i.e. the ENOENT is a special case.
Looking back at the old originating code for generic_setup_phy the return value was 0 for similar ENOENT check.
Maybe this is something that should be restored or such check be moved to caller of generic_setup_phy.
On 2023-07-12 03:48, Marek Vasut wrote:
On 7/12/23 02:06, Jonas Karlman wrote:
On 2023-07-12 01:59, Marek Vasut wrote:
On 7/12/23 01:48, Jonas Karlman wrote:
generic_setup_phy may mask a power on failure due to the return value from generic_phy_exit, typically 0, is being used as return value.
Fix this by ignoring the return value of the generic_phy_exit call, also remove an unnecessary check for -ENOENT.
Why is it unnecessary ? If I recall it right, the ENOENT check is to not fail if the PHY is not present in DT, which may not be a failure.
In the current state ret is returned any way, so this check is currently pointless.
I don't think so, the test is:
if (ret && ret != -ENOENT) return ret;
i.e. the ENOENT is a special case.
Sure, but in this function such check does not affect any outcome. The function look like:
if (ret) { if (ret != -ENOENT) return ret; } else { // init and power on } return ret;
As seen above ret is always returned unless it is 0 in the initial check, making the ENOENT check unnecessary and misleading in this function.
The old ehci_setup_phy function, see [1], returned 0 instead of ret as the final return statement, making the ENOENT check meaningful.
This change in behavior may have been unintentional in [2]. Because this new behavior was merged in v2023.01-rc1, I did not see it appropriate to revert back to the old ehci_setup_phy behavior and instead just removed the unnecessary ENOENT check.
I can include a check for ENOENT at the generic_setup_phy call sites if we want to restore the old behavior?
[1] https://source.denx.de/u-boot/u-boot/-/commit/75341e9c16aa10929a1616060a3b56... [2] https://source.denx.de/u-boot/u-boot/-/commit/083f8aa978a837542ffa53e5247d36...
Regards, Jonas
Looking back at the old originating code for generic_setup_phy the return value was 0 for similar ENOENT check.
Maybe this is something that should be restored or such check be moved to caller of generic_setup_phy.
On 7/12/23 09:36, Jonas Karlman wrote:
On 2023-07-12 03:48, Marek Vasut wrote:
On 7/12/23 02:06, Jonas Karlman wrote:
On 2023-07-12 01:59, Marek Vasut wrote:
On 7/12/23 01:48, Jonas Karlman wrote:
generic_setup_phy may mask a power on failure due to the return value from generic_phy_exit, typically 0, is being used as return value.
Fix this by ignoring the return value of the generic_phy_exit call, also remove an unnecessary check for -ENOENT.
Why is it unnecessary ? If I recall it right, the ENOENT check is to not fail if the PHY is not present in DT, which may not be a failure.
In the current state ret is returned any way, so this check is currently pointless.
I don't think so, the test is:
if (ret && ret != -ENOENT) return ret;
i.e. the ENOENT is a special case.
Sure, but in this function such check does not affect any outcome. The function look like:
if (ret) { if (ret != -ENOENT) return ret; } else { // init and power on } return ret;
As seen above ret is always returned unless it is 0 in the initial check, making the ENOENT check unnecessary and misleading in this function.
Ah, I see it now, thanks for clarification.
The old ehci_setup_phy function, see [1], returned 0 instead of ret as the final return statement, making the ENOENT check meaningful.
This change in behavior may have been unintentional in [2]. Because this new behavior was merged in v2023.01-rc1, I did not see it appropriate to revert back to the old ehci_setup_phy behavior and instead just removed the unnecessary ENOENT check.
I somehow suspect that if you get ENOENT, you might need to return 0, but please wait for Patrice to double-check.
I can include a check for ENOENT at the generic_setup_phy call sites if we want to restore the old behavior?
Probably no due to duplication. But please wait for Patrice.
participants (2)
-
Jonas Karlman -
Marek Vasut