Re: [PATCH 2/2] phy: qcom-qmp: fix pipe-clock imbalance on power-on failure

[Johan Hovold]

On Mon, May 02, 2022 at 03:39:41PM +0530, Vinod Koul wrote:
> On 20-04-22, 17:23, Johan Hovold wrote:
> > Make sure to disable the pipe clock also if ufs-reset deassertion fails
> > during power on.
> > 
> > Note that the ufs-reset is asserted in qcom_qmp_phy_com_exit().
> > 
> > Fixes: c9b589791fc1 ("phy: qcom: Utilize UFS reset controller")
> > Cc: Evan Green <evgreen@xxxxxxxxxxxx>
> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 8c2300bfe489..7d2d1ab061f7 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -5375,14 +5375,14 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  		if (ret) {
> >  			dev_err(qmp->dev, "lane%d reset deassert failed\n",
> >  				qphy->index);
> > -			goto err_lane_rst;
> > +			return ret;
> 
> This can be skipped if we retain the err_lane_rst label
> 
> >  		}
> >  	}
> >  
> >  	ret = clk_prepare_enable(qphy->pipe_clk);
> >  	if (ret) {
> >  		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> > -		goto err_clk_enable;
> > +		goto err_reset_lane;
> >  	}
> >  
> >  	/* Tx, Rx, and PCS configurations */
> > @@ -5433,7 +5433,7 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  
> >  	ret = reset_control_deassert(qmp->ufs_reset);
> >  	if (ret)
> > -		goto err_lane_rst;
> > +		goto err_disable_pipe_clk;
> 
> this is the actual fix...

Right, but with a one-line fix this would read

	goto err_pcs_ready;

which makes no sense at all. Is that really what you prefer?

Renaming just this one and the below one, would leave the error labels
named using two different schemes, which also isn't very nice, but
perhaps ok.

> >  
> >  	qcom_qmp_phy_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl,
> >  			       cfg->pcs_misc_tbl_num);
> > @@ -5472,17 +5472,17 @@ static int qcom_qmp_phy_power_on(struct phy *phy)
> >  					 PHY_INIT_COMPLETE_TIMEOUT);
> >  		if (ret) {
> >  			dev_err(qmp->dev, "phy initialization timed-out\n");
> > -			goto err_pcs_ready;
> > +			goto err_disable_pipe_clk;
> 
> same rename here
> 
> >  		}
> >  	}
> >  	return 0;
> >  
> > -err_pcs_ready:
> > +err_disable_pipe_clk:
> >  	clk_disable_unprepare(qphy->pipe_clk);
> > -err_clk_enable:
> > +err_reset_lane:
> >  	if (cfg->has_lane_rst)
> >  		reset_control_assert(qphy->lane_rst);
> > -err_lane_rst:
> > +
> >  	return ret;
> 
> while I feel the names given by this patch are better, they should not
> be in a fix patch. We should just add the one line fix here and do
> renames later

I'll respin if you prefer, just want to double check that you really
want a one line fix (i.e. goto err_pcs_ready).

Johan