Re: [PATCH v3 2/4] iio: core: Add opaque_struct_size() helper and use it

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Jul 29, 2023 at 12:46:18PM +0100, Jonathan Cameron wrote:
> On Mon, 24 Jul 2023 14:02:02 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p))
> > + * and the result, depending on the @a, may be way off the initial size.
> 
> How often is this true?  A quick and dirty grep suggests at least 2 so perhaps
> worth retaining the old behaviour.

You mean that the sizeof(_some_grepped_struct_) is much less than an alignment
in those uses?

> Can we take that into account?  Maybe something like
> 
> #define opaque_struct_size(p, a, s) ((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)): sizeof(*p)) 

(s) will be evaluated twice, not good. So, not in this form.

> Or do it at the call site below.

Looks much better to me.

...

> 	if (sizeof_priv)
> 		alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
> 	else
> 		alloc_size = sizeof(struct iio_dev_opaque);

Right.

...

> > -	indio_dev->priv = (char *)iio_dev_opaque +
> > -		ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> > +	indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN);
> 
> Would have been safer if original code set this to NULL if
> sizeof_priv == 0

Yeah, original code and proposed change has no difference in this sense.

> A driver doing that should never have used iio_priv() but nicer if it was
> NULL rather than off the end of the allocation.

Agree.
But looking at the above, I would rather see that in a form of

	if (...)
		priv = opaque_struct_data(...);
	else
		priv = NULL;

-- 
With Best Regards,
Andy Shevchenko