| Message ID | 20200525170628.503283-20-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: 2nd set of timestamp alignment fixes. | expand |
On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > One of a class of bugs pointed out by Lars in a recent review. > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned > to the size of the timestamp (8 bytes). This is not guaranteed in > this driver which uses an array of smaller elements on the stack. > > Here we use an explicit structure and rely on that to enforce > alignment on the stack. Note there was never a data leak here > due to the explicit memset. > > Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support") > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/iio/adc/ti-ads1015.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c > index 5ea4f45d6bad..05853723dbdb 100644 > --- a/drivers/iio/adc/ti-ads1015.c > +++ b/drivers/iio/adc/ti-ads1015.c > @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p) > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > struct ads1015_data *data = iio_priv(indio_dev); > - s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding + 4x s16 timestamp */ > + /* Ensure natural alignment for buffer elements */ > + struct { > + s16 channel; > + s64 ts; > + } scan; Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from iio_push_to_buffers_with_timestamp() point of view? > int chan, ret, res; > > - memset(buf, 0, sizeof(buf)); > + memset(&scan, 0, sizeof(scan)); > > mutex_lock(&data->lock); > chan = find_first_bit(indio_dev->active_scan_mask, > @@ -399,10 +403,10 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p) > goto err; > } > > - buf[0] = res; > + scan.channel = res; > mutex_unlock(&data->lock); > > - iio_push_to_buffers_with_timestamp(indio_dev, buf, > + iio_push_to_buffers_with_timestamp(indio_dev, &scan, > iio_get_time_ns(indio_dev)); > > err: > -- > 2.26.2 >
On 5/25/20 7:52 PM, Andy Shevchenko wrote: > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote: >> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> >> One of a class of bugs pointed out by Lars in a recent review. >> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned >> to the size of the timestamp (8 bytes). This is not guaranteed in >> this driver which uses an array of smaller elements on the stack. >> >> Here we use an explicit structure and rely on that to enforce >> alignment on the stack. Note there was never a data leak here >> due to the explicit memset. >> >> Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support") >> Reported-by: Lars-Peter Clausen <lars@metafoo.de> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/iio/adc/ti-ads1015.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c >> index 5ea4f45d6bad..05853723dbdb 100644 >> --- a/drivers/iio/adc/ti-ads1015.c >> +++ b/drivers/iio/adc/ti-ads1015.c >> @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p) >> struct iio_poll_func *pf = p; >> struct iio_dev *indio_dev = pf->indio_dev; >> struct ads1015_data *data = iio_priv(indio_dev); >> - s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding + 4x s16 timestamp */ >> + /* Ensure natural alignment for buffer elements */ >> + struct { >> + s16 channel; >> + s64 ts; >> + } scan; > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from > iio_push_to_buffers_with_timestamp() point of view? No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks like we can't rely on implicit padding, but need to always explicitly specify it. Or maybe we can typedef and IIO timestamp type with an explicit __aligned attribute. I wonder if that works... After having a quick look, the kernel already defines aligned_u64, so maybe using that is an option.
On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote: > On 5/25/20 7:52 PM, Andy Shevchenko wrote: > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> ... > > > - s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding + 4x s16 timestamp */ > > > + /* Ensure natural alignment for buffer elements */ > > > + struct { > > > + s16 channel; > > > + s64 ts; > > > + } scan; > > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from > > iio_push_to_buffers_with_timestamp() point of view? > > No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks > like we can't rely on implicit padding, but need to always explicitly > specify it. > > Or maybe we can typedef and IIO timestamp type with an explicit __aligned > attribute. I wonder if that works... After having a quick look, the kernel > already defines aligned_u64, so maybe using that is an option. Another way is simple to provide offset of timestamp member as a parameter. Though, if it's an ABI, then alas, we need to align it properly. Also, wouldn't be better to explicitly show the padding? struct { s16 channel; s16 padding[3]; s64 ts; } scan; (matter of style though, just saying).
On Tue, 26 May 2020 12:15:56 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote: > > On 5/25/20 7:52 PM, Andy Shevchenko wrote: > > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > ... > > > > > - s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding + 4x s16 timestamp */ > > > > + /* Ensure natural alignment for buffer elements */ > > > > + struct { > > > > + s16 channel; > > > > + s64 ts; > > > > + } scan; > > > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from > > > iio_push_to_buffers_with_timestamp() point of view? > > > > No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks > > like we can't rely on implicit padding, but need to always explicitly > > specify it. > > > > Or maybe we can typedef and IIO timestamp type with an explicit __aligned > > attribute. I wonder if that works... After having a quick look, the kernel > > already defines aligned_u64, so maybe using that is an option. > > Another way is simple to provide offset of timestamp member as a parameter. > Though, if it's an ABI, then alas, we need to align it properly. > > Also, wouldn't be better to explicitly show the padding? > > struct { > s16 channel; > s16 padding[3]; > s64 ts; > } scan; > > (matter of style though, just saying). > gah. Thanks for pointing this out Andy. I wanted to avoid explicitly calling out empty padding because it seemed to me to be more likely to be error prone than filling it in. I was trying to avoid using __aligned on the stack as it only works for more recent kernels (due to gcc version changes) and some of these predate that point. I guess we just do it explicitly in all these cases. The two patches that have already gone to Greg both have sufficient data to ensure the structure is big enough (only 16 bytes padding in one and none in the other). I think we are also fine for the original question as well as it won't matter if the whole structure is aligned to 4 bytes on x86_32 and similar as an 8 byte write will be fine. So fun question - do we want to enforce 8 byte alignment of the whole structure, or simply the padding? Maybe better to just do the padding explicitly as Andy suggested. Jonathan
On Tue, May 26, 2020 at 05:43:28PM +0100, Jonathan Cameron wrote: > On Tue, 26 May 2020 12:15:56 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote: > > > On 5/25/20 7:52 PM, Andy Shevchenko wrote: > > > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron wrote: > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> ... > > > > > + struct { > > > > > + s16 channel; > > > > > + s64 ts; > > > > > + } scan; > > > > Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from > > > > iio_push_to_buffers_with_timestamp() point of view? > > > > > > No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks > > > like we can't rely on implicit padding, but need to always explicitly > > > specify it. > > > > > > Or maybe we can typedef and IIO timestamp type with an explicit __aligned > > > attribute. I wonder if that works... After having a quick look, the kernel > > > already defines aligned_u64, so maybe using that is an option. > > > > Another way is simple to provide offset of timestamp member as a parameter. > > Though, if it's an ABI, then alas, we need to align it properly. > > > > Also, wouldn't be better to explicitly show the padding? > > > > struct { > > s16 channel; > > s16 padding[3]; > > s64 ts; > > } scan; > > > > (matter of style though, just saying). > > > > gah. Thanks for pointing this out Andy. I wanted to avoid explicitly > calling out empty padding because it seemed to me to be more likely to > be error prone than filling it in. > > I was trying to avoid using __aligned on the stack as it only works for > more recent kernels (due to gcc version changes) and some of these predate > that point. > > I guess we just do it explicitly in all these cases. > > The two patches that have already gone to Greg both have sufficient > data to ensure the structure is big enough (only 16 bytes padding in one and > none in the other). > > I think we are also fine for the original question as well as it won't > matter if the whole structure is aligned to 4 bytes on x86_32 and > similar as an 8 byte write will be fine. > > So fun question - do we want to enforce 8 byte alignment of the whole > structure, or simply the padding? > > Maybe better to just do the padding explicitly as Andy suggested. I have talked to colleague of mine, and we concluded (but without any documentation proved evidence, one needs basically to read C standard followed by ABI of all architectures supported by Linux) that the following will work. Consider your patch, which introduces natural alignment via struct: struct scan { s16 ...; ... s64 ts; }; When we access ts as struct member like scan->ts, compiler makes sure that there will be no hardware exception (due to unaligned access). Now, we _assume_ that dereferencing like void *buf = &scan; (int64_t *)buf[ts_offset] = value; will work flawlessly because above. If it's indeed a case, what we simple need is to pass ts offset into iio_push_to_buffers_with_timestamp(). If it's not the case, we _additionally_ will need to replace (int64_t *)buf[ts_offset] = value; by put_unaligned(value, (int64_t *)...);
On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >On Tue, May 26, 2020 at 05:43:28PM +0100, Jonathan Cameron wrote: >> On Tue, 26 May 2020 12:15:56 +0300 >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> > On Tue, May 26, 2020 at 10:11:44AM +0200, Lars-Peter Clausen wrote: >> > > On 5/25/20 7:52 PM, Andy Shevchenko wrote: >> > > > On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron >wrote: >> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >... > >> > > > > + struct { >> > > > > + s16 channel; >> > > > > + s64 ts; >> > > > > + } scan; >> > > > Hmm... On x86_32 and x86_64 this will give different padding. >Is it okay from >> > > > iio_push_to_buffers_with_timestamp() point of view? >> > > >> > > No, this is terrible. IIO expects 64 bit words to be 64 bit >aligned. Looks >> > > like we can't rely on implicit padding, but need to always >explicitly >> > > specify it. >> > > >> > > Or maybe we can typedef and IIO timestamp type with an explicit >__aligned >> > > attribute. I wonder if that works... After having a quick look, >the kernel >> > > already defines aligned_u64, so maybe using that is an option. >> > >> > Another way is simple to provide offset of timestamp member as a >parameter. >> > Though, if it's an ABI, then alas, we need to align it properly. >> > >> > Also, wouldn't be better to explicitly show the padding? >> > >> > struct { >> > s16 channel; >> > s16 padding[3]; >> > s64 ts; >> > } scan; >> > >> > (matter of style though, just saying). >> > >> >> gah. Thanks for pointing this out Andy. I wanted to avoid >explicitly >> calling out empty padding because it seemed to me to be more likely >to >> be error prone than filling it in. >> >> I was trying to avoid using __aligned on the stack as it only works >for >> more recent kernels (due to gcc version changes) and some of these >predate >> that point. >> >> I guess we just do it explicitly in all these cases. >> >> The two patches that have already gone to Greg both have sufficient >> data to ensure the structure is big enough (only 16 bytes padding in >one and >> none in the other). >> >> I think we are also fine for the original question as well as it >won't >> matter if the whole structure is aligned to 4 bytes on x86_32 and >> similar as an 8 byte write will be fine. >> >> So fun question - do we want to enforce 8 byte alignment of the whole >> structure, or simply the padding? >> >> Maybe better to just do the padding explicitly as Andy suggested. > >I have talked to colleague of mine, and we concluded (but without any >documentation proved evidence, one needs basically to read C standard >followed >by ABI of all architectures supported by Linux) that the following will >work. > >Consider your patch, which introduces natural alignment via struct: > > struct scan { > s16 ...; > ... > s64 ts; > }; > >When we access ts as struct member like scan->ts, compiler makes sure >that >there will be no hardware exception (due to unaligned access). Now, we >_assume_ >that dereferencing like > > void *buf = &scan; > (int64_t *)buf[ts_offset] = value; > >will work flawlessly because above. > >If it's indeed a case, what we simple need is to pass ts offset into >iio_push_to_buffers_with_timestamp(). Realistically it might work but doesn't help us. We need all receivers of that buffer to know the location of ts. It has always been implicit like any other channel. Rules are natural alignment so alignment is same as size of element. Hence we need to ensure padding, but I'm not sure about alignment. If padding was right for 8 byte alignment but whole structure had 4 byte alignment then I think we should be fine. So slightly more relaxed the ensuring ts is 8 byte aligned. Might be easier to just align it though than explain this subtlety. J > >If it's not the case, we _additionally_ will need to replace > (int64_t *)buf[ts_offset] = value; >by > put_unaligned(value, (int64_t *)...);
On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote: > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Might be easier to just align it though than explain this subtlety. The easiest one seems to move ts to be first member of the struct / buffer. Problem solved.
On Wed, 27 May 2020 00:03:13 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote: > > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Might be easier to just align it though than explain this subtlety. > > The easiest one seems to move ts to be first member of the struct / buffer. Problem solved. Sort of, but it brings other problems. 1) Breaks userspace ABI. Yes, no one should be relying on the ordering of elements from a given sensor, but we all know someone will be. As things currently stand note we only have one actual report of there being a case where the alignment stuff breaks down (before I broke it much more significantly with this patch set!) 2) We have to rework all the drivers, including the majority that use a suitably aligned buffer anyway (typically ____cacheline_aligned for DMA safety). The structure thing doesn't work any more because the timestamp is optional, so we have to have the drivers do their alignment differently depending on whether it is there or not, so we are back to use using __aligned(8) for all the buffers. At the end of the day, the iio_push_to_buffers_with_timestamp is only workable because the timestamp is at the end of the buffer. At this point I'm thinking we just stick to u8, u16 etc arrays. At that point we either decide that we are happy to not backport past (IIRC) 4.19 where the minimum gcc version was increased such that __aligned(8) works on the stack, or we move them into the iio_priv() structure where __aligned(8) always worked and hence squash the issue of kernel data leaking without a memset on each scan. The only remaining question is whether we get extra clarity by using struct { s16 channels[2]; // I think we can actually drop the padding if marking the timestamp as // __aligned(8) u8 padding[4]; s64 timestamp __aligned(8); } scan; or s16 scan[8] __aligned(8); For the __aligned part this from the gcc docs looks helpful: https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Type-Attributes.html "Note that the alignment of any given struct or union type is required by the ISO C standard to be at least a perfect multiple of the lowest common multiple of the alignments of all of the members of the struct or union in question. This means that you can effectively adjust the alignment of a struct or union type by attaching an aligned attribute to any one of the members of such a type, but the notation illustrated in the example above is a more obvious, intuitive, and readable way to request the compiler to adjust the alignment of an entire struct or union type. " So I think that means we can safely do struct { u8 channel; s64 ts __aligned(8); }; and be guaranteed correct padding and alignment in what I think is a fairly readable form. It's also self documenting so I can probably drop some of the explanatory comments. Jonathan
On Wed, May 27, 2020 at 12:41:07PM +0100, Jonathan Cameron wrote: > On Wed, 27 May 2020 00:03:13 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote: > > > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Might be easier to just align it though than explain this subtlety. > > > > The easiest one seems to move ts to be first member of the struct / buffer. Problem solved. > > Sort of, but it brings other problems. > > 1) Breaks userspace ABI. Yes, no one should be relying on the ordering of > elements from a given sensor, but we all know someone will be. As > things currently stand note we only have one actual report of there > being a case where the alignment stuff breaks down (before I broke it > much more significantly with this patch set!) > > 2) We have to rework all the drivers, including the majority that use a suitably > aligned buffer anyway (typically ____cacheline_aligned for DMA safety). > The structure thing doesn't work any more because the timestamp is optional, > so we have to have the drivers do their alignment differently depending on whether > it is there or not, so we are back to use using __aligned(8) for all the buffers. > At the end of the day, the iio_push_to_buffers_with_timestamp is only workable > because the timestamp is at the end of the buffer. I see. > At this point I'm thinking we just stick to u8, u16 etc arrays. At that point > we either decide that we are happy to not backport past (IIRC) 4.19 where the minimum gcc > version was increased such that __aligned(8) works on the stack, or we move them into > the iio_priv() structure where __aligned(8) always worked and hence squash the issue > of kernel data leaking without a memset on each scan. The only remaining question is > whether we get extra clarity by using > > struct { > s16 channels[2]; > // I think we can actually drop the padding if marking the timestamp as > // __aligned(8) > u8 padding[4]; > s64 timestamp __aligned(8); > } scan; > > or > > s16 scan[8] __aligned(8); > > > For the __aligned part this from the gcc docs looks helpful: > > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Type-Attributes.html > > "Note that the alignment of any given struct or union type is > required by the ISO C standard to be at least a perfect multiple > of the lowest common multiple of the alignments of all of the > members of the struct or union in question. This means that you > can effectively adjust the alignment of a struct or union type by > attaching an aligned attribute to any one of the members of such a > type, but the notation illustrated in the example above is a more > obvious, intuitive, and readable way to request the compiler to > adjust the alignment of an entire struct or union type. " This means that struct sx { u8 channel; s64 ts; }; struct sx y, *py = &y; py will be always aligned to at least 8 bytes (per s64 type). It has no effect on the members themselves. > So I think that means we can safely do > > struct { > u8 channel; > s64 ts __aligned(8); > }; I don't know how this attribute will affect *a member* of the struct. Perhaps it's straightforward and GCC simple apply it to POD. > and be guaranteed correct padding and alignment in what I think is > a fairly readable form. It's also self documenting so I can > probably drop some of the explanatory comments. If it is documented, then I fully agree, otherwise will look like a (fragile) hack.
On Wed, May 27, 2020 at 03:37:13PM +0300, Andy Shevchenko wrote: > On Wed, May 27, 2020 at 12:41:07PM +0100, Jonathan Cameron wrote: > > On Wed, 27 May 2020 00:03:13 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > So I think that means we can safely do > > > > struct { > > u8 channel; > > s64 ts __aligned(8); > > }; > > I don't know how this attribute will affect *a member* of the struct. Perhaps > it's straightforward and GCC simple apply it to POD. > > > and be guaranteed correct padding and alignment in what I think is > > a fairly readable form. It's also self documenting so I can > > probably drop some of the explanatory comments. > > If it is documented, then I fully agree, otherwise will look > like a (fragile) hack. I meant here if __aligned() behaviour against POD inside the structure is documented in GCC manuals / etc.
On Wed, 27 May 2020 15:37:13 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, May 27, 2020 at 12:41:07PM +0100, Jonathan Cameron wrote: > > On Wed, 27 May 2020 00:03:13 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, May 26, 2020 at 08:17:11PM +0100, Jonathan Cameron wrote: > > > > On 26 May 2020 18:06:12 BST, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > Might be easier to just align it though than explain this subtlety. > > > > > > The easiest one seems to move ts to be first member of the struct / buffer. Problem solved. > > > > Sort of, but it brings other problems. > > > > 1) Breaks userspace ABI. Yes, no one should be relying on the ordering of > > elements from a given sensor, but we all know someone will be. As > > things currently stand note we only have one actual report of there > > being a case where the alignment stuff breaks down (before I broke it > > much more significantly with this patch set!) > > > > 2) We have to rework all the drivers, including the majority that use a suitably > > aligned buffer anyway (typically ____cacheline_aligned for DMA safety). > > The structure thing doesn't work any more because the timestamp is optional, > > so we have to have the drivers do their alignment differently depending on whether > > it is there or not, so we are back to use using __aligned(8) for all the buffers. > > At the end of the day, the iio_push_to_buffers_with_timestamp is only workable > > because the timestamp is at the end of the buffer. > > I see. > > > At this point I'm thinking we just stick to u8, u16 etc arrays. At that point > > we either decide that we are happy to not backport past (IIRC) 4.19 where the minimum gcc > > version was increased such that __aligned(8) works on the stack, or we move them into > > the iio_priv() structure where __aligned(8) always worked and hence squash the issue > > of kernel data leaking without a memset on each scan. The only remaining question is > > whether we get extra clarity by using > > > > struct { > > s16 channels[2]; > > // I think we can actually drop the padding if marking the timestamp as > > // __aligned(8) > > u8 padding[4]; > > s64 timestamp __aligned(8); > > } scan; > > > > or > > > > s16 scan[8] __aligned(8); > > > > > > For the __aligned part this from the gcc docs looks helpful: > > > > https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Type-Attributes.html > > > > "Note that the alignment of any given struct or union type is > > required by the ISO C standard to be at least a perfect multiple > > of the lowest common multiple of the alignments of all of the > > members of the struct or union in question. This means that you > > can effectively adjust the alignment of a struct or union type by > > attaching an aligned attribute to any one of the members of such a > > type, but the notation illustrated in the example above is a more > > obvious, intuitive, and readable way to request the compiler to > > adjust the alignment of an entire struct or union type. " > > This means that > > struct sx { > u8 channel; > s64 ts; > }; > > struct sx y, *py = &y; > > py will be always aligned to at least 8 bytes (per s64 type). > It has no effect on the members themselves. Ah. I cut too much out. "This attribute specifies a minimum alignment (in bytes) for variables of the specified type." So taking... struct sx { u8 channel; s64 ts __aligned(8); }; So if ts is __aligned(8) it will be aligned to 8 bytes, but so will the structure containing it. hence will will have 7 bytes of padding. There are a few similar users of this in kernel. For example https://elixir.bootlin.com/linux/latest/source/include/linux/efi.h#L809 Which after digging in the UEFI spec is because all variables in UEFI are defined to have natural alignment and the structures are aligned to the largest size. So effectively same case we have. Lots of other cases, that one just had a convenient spec to point at. They mostly exist in uapi headers presumably to allow stuff to consistent across from x86_64 to x86_32 userspace. > > > So I think that means we can safely do > > > > struct { > > u8 channel; > > s64 ts __aligned(8); > > }; > > I don't know how this attribute will affect *a member* of the struct. Perhaps > it's straightforward and GCC simple apply it to POD. > > > and be guaranteed correct padding and alignment in what I think is > > a fairly readable form. It's also self documenting so I can > > probably drop some of the explanatory comments. > > If it is documented, then I fully agree, otherwise will look > like a (fragile) hack. > Agreed. It seems to be documented and I'm reassured by the bunch of existing places this seems to be being used. Also helps that they are mostly for uapi consistency which is really want we are talking about here as well. Will let this sit for a few days before I do anything about a v2 in the hope that someone will point out any more corner cases I should have known about! Jonathan
diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c index 5ea4f45d6bad..05853723dbdb 100644 --- a/drivers/iio/adc/ti-ads1015.c +++ b/drivers/iio/adc/ti-ads1015.c @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p) struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; struct ads1015_data *data = iio_priv(indio_dev); - s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding + 4x s16 timestamp */ + /* Ensure natural alignment for buffer elements */ + struct { + s16 channel; + s64 ts; + } scan; int chan, ret, res; - memset(buf, 0, sizeof(buf)); + memset(&scan, 0, sizeof(scan)); mutex_lock(&data->lock); chan = find_first_bit(indio_dev->active_scan_mask, @@ -399,10 +403,10 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p) goto err; } - buf[0] = res; + scan.channel = res; mutex_unlock(&data->lock); - iio_push_to_buffers_with_timestamp(indio_dev, buf, + iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev)); err: