Re: [PATCH 3/3] media: uapi: Use unsigned int values for assigning bits in u32 fields

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Mon, May 08, 2023 at 09:58:48AM +0300, Sakari Ailus wrote:
> On Mon, May 08, 2023 at 09:30:23AM +0300, Laurent Pinchart wrote:
> > On Mon, May 08, 2023 at 09:19:24AM +0300, Sakari Ailus wrote:
> > > On Sat, May 06, 2023 at 02:32:23PM +0300, Laurent Pinchart wrote:
> > > > On Fri, May 05, 2023 at 11:51:01PM +0300, Sakari Ailus wrote:
> > > > > Use unsigned int values annoted by "U" for u32 fields. While this is a
> > > > > good practice, there doesn't appear to be a bug that this patch would fix.
> > > > > 
> > > > > The patch has been generated using the following command:
> > > > > 
> > > > > 	perl -i -pe 's/\([0-9]+\K <</U <</g' -- include/uapi/linux/media.h
> > > > 
> > > > How about using the _BITUL() macro from include/uapi/linux/const.h ?
> > > 
> > > These are u32 whereas _BITUL makes an unsigned long. Int (as in U) is 32
> > > bits on all platforms where Linux is used AFAIK.
> > 
> > I know, but is it a problem ?
> 
> If we have a u32 field, unsigned int is the right type for that (from
> non-fixed length C types), not unsigned long. In practice it would work, I
> have no doubts about that. The compiler could still do different decisions
> due to this, promoting values to a 64-bits for instance.
> 
> If we had _BITU(), I'd be happy to use that. :-)

Note how BIT() is defined in include/vdso/bits.h:

#include <vdso/const.h>

#define BIT(nr)                 (UL(1) << (nr))

And in include/vdso/const.h:

#include <uapi/linux/const.h>

#define UL(x)           (_UL(x))

BIT() is thus essentially identical to _BITUL(). As we use the former
everywhere without any trouble, I wouldn't expect issue with the latter.

> How about this: let's merge this patch and then see how a _BITU() macro
> would fare.

-- 
Regards,

Laurent Pinchart