| Message ID | 20231213022500.9011-1-cw9316.lee@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: qcom: Re-fix for error handling | expand |
On Wed, Dec 13, 2023 at 11:25:00AM +0900, Chanwoo Lee wrote: > From: ChanWoo Lee <cw9316.lee@samsung.com> > > I modified the code to handle errors. > > The error handling code has been changed from the patch below. > -'commit 031312dbc695 ("scsi: ufs: ufs-qcom: Remove unnecessary goto statements")' > > What I have confirmed are three cases. > 1) ufs_qcom_host_reset -> 'reset_control_deassert' error -> return 0; > 2) ufs_qcom_clk_scale_notify -> 'ufs_qcom_clk_scale_up_/down_pre_change' error -> return 0; > 3) ufs_qcom_init_lane_clks -> 'ufs_qcom_host_clk_get(tx_lane1_sync_clk)' error -> return 0; > > It is unknown whether the above commit was intended to change error handling. > However, if it is not an intended fix, a patch may be needed. I think you're right, these were not intentionally changed. There's a patch series in flight right now that cleans up some of this driver and inadvertently tackles some of the problems below. > > Signed-off-by: ChanWoo Lee <cw9316.lee@samsung.com> > --- > drivers/ufs/host/ufs-qcom.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 96cb8b5b4e66..8a93d93ab08f 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -313,6 +313,8 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) > > err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk", > &host->tx_l1_sync_clk, true); > + if (err) > + return err; This patch cleans this up: https://lore.kernel.org/linux-arm-msm/20231208065902.11006-2-manivannan.sadhasivam@linaro.org/ > } > > return 0; > @@ -404,9 +406,11 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba) > usleep_range(200, 210); > > ret = reset_control_deassert(host->core_reset); > - if (ret) > + if (ret) { > dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n", > __func__, ret); > + return ret; > + } This patch cleans this up: https://lore.kernel.org/linux-arm-msm/20231208065902.11006-8-manivannan.sadhasivam@linaro.org/#t > > usleep_range(1000, 1100); > > @@ -415,7 +419,7 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba) > hba->is_irq_enabled = true; > } > > - return 0; > + return ret; If I'm reading right returning ret is pointless here with your change above (it already returns ret, and it is no longer updated right so the only possible value here is 0? > } > > static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba) > @@ -1535,7 +1539,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, > ufshcd_uic_hibern8_exit(hba); > } > > - return 0; > + return err; > } I think you could move this one up into the PRE_CHANGE block and leave return 0 here? I believe this is the only case not yet covered by the patch series I linked. Good catch! > > static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host) > -- > 2.29.0 > >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 96cb8b5b4e66..8a93d93ab08f 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -313,6 +313,8 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host) err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk", &host->tx_l1_sync_clk, true); + if (err) + return err; } return 0; @@ -404,9 +406,11 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba) usleep_range(200, 210); ret = reset_control_deassert(host->core_reset); - if (ret) + if (ret) { dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n", __func__, ret); + return ret; + } usleep_range(1000, 1100); @@ -415,7 +419,7 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba) hba->is_irq_enabled = true; } - return 0; + return ret; } static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba) @@ -1535,7 +1539,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, ufshcd_uic_hibern8_exit(hba); } - return 0; + return err; } static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)