linux-kernel - Re: [PATCH] staging: comedi: Improved readability of function comedi_nsamples

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Jun 2018 11:56:16 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     chris <eklikeroomys@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, Frank Mori Hess <fmh6jj@...il.com>,
        Ian Abbott <abbotti@....co.uk>,
        Simo Koskinen <koskisoft@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: comedi: Improved readability of function
 comedi_nsamples_left.

You haven't been sending the v2 and v3 patches in the right way.

Take a look through the email archive and see how this is normally done.
Also google it:
https://www.google.com/search?q=how+to+send+a+v2+patch

On Sun, Jun 10, 2018 at 12:30:01PM +0200, chris wrote:
> Hi Greg,
> I've added changelog text to the patch below. Appreciate your 
> feedback!
> Regards,
> Chris Opperman
> 
> >8----------------------------------------------------------------------8<
> 
> Improved the readability of comedi_nsamples_left:
> a) Reduced nesting by using more return calls.

nit: return isn't a function so it can't be called.

> b) Separated variable declarations from code.

It was separate in the original code too.

> c) Using kernel integer types.

The original types were fine/better.

The rules are that we don't like uint32_t and the reason for not liking
it is that we looked through the kernel code and standardized on what
was most common...

Generally u32 and friends mean that the hardware or network protocol
specifies the number of bits.  That's especially true for s32.  If you
see code using s32 when it's not tied to a hardware spec then that's a
sign that the code is written by an insane person and it's a tricky
situation to deal with.

Some other reasons to use u32 is because it's shorter to type than
unsigned int and because of line lengths...  I've sometimes done that.
But it's perhaps lazy.

> 
> Signed-off-by: Chris Opperman <eklikeroomys@...il.com>
> ---
>  drivers/staging/comedi/drivers.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> index 9d73347..3207ae2 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -468,26 +468,25 @@ EXPORT_SYMBOL_GPL(comedi_nscans_left);
>   * Returns the number of samples remaining to complete the command, or the
>   * specified expected number of samples (@nsamples), whichever is fewer.
>   */
> -unsigned int comedi_nsamples_left(struct comedi_subdevice *s,
> -				  unsigned int nsamples)
> +u32 comedi_nsamples_left(struct comedi_subdevice *s, u32 nsamples)
>  {
>  	struct comedi_async *async = s->async;
>  	struct comedi_cmd *cmd = &async->cmd;
> +	u32 scans_left;

I would make scans_left an unsigned long long so that you can remove
the cast when you do the multiply.

> +	u64 samples_left;

Nothing wrong with unsigned long long.

regards,
dan carpenter