| Message ID | 20231214021401.26474-1-cw9316.lee@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] scsi: ufs: qcom: Re-fix for error handling | expand |
On Thu, Dec 14, 2023 at 11:14:01AM +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")' > > This is the case I checked. > * ufs_qcom_clk_scale_notify -> 'ufs_qcom_clk_scale_up_/down_pre_change' 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. > Fixes: 031312dbc695 ("scsi: ufs: ufs-qcom: Remove unnecessary goto statements") > Signed-off-by: ChanWoo Lee <cw9316.lee@samsung.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks for spotting and fixing the issue! This is one of the reasons why the error path should directly return instead of slipping. - Mani > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > --- > v1->v2: Remove things already in progress > 1) ufs_qcom_host_reset -> 'reset_control_deassert' error -> return 0; > -> https://lore.kernel.org/linux-arm-msm/20231208065902.11006-8-manivannan.sadhasivam@linaro.org/#t > 2) ufs_qcom_init_lane_clks -> 'ufs_qcom_host_clk_get(tx_lane1_sync_clk)' error -> return 0; > -> https://lore.kernel.org/linux-arm-msm/20231208065902.11006-2-manivannan.sadhasivam@linaro.org/ > --- > --- > drivers/ufs/host/ufs-qcom.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 96cb8b5b4e66..17e24270477d 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1516,9 +1516,11 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, > err = ufs_qcom_clk_scale_up_pre_change(hba); > else > err = ufs_qcom_clk_scale_down_pre_change(hba); > - if (err) > - ufshcd_uic_hibern8_exit(hba); > > + if (err) { > + ufshcd_uic_hibern8_exit(hba); > + return err; > + } > } else { > if (scale_up) > err = ufs_qcom_clk_scale_up_post_change(hba); > -- > 2.29.0 > >
On Thu, Dec 14, 2023 at 11:14:01AM +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")' > > This is the case I checked. > * ufs_qcom_clk_scale_notify -> 'ufs_qcom_clk_scale_up_/down_pre_change' 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. Can you be a bit specific about what you fixed here in the commit? Both the subject and the description is vague and sounds like you're still unsure if this change is a good idea. The review on the prior patch and this one is indicating that this change is necessary and a fix, so let's be more confident in the description for future readers. Write as you please, but something like: scsi: ufs: qcom: Return ufs_qcom_clk_scale_*() errors in ufs_qcom_clk_scale_notify() In commit 031312dbc695 ("scsi: ufs: ufs-qcom: Remove unnecessary goto statements") the error handling was accidentally changed, resulting in the error of ufs_qcom_clk_scale_*() calls not being returned. Let's make sure those errors are properly returned. > > Signed-off-by: ChanWoo Lee <cw9316.lee@samsung.com> This deserves a Fixes: tag (I see Mani mentioned that) > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> I did not provide a Reviewed-by tag in v1, this is not accurate: https://lore.kernel.org/linux-arm-msm/m5wjp3yb3qpheyzgipekeagycboifqdpw54nquzqsftufap3yc@kxjwi4y63adj/ Outside of that this looks good, thanks! > --- > v1->v2: Remove things already in progress > 1) ufs_qcom_host_reset -> 'reset_control_deassert' error -> return 0; > -> https://lore.kernel.org/linux-arm-msm/20231208065902.11006-8-manivannan.sadhasivam@linaro.org/#t > 2) ufs_qcom_init_lane_clks -> 'ufs_qcom_host_clk_get(tx_lane1_sync_clk)' error -> return 0; > -> https://lore.kernel.org/linux-arm-msm/20231208065902.11006-2-manivannan.sadhasivam@linaro.org/ > --- > --- > drivers/ufs/host/ufs-qcom.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 96cb8b5b4e66..17e24270477d 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1516,9 +1516,11 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, > err = ufs_qcom_clk_scale_up_pre_change(hba); > else > err = ufs_qcom_clk_scale_down_pre_change(hba); > - if (err) > - ufshcd_uic_hibern8_exit(hba); > > + if (err) { > + ufshcd_uic_hibern8_exit(hba); > + return err; > + } > } else { > if (scale_up) > err = ufs_qcom_clk_scale_up_post_change(hba); > -- > 2.29.0 >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 96cb8b5b4e66..17e24270477d 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -1516,9 +1516,11 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, err = ufs_qcom_clk_scale_up_pre_change(hba); else err = ufs_qcom_clk_scale_down_pre_change(hba); - if (err) - ufshcd_uic_hibern8_exit(hba); + if (err) { + ufshcd_uic_hibern8_exit(hba); + return err; + } } else { if (scale_up) err = ufs_qcom_clk_scale_up_post_change(hba);