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@xxxxxxxxxx> > 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
