Re: [PATCH v4 5/8] iio: core: Use new helpers from overflow.h in iio_device_alloc()

[Jonathan Cameron <jic23@xxxxxxxxxx>]

On Thu, 29 Feb 2024 16:29:43 +0100
Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > ---
> >  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á
> 
> 
>