Re: [PATCH] staging: pi433: using div64_u64() instead of do_div()

[Dan Carpenter]

On Thu, Feb 10, 2022 at 09:21:08AM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 10 February 2022 08:06
> > 
> > On Wed, Feb 09, 2022 at 08:15:13PM +0100, Christophe JAILLET wrote:
> > > Le 21/01/2022 à 14:34, David Laight a écrit :
> > > > From: Jiapeng Chong
> > > > > Sent: 21 January 2022 11:50
> > > > > Subject: [PATCH] staging: pi433: using div64_u64() instead of do_div()
> > > > >
> > > > > Clean the following coccicheck warning:
> > > > >
> > > > > ./drivers/staging/pi433/rf69.c:286:1-7: WARNING: do_div() does a
> > > > > 64-by-32 division, please consider using div64_u64 instead.
> > > >
> > > > That is one of patchcheck's worse warnings.
> > > >
> > > > You need to check the domain of the divisor, not its type.
> > > >
> > > > do_div() exists to avoid expensive 64bit divides when the
> > > > divisor is small.
> > > >
> > > > 	David
> > > >
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > > Registration No: 1397386 (Wales)
> > > >
> > > >
> > >
> > > Moreover, the patch is broken by itself.
> > >
> > > See [1] were it was already reported that do_div() and div64_u64() don't
> > > have the same calling convention.
> > >
> > > Looks that div64_u64() and div64_ul() works the same way.
> > 
> > We could mark those as __must_check functions.
> 
> That, and some kind of AI system to filter out untested patches
> from (presumably) students who think that the output from these
> tools 'must be right'.
> 
> Same for all the patches for using swap(), min() LIST_HEAD() etc.
> They are at best churn and make the code harder to read.

Churn is kind of the whole point of staging.  Generally, churn is a net
positive for any subsystem.  It's good to get eyes on the code.

The truth is that I looked at this patch and thought "I don't know
what do_div() does" so I moved on.  This is the first time we've ever
recieved a staging patch to convert do_div().  Next time we will all
know the issues with do_div() better.

Adding a __must_check is a good safety measure as well.

I've looked at adding a Smatch check for ignoring the returns for
functions which have no side effects but I've never completed that
work...  :(

regards,
dan carpenter