[RFC PATCH] phy: qcom: ipq4019-usb: Propagate reset operation failures

[RFC PATCH] phy: qcom: ipq4019-usb: Propagate reset operation failures

[Pengpeng Hou]

The IPQ4019 USB PHY power operations ignore reset assertion and
deassertion failures.  Their callers can therefore publish a PHY power
transition after the reset controller rejected part of it.

Propagate each reset error and restore the reset that was changed
earlier in the same HS transition where possible.  This is an RFC
because a failed reset operation can leave hardware state uncertain;
feedback is requested on whether the proposed best-effort rollback
matches these PHYs.

The issue was identified via static analysis and manually reviewed.

Assisted-by: LLM
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c | 46 ++++++++++++++++-----
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c
index da6f290af722..8c1834cd6a8e 100644
--- a/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c
+++ b/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c
@@ -30,7 +30,12 @@ static int ipq4019_ss_phy_power_off(struct phy *_phy)
 {
 	struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
 
-	reset_control_assert(phy->por_rst);
+	int ret;
+
+	ret = reset_control_assert(phy->por_rst);
+	if (ret)
+		return ret;
+
 	msleep(10);
 
 	return 0;
@@ -40,11 +45,13 @@ static int ipq4019_ss_phy_power_on(struct phy *_phy)
 {
 	struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
 
-	ipq4019_ss_phy_power_off(_phy);
+	int ret;
 
-	reset_control_deassert(phy->por_rst);
+	ret = ipq4019_ss_phy_power_off(_phy);
+	if (ret)
+		return ret;
 
-	return 0;
+	return reset_control_deassert(phy->por_rst);
 }
 
 static const struct phy_ops ipq4019_usb_ss_phy_ops = {
@@ -56,10 +63,20 @@ static int ipq4019_hs_phy_power_off(struct phy *_phy)
 {
 	struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
 
-	reset_control_assert(phy->por_rst);
+	int ret;
+
+	ret = reset_control_assert(phy->por_rst);
+	if (ret)
+		return ret;
+
 	msleep(10);
 
-	reset_control_assert(phy->srif_rst);
+	ret = reset_control_assert(phy->srif_rst);
+	if (ret) {
+		reset_control_deassert(phy->por_rst);
+		return ret;
+	}
+
 	msleep(10);
 
 	return 0;
@@ -69,14 +86,23 @@ static int ipq4019_hs_phy_power_on(struct phy *_phy)
 {
 	struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
 
-	ipq4019_hs_phy_power_off(_phy);
+	int ret;
+
+	ret = ipq4019_hs_phy_power_off(_phy);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(phy->srif_rst);
+	if (ret)
+		return ret;
 
-	reset_control_deassert(phy->srif_rst);
 	msleep(10);
 
-	reset_control_deassert(phy->por_rst);
+	ret = reset_control_deassert(phy->por_rst);
+	if (ret)
+		reset_control_assert(phy->srif_rst);
 
-	return 0;
+	return ret;
 }
 
 static const struct phy_ops ipq4019_usb_hs_phy_ops = {
-- 
2.43.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [RFC PATCH] phy: qcom: ipq4019-usb: Propagate reset operation failures

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly introduced error rollback in `ipq4019_hs_phy_power_off()` incorrectly deasserts `por_rst` when called from `ipq4019_hs_phy_power_on()`, corrupting the PHY state.
--

commit cf6cfeb0a60a431fc71ac93de671a2fcf94d05dc
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

phy: qcom: ipq4019-usb: Propagate reset operation failures
    
This commit propagates reset operation failures in the IPQ4019 USB PHY driver
and attempts a best-effort rollback if an operation fails.

> diff --git a/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c
> index da6f290af7221..8c1834cd6a8e9 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c

[ ... ]

> @@ -56,10 +63,20 @@ static int ipq4019_hs_phy_power_off(struct phy *_phy)
>  {
>  	struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
>  
> -	reset_control_assert(phy->por_rst);
> +	int ret;
> +
> +	ret = reset_control_assert(phy->por_rst);
> +	if (ret)
> +		return ret;
> +
>  	msleep(10);
>  
> -	reset_control_assert(phy->srif_rst);
> +	ret = reset_control_assert(phy->srif_rst);
> +	if (ret) {
> +		reset_control_deassert(phy->por_rst);
> +		return ret;
> +	}
> +
>  	msleep(10);
>  
>  	return 0;
> @@ -69,14 +86,23 @@ static int ipq4019_hs_phy_power_on(struct phy *_phy)
>  {
>  	struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
>  
> -	ipq4019_hs_phy_power_off(_phy);
> +	int ret;
> +
> +	ret = ipq4019_hs_phy_power_off(_phy);
> +	if (ret)
> +		return ret;

[Severity: Medium]
When ipq4019_hs_phy_power_on() calls ipq4019_hs_phy_power_off() to pulse the
reset lines, it starts from an already-off state (where por_rst is asserted).

If reset_control_assert(phy->srif_rst) fails inside ipq4019_hs_phy_power_off(),
the rollback will unconditionally call reset_control_deassert(phy->por_rst).

Does this incorrectly assume the initial state was on, leaving the hardware in
an inconsistent state (por_rst deasserted, srif_rst asserted) instead of safely
remaining in a clean off state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828091857.8581-1-pengpeng@iscas.ac.cn?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy