| Message ID | 20240228204919.3680786-6-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: core: New macros and making use of them | expand |
On Wed, 2024-02-28 at 22:41 +0200, Andy Shevchenko wrote: > We have two new helpers struct_size_with_data() and struct_data_pointer() > that we can utilize in iio_device_alloc(). Do it so. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/iio/industrialio-core.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 1986b3386307..223013725e32 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, > int sizeof_priv) > size_t alloc_size; > > if (sizeof_priv) > - alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN) + > sizeof_priv; > + alloc_size = struct_size_with_data(iio_dev_opaque, > IIO_DMA_MINALIGN, sizeof_priv); > else > alloc_size = sizeof(struct iio_dev_opaque); > > @@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, > int sizeof_priv) > indio_dev = &iio_dev_opaque->indio_dev; > > if (sizeof_priv) > - indio_dev->priv = (char *)iio_dev_opaque + > - ALIGN(sizeof(struct iio_dev_opaque), > IIO_DMA_MINALIGN); > + indio_dev->priv = struct_data_pointer(iio_dev_opaque, > IIO_DMA_MINALIGN); I'd +1 for implementing what Kees suggested in IIO. Only thing is (I think), we need to move struct iio_dev indioo_dev to the end of struct iio_dev_opaque. - Nuno Sá
On Thu, 29 Feb 2024 16:29:43 +0100 Nuno Sá <noname.nuno@gmail.com> wrote: > On Wed, 2024-02-28 at 22:41 +0200, Andy Shevchenko wrote: > > We have two new helpers struct_size_with_data() and struct_data_pointer() > > that we can utilize in iio_device_alloc(). Do it so. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > > --- > > drivers/iio/industrialio-core.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > > index 1986b3386307..223013725e32 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, > > int sizeof_priv) > > size_t alloc_size; > > > > if (sizeof_priv) > > - alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN) + > > sizeof_priv; > > + alloc_size = struct_size_with_data(iio_dev_opaque, > > IIO_DMA_MINALIGN, sizeof_priv); > > else > > alloc_size = sizeof(struct iio_dev_opaque); > > > > @@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, > > int sizeof_priv) > > indio_dev = &iio_dev_opaque->indio_dev; > > > > if (sizeof_priv) > > - indio_dev->priv = (char *)iio_dev_opaque + > > - ALIGN(sizeof(struct iio_dev_opaque), > > IIO_DMA_MINALIGN); > > + indio_dev->priv = struct_data_pointer(iio_dev_opaque, > > IIO_DMA_MINALIGN); > > I'd +1 for implementing what Kees suggested in IIO. Only thing is (I think), we > need to move struct iio_dev indioo_dev to the end of struct iio_dev_opaque. That is going to be messy and without horrible hacks (I think) add more padding we don't need. At the moment the struct iio_dev and the struct iio_dev_opaque are aligned as at the start of the structure. The priv data is aligned by padding the larger struct iio_dev_opaque, so if you want the priv handle to be to data defined in struct iio_dev you would need to add additional padding so that struct iio_dev_opaque { stuff... // this next __aligned() is implicit anyway because of the rules for // a structure always being aligned to the alignment of it's max aligned // element. struct iio_dev __aligned (IIO_DMA_ALIGN) { stuff u8 priv[] __aligned(IIO_DMA_ALIGN); } } How about using what Kees suggests on the iio_dev_opaque (which think is cleaner anyway as that's what we are allocating) and keeping the magic pointer to priv in the struct iio_dev; The compiler looses some visibility for iio_priv() accesses but can it do much with those anyway? They always get cast to a struct driver_specific * and getting the original allocation wrong is not easy to do as we pass that struct size in. Note, for others not aware of what is going on here, the priv pointer in iio_dev is to allow efficient static inline iio_priv() calls without needing to either make a function call, or expose the internals of the opaque structure in which the iio_dev and the priv data are embedded. Standard pattern is: struct driver_specific *bob; struct iio_dev *indio_dev = dev_iio_device_alloc(dev, sizeof(*bob)); // which allocates the iio_dev_opaque, but returns the contained iio_dev bob = iio_priv(indio_dev); So struct iio_dev_opaque { struct iio_dev indio_dev { stuff.. void *priv; }; stuff.. int priv_count; u8 priv[] __aligned(IIO_DMA_ALIGN) __counted_by(priv_count); } with indio_dev->priv = iio_dev_opaque->dev? This cleanups up a few IIO core bits but no impact outside them. Nice to have those cleanups. Is there any way to have that internal iio_dev->priv pointer associated with a __counted_by even though it's pointing elsewhere than a local variable sized trailing element? struct iio_dev { stuff u32 count; void *priv __counted_by(count); } compiles with gcc but without digging further I have no idea if it does anything useful! Jonathan > > - Nuno Sá > > >
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 1986b3386307..223013725e32 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) size_t alloc_size; if (sizeof_priv) - alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN) + sizeof_priv; + alloc_size = struct_size_with_data(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv); else alloc_size = sizeof(struct iio_dev_opaque); @@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) indio_dev = &iio_dev_opaque->indio_dev; if (sizeof_priv) - indio_dev->priv = (char *)iio_dev_opaque + - ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN); + indio_dev->priv = struct_data_pointer(iio_dev_opaque, IIO_DMA_MINALIGN); else indio_dev->priv = NULL;