| Message ID | YpXD4nLc4iCxpw91@kili (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: ufs: clean up ufshpb_check_hpb_reset_query() | expand |
> Smatch complains that the if (flag_res) is not required: > > drivers/ufs/core/ufshpb.c:2306 ufshpb_check_hpb_reset_query() > warn: duplicate check 'flag_res' (previous on line 2301) > > Re-write the "if (flag_res)" checking to be more clear. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> In HPB Reset, the Host set this flag as '1' to inform the device that host reset its L2P data. The Device resets flag as '0' when the device inactivated all region information. 0h: HPB reset completed or not started yet. 1h: HPB reset in progress. Would make sense to me to contain this logic within this function, Instead of returning just the flag value. Thanks, Avri > --- > drivers/ufs/core/ufshpb.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/ufs/core/ufshpb.c b/drivers/ufs/core/ufshpb.c > index fb122eaed28b..95b501b824df 100644 > --- a/drivers/ufs/core/ufshpb.c > +++ b/drivers/ufs/core/ufshpb.c > @@ -2299,17 +2299,15 @@ static bool ufshpb_check_hpb_reset_query(struct > ufs_hba *hba) > } > > if (!flag_res) > - goto out; > + return false; > > usleep_range(1000, 1100); > } > - if (flag_res) { > - dev_err(hba->dev, > - "%s fHpbReset was not cleared by the device\n", > - __func__); > - } > -out: > - return flag_res; > + > + dev_err(hba->dev, > + "%s fHpbReset was not cleared by the device\n", > + __func__); > + return true; > } > > /** > -- > 2.35.1
On Wed, Jun 01, 2022 at 06:25:01AM +0000, Avri Altman wrote: > > Smatch complains that the if (flag_res) is not required: > > > > drivers/ufs/core/ufshpb.c:2306 ufshpb_check_hpb_reset_query() > > warn: duplicate check 'flag_res' (previous on line 2301) > > > > Re-write the "if (flag_res)" checking to be more clear. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > In HPB Reset, the Host set this flag as '1' to inform the device that host reset its L2P data. > The Device resets flag as '0' when the device inactivated all region information. > 0h: HPB reset completed or not started yet. > 1h: HPB reset in progress. > > Would make sense to me to contain this logic within this function, > Instead of returning just the flag value. > > Thanks, > Avri I am not sure I understand. To be honest, this function is not beautiful at all. With boolean functions, the name should tell you what the return means. Examples are: if (!access_ok()), if (IS_ERR() etc. In this case the return is not clear from the name. The second thing is that I really don't like returning true for failure and returning false for success. Returning zero and negatives is good but with true/false it should be true == success. So, yes, I wasn't super happy with this function either. But I just did a minimal clean up to make the returns more clear. If you want to drop this patch and write a more extensive one then I would be really happy about that. regards, dan carpenter
> On Wed, Jun 01, 2022 at 06:25:01AM +0000, Avri Altman wrote: > > > Smatch complains that the if (flag_res) is not required: > > > > > > drivers/ufs/core/ufshpb.c:2306 ufshpb_check_hpb_reset_query() > > > warn: duplicate check 'flag_res' (previous on line 2301) > > > > > > Re-write the "if (flag_res)" checking to be more clear. > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > In HPB Reset, the Host set this flag as '1' to inform the device that host > reset its L2P data. > > The Device resets flag as '0' when the device inactivated all region > information. > > 0h: HPB reset completed or not started yet. > > 1h: HPB reset in progress. > > > > Would make sense to me to contain this logic within this function, > > Instead of returning just the flag value. > > > > Thanks, > > Avri > > I am not sure I understand. > > To be honest, this function is not beautiful at all. With boolean functions, the > name should tell you what the return means. Examples > are: if (!access_ok()), if (IS_ERR() etc. In this case the return is not clear from > the name. Right. > > The second thing is that I really don't like returning true for failure and > returning false for success. Returning zero and negatives is good but with > true/false it should be true == success. Exactly. > > So, yes, I wasn't super happy with this function either. But I just did a > minimal clean up to make the returns more clear. If you want to drop this > patch and write a more extensive one then I would be really happy about > that. Yeah - I will. Thanks, Avri > > regards, > dan carpenter
diff --git a/drivers/ufs/core/ufshpb.c b/drivers/ufs/core/ufshpb.c index fb122eaed28b..95b501b824df 100644 --- a/drivers/ufs/core/ufshpb.c +++ b/drivers/ufs/core/ufshpb.c @@ -2299,17 +2299,15 @@ static bool ufshpb_check_hpb_reset_query(struct ufs_hba *hba) } if (!flag_res) - goto out; + return false; usleep_range(1000, 1100); } - if (flag_res) { - dev_err(hba->dev, - "%s fHpbReset was not cleared by the device\n", - __func__); - } -out: - return flag_res; + + dev_err(hba->dev, + "%s fHpbReset was not cleared by the device\n", + __func__); + return true; } /**
Smatch complains that the if (flag_res) is not required: drivers/ufs/core/ufshpb.c:2306 ufshpb_check_hpb_reset_query() warn: duplicate check 'flag_res' (previous on line 2301) Re-write the "if (flag_res)" checking to be more clear. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/ufs/core/ufshpb.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)