[PATCH] scsi: ufs: clean up ufshpb_check_hpb_reset_query()

[PATCH] scsi: ufs: clean up ufshpb_check_hpb_reset_query()

[Dan Carpenter]

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(-)

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

RE: [PATCH] scsi: ufs: clean up ufshpb_check_hpb_reset_query()

[Avri Altman]

> 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

Re: [PATCH] scsi: ufs: clean up ufshpb_check_hpb_reset_query()

[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.

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

RE: [PATCH] scsi: ufs: clean up ufshpb_check_hpb_reset_query()

[Avri Altman]

> 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