[PATCH v2 0/3] phy: Fix generic_setup_phy return value on power on failure

[PATCH v2 0/3] phy: Fix generic_setup_phy return value on power on failure

[Jonas Karlman]

This series fixes two issues related to the return value in case of
error from generic_setup_phy().

Patch 1 fixes that the error code from power_on gets returned.
Patch 2 fixes an regression so that success is returned instead of ENOENT.
Patch 3 refactor generic_{setup,shutdown}_phy() to improve readability.

Changes in v2:
- Split patch in two
- Restore old behavior for ENOENT
- Add unit tests

Jonas Karlman (3):
  phy: Fix generic_setup_phy() return value on power on failure
  phy: Return success from generic_setup_phy() when phy is not found
  phy: Refactor generic_{setup,shutdown}_phy() to reduce complexity

 drivers/phy/phy-uclass.c | 41 ++++++++++++++++------------------------
 test/dm/phy.c            | 29 ++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 25 deletions(-)

-- 
2.42.0

[PATCH v2 1/3] phy: Fix generic_setup_phy() return value on power on failure

[Jonas Karlman]

generic_phy_exit() typically return 0 for a struct phy that has been
initialized with a generic_phy_init() call.

generic_setup_phy() returns the value from a generic_phy_exit() call
when generic_phy_power_on() fails. This hides the failed state of the
power_on ops from the caller of generic_setup_phy().

Fix this by ignoring the return value of the generic_phy_exit() call and
return the value from the generic_phy_power_on() call.

Fixes: 84e561407a5f ("phy: Add generic_{setup,shutdown}_phy() helpers")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- Move refactor to own patch
- Add unit test

 drivers/phy/phy-uclass.c |  2 +-
 test/dm/phy.c            | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
index 7d707c022934..d745e7babc12 100644
--- a/drivers/phy/phy-uclass.c
+++ b/drivers/phy/phy-uclass.c
@@ -526,7 +526,7 @@ int generic_setup_phy(struct udevice *dev, struct phy *phy, int index)
 
 		ret = generic_phy_power_on(phy);
 		if (ret)
-			ret = generic_phy_exit(phy);
+			generic_phy_exit(phy);
 	}
 
 	return ret;
diff --git a/test/dm/phy.c b/test/dm/phy.c
index 2abd27b22d6d..4da4841f40f7 100644
--- a/test/dm/phy.c
+++ b/test/dm/phy.c
@@ -234,3 +234,27 @@ static int dm_test_phy_multi_exit(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_phy_multi_exit, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int dm_test_phy_setup(struct unit_test_state *uts)
+{
+	struct phy phy;
+	struct udevice *parent;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_SIMPLE_BUS,
+					      "gen_phy_user", &parent));
+
+	/* normal */
+	ut_assertok(generic_setup_phy(parent, &phy, 0));
+	ut_assertok(generic_shutdown_phy(&phy));
+
+	/* power_off fail with -EIO */
+	ut_assertok(generic_setup_phy(parent, &phy, 1));
+	ut_asserteq(-EIO, generic_shutdown_phy(&phy));
+
+	/* power_on fail with -EIO */
+	ut_asserteq(-EIO, generic_setup_phy(parent, &phy, 2));
+	ut_assertok(generic_shutdown_phy(&phy));
+
+	return 0;
+}
+DM_TEST(dm_test_phy_setup, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.42.0

[PATCH v2 2/3] phy: Return success from generic_setup_phy() when phy is not found

[Jonas Karlman]

Restore the old behavior of ehci_setup_phy() and ohci_setup_phy() to
return success when generic_phy_get_by_index() return -ENOENT.

Fixes: 84e561407a5f ("phy: Add generic_{setup,shutdown}_phy() helpers")
Fixes: 10005004db73 ("usb: ohci: Make usage of generic_{setup,shutdown}_phy() helpers")
Fixes: 083f8aa978a8 ("usb: ehci: Make usage of generic_{setup,shutdown}_phy() helpers")
Fixes: 75341e9c16aa ("usb: ehci: Remove unused ehci_{setup,shutdown}_phy() helpers")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- New patch to fix regression introduced in v2023.01-rc1

 drivers/phy/phy-uclass.c | 4 ++--
 test/dm/phy.c            | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
index d745e7babc12..343c23cead0c 100644
--- a/drivers/phy/phy-uclass.c
+++ b/drivers/phy/phy-uclass.c
@@ -517,8 +517,8 @@ int generic_setup_phy(struct udevice *dev, struct phy *phy, int index)
 
 	ret = generic_phy_get_by_index(dev, index, phy);
 	if (ret) {
-		if (ret != -ENOENT)
-			return ret;
+		if (ret == -ENOENT)
+			return 0;
 	} else {
 		ret = generic_phy_init(phy);
 		if (ret)
diff --git a/test/dm/phy.c b/test/dm/phy.c
index 4da4841f40f7..4f91abca3a02 100644
--- a/test/dm/phy.c
+++ b/test/dm/phy.c
@@ -255,6 +255,11 @@ static int dm_test_phy_setup(struct unit_test_state *uts)
 	ut_asserteq(-EIO, generic_setup_phy(parent, &phy, 2));
 	ut_assertok(generic_shutdown_phy(&phy));
 
+	/* generic_phy_get_by_index fail with -ENOENT */
+	ut_asserteq(-ENOENT, generic_phy_get_by_index(parent, 3, &phy));
+	ut_assertok(generic_setup_phy(parent, &phy, 3));
+	ut_assertok(generic_shutdown_phy(&phy));
+
 	return 0;
 }
 DM_TEST(dm_test_phy_setup, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.42.0

[PATCH v2 3/3] phy: Refactor generic_{setup, shutdown}_phy() to reduce complexity

[Jonas Karlman]

Refactor generic_{setup,shutdown}_phy() to reduce complexity and
indentation. This have no intended functional change.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- Split code refactor into own patch

 drivers/phy/phy-uclass.c | 41 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
index 343c23cead0c..190108e04c7f 100644
--- a/drivers/phy/phy-uclass.c
+++ b/drivers/phy/phy-uclass.c
@@ -510,44 +510,35 @@ 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;
-
-	if (!phy)
-		return 0;
+	int ret;
 
 	ret = generic_phy_get_by_index(dev, index, phy);
-	if (ret) {
-		if (ret == -ENOENT)
-			return 0;
-	} else {
-		ret = generic_phy_init(phy);
-		if (ret)
-			return ret;
+	if (ret)
+		return ret == -ENOENT ? 0 : ret;
 
-		ret = generic_phy_power_on(phy);
-		if (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;
 }
 
 int generic_shutdown_phy(struct phy *phy)
 {
-	int ret = 0;
+	int ret;
 
-	if (!phy)
+	if (!generic_phy_valid(phy))
 		return 0;
 
-	if (generic_phy_valid(phy)) {
-		ret = generic_phy_power_off(phy);
-		if (ret)
-			return ret;
-
-		ret = generic_phy_exit(phy);
-	}
+	ret = generic_phy_power_off(phy);
+	if (ret)
+		return ret;
 
-	return ret;
+	return generic_phy_exit(phy);
 }
 
 UCLASS_DRIVER(phy) = {
-- 
2.42.0

Re: [PATCH v2 1/3] phy: Fix generic_setup_phy() return value on power on failure

[Tom Rini]

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

On Thu, Aug 31, 2023 at 11:07:08PM +0000, Jonas Karlman wrote:

> generic_phy_exit() typically return 0 for a struct phy that has been
> initialized with a generic_phy_init() call.
> 
> generic_setup_phy() returns the value from a generic_phy_exit() call
> when generic_phy_power_on() fails. This hides the failed state of the
> power_on ops from the caller of generic_setup_phy().
> 
> Fix this by ignoring the return value of the generic_phy_exit() call and
> return the value from the generic_phy_power_on() call.
> 
> Fixes: 84e561407a5f ("phy: Add generic_{setup,shutdown}_phy() helpers")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/3] phy: Return success from generic_setup_phy() when phy is not found

[Tom Rini]

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

On Thu, Aug 31, 2023 at 11:07:10PM +0000, Jonas Karlman wrote:

> Restore the old behavior of ehci_setup_phy() and ohci_setup_phy() to
> return success when generic_phy_get_by_index() return -ENOENT.
> 
> Fixes: 84e561407a5f ("phy: Add generic_{setup,shutdown}_phy() helpers")
> Fixes: 10005004db73 ("usb: ohci: Make usage of generic_{setup,shutdown}_phy() helpers")
> Fixes: 083f8aa978a8 ("usb: ehci: Make usage of generic_{setup,shutdown}_phy() helpers")
> Fixes: 75341e9c16aa ("usb: ehci: Remove unused ehci_{setup,shutdown}_phy() helpers")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 3/3] phy: Refactor generic_{setup, shutdown}_phy() to reduce complexity

[Tom Rini]

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

On Thu, Aug 31, 2023 at 11:07:11PM +0000, Jonas Karlman wrote:

> Refactor generic_{setup,shutdown}_phy() to reduce complexity and
> indentation. This have no intended functional change.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]