RE: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

From: David Laight <David.Laight@ACULAB.COM>
To: "'sakari.ailus@linux.intel.com'" <sakari.ailus@linux.intel.com>
Cc: 'Mitali Borkar' <mitaliborkar810@gmail.com>,
	"bingbu.cao@intel.com" <bingbu.cao@intel.com>,
	"tian.shu.qiu@intel.com" <tian.shu.qiu@intel.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"linux-staging@lists.linux.dev" <linux-staging@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"outreachy-kernel@googlegroups.com" 
	<outreachy-kernel@googlegroups.com>,
	"mitali_s@me.iitr.ac.in" <mitali_s@me.iitr.ac.in>
Subject: RE: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)
Date: Tue, 13 Apr 2021 10:32:05 +0000	[thread overview]
Message-ID: <249c86809f374e13ac0be28c279eae7e@AcuMS.aculab.com> (raw)
In-Reply-To: <20210413095621.GQ3@paasikivi.fi.intel.com>

From: sakari.ailus@linux.intel.com
> Sent: 13 April 2021 10:56
> 
> Hi David,
> 
> On Tue, Apr 13, 2021 at 07:40:12AM +0000, David Laight wrote:
> > From: Mitali Borkar
> > > Sent: 12 April 2021 00:09
> > >
> > > This patch fixes the warning identified by checkpatch.pl by replacing
> > > __attribute__aligned(size) with __aligned(size)
> > >
> > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > ---
> > >  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
> > >  1 file changed, 37 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index 589d5ccee3a7..d95ca9ebfafb 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
> > >   */
> > >  struct ipu3_uapi_awb_raw_buffer {
> > >  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> > > -		__attribute__((aligned(32)));
> > > +		__aligned(32);
> > >  } __packed;
> >
> > WTF?
> >
> > It either has 1-byte alignment because it is just __u8,
> > 32-byte because of the aligned(32),
> > or 1 byte because of the outer packed.
> >
> > What alignment does this (and all the other) structures
> > actually need?
> 
> 32 as noted above. Here packed makes no difference though.

Bollocks - it ought to override the __aligned(32);

> Some of these structs are used embedded in other structs or alone. I
> haven't checked this one.
> 
> It's also possible to have __packed and __aligned() conflict (in which case
> a decent compiler would give you a warning) --- which does not happen
> currently AFAIK.

At least one compiler is objecting to some similar constructs.

> >
> > Specifying 'packed' isn't free.
> 
> It may be free if the packed alignment of the fields corresponds to
> architecture's packing. Here __aligned() is used to satisfy
> hardware alignment requirements and __packed is used to ensure the same
> memory layout independently of ABI rules.

No that isn't what packed is for.
'packed' also tells the compiler that the structure may 'exist' at
an unaligned address.
On many architectures this requires the compiler use byte sized
access and shifts for all members.

If you are worried that the compiler/ABI might have inserted
padding then add a compile-time assert on the structure size.
But most kernel code assumes that structures where everything
is on its natural boundary won't have any padding.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)