linux-kernel - Re: [PATCH] staging: media: atomisp: Use BIT macro instead of left shifting

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]

Message-ID: <YfVstOJ38OTtd43n@smile.fi.intel.com>
Date:   Sat, 29 Jan 2022 18:35:00 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Moses Christopher Bollavarapu <mosescb.dev@...il.com>
Cc:     linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        Tsuchiya Yuto <kitakar@...il.com>
Subject: Re: [PATCH] staging: media: atomisp: Use BIT macro instead of left
 shifting

On Sat, Jan 29, 2022 at 12:38:04PM +0100, Moses Christopher Bollavarapu wrote:

It's a bit too many people in Cc list (I dropped more than a half in this reply).
I would suggest to use my script [1] or look into it for a hint how to reduce it.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

> There is a BIT(nr) macro available in Linux Kernel,
> which does the same thing.

In some cases it might fix a (theoretical) UB issue.

> Example: BIT(7) = (1UL << 7)

...

> +		s_config->source.tpg.x_mask = BIT(4) - 1;

> +		s_config->source.tpg.y_mask = BIT(4) - 1;

> +		s_config->source.tpg.xy_mask = BIT(8) - 1;

For masks you may use GENMASK().

...

> -		irq = irq & 1 << INTR_IIR;
> +		irq = irq & BIT(INTR_IIR);

It may be:

	irq &= BIT(...);

...

> -	irq = irq & 1 << INTR_IIR;
> +	irq = irq & BIT(INTR_IIR);

Ditto.

...

> -		virt += (1 << PAGE_SHIFT);
> +		virt += BIT(PAGE_SHIFT);

This is weird. Shouldn't be as simple as

	virt += PAGE_SIZE;

?

...

>  enum ia_css_sp_sleep_mode {
>  	SP_DISABLE_SLEEP_MODE = 0,
> -	SP_SLEEP_AFTER_FRAME = 1 << 0,
> -	SP_SLEEP_AFTER_IRQ = 1 << 1
> +	SP_SLEEP_AFTER_FRAME  = BIT(0),

> +	SP_SLEEP_AFTER_IRQ    = BIT(1)

While at it, add a comma here.

>  };

...

> +	IA_CSS_DEBUG_DUMP_ALL = BIT(14)  /** Dump all device parameters */

Ditto.

...

On top of this don't forget to add

	#include <linux/bits.h>

to the modified header files.

-- 
With Best Regards,
Andy Shevchenko