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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux