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

[Dan Carpenter <dan.carpenter@oracle.com>]

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