diff mbox series

[v2,1/8] iio: core: Add opaque_struct_size() helper and use it

Message ID 20230721170022.3461-2-andriy.shevchenko@linux.intel.com (mailing list archive)
State Changes Requested
Headers show
Series iio: core: A few code cleanups and documentation fixes | expand

Commit Message

Andy Shevchenko July 21, 2023, 5 p.m. UTC 
Introduce opaque_struct_size() helper, which may be moved
to overflow.h in the future, and use it in the IIO core.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Comments

Andy Shevchenko July 21, 2023, 5:22 p.m. UTC | #1 
On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> Introduce opaque_struct_size() helper, which may be moved
> to overflow.h in the future, and use it in the IIO core.

...

> +#define opaque_struct_size(p, a, s)	({		\
> +	size_t _psize = sizeof(*(p));			\
> +	size_t _asize = ALIGN(_psize, (a));		\
> +	size_t _ssize = s;				\
> +	_ssize ? size_add(_asize, _ssize) : _psize;	\
> +})
> +
> +#define opaque_struct_data(p, a)			\
> +	((void *)(p) + ALIGN(sizeof(*(p)), (a)))

Hmm... This can potentially evaluate p twice.

Perhaps this variant is better:

#define opaque_struct_data(p, a)	ALIGN((p) + 1, (a)))

Besides, I don't think we should worry about @s evaluation in the main macro
which may be simplified to

#define opaque_struct_size(p, a, s)					\
	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))

Thoughts?
Andy Shevchenko July 21, 2023, 5:25 p.m. UTC | #2 
On Fri, Jul 21, 2023 at 08:22:35PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> > Introduce opaque_struct_size() helper, which may be moved
> > to overflow.h in the future, and use it in the IIO core.

...

> > +#define opaque_struct_size(p, a, s)	({		\
> > +	size_t _psize = sizeof(*(p));			\
> > +	size_t _asize = ALIGN(_psize, (a));		\
> > +	size_t _ssize = s;				\
> > +	_ssize ? size_add(_asize, _ssize) : _psize;	\
> > +})
> > +
> > +#define opaque_struct_data(p, a)			\
> > +	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
> 
> Hmm... This can potentially evaluate p twice.
> 
> Perhaps this variant is better:
> 
> #define opaque_struct_data(p, a)	ALIGN((p) + 1, (a)))
> 
> Besides, I don't think we should worry about @s evaluation in the main macro
> which may be simplified to
> 
> #define opaque_struct_size(p, a, s)					\
> 	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))
> 
> Thoughts?

Also we may leave the struct always be aligned which makes the above even
simpler (but might waste bytes if @s = 0).

#define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))

(with the respective documentation update).
Andy Shevchenko July 21, 2023, 5:27 p.m. UTC | #3 
On Fri, Jul 21, 2023 at 08:25:12PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 08:22:35PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> > > Introduce opaque_struct_size() helper, which may be moved
> > > to overflow.h in the future, and use it in the IIO core.

...

> > > +#define opaque_struct_size(p, a, s)	({		\
> > > +	size_t _psize = sizeof(*(p));			\
> > > +	size_t _asize = ALIGN(_psize, (a));		\
> > > +	size_t _ssize = s;				\
> > > +	_ssize ? size_add(_asize, _ssize) : _psize;	\
> > > +})
> > > +
> > > +#define opaque_struct_data(p, a)			\
> > > +	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
> > 
> > Hmm... This can potentially evaluate p twice.
> > 
> > Perhaps this variant is better:
> > 
> > #define opaque_struct_data(p, a)	ALIGN((p) + 1, (a)))
> > 
> > Besides, I don't think we should worry about @s evaluation in the main macro
> > which may be simplified to
> > 
> > #define opaque_struct_size(p, a, s)					\
> > 	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))
> > 
> > Thoughts?
> 
> Also we may leave the struct always be aligned which makes the above even
> simpler (but might waste bytes if @s = 0).
> 
> #define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
> 
> (with the respective documentation update).

Jonathan, this patch can be skipped and if you are okay with the rest, the rest
may be (clearly, I have just checked) applied without as there is no dependency.
Uwe Kleine-König July 21, 2023, 6:12 p.m. UTC | #4 
Hello Andy,

On Fri, Jul 21, 2023 at 08:25:12PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 08:22:35PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> > > Introduce opaque_struct_size() helper, which may be moved
> > > to overflow.h in the future, and use it in the IIO core.
> 
> ...
> 
> > > +#define opaque_struct_size(p, a, s)	({		\
> > > +	size_t _psize = sizeof(*(p));			\
> > > +	size_t _asize = ALIGN(_psize, (a));		\
> > > +	size_t _ssize = s;				\
> > > +	_ssize ? size_add(_asize, _ssize) : _psize;	\
> > > +})
> > > +
> > > +#define opaque_struct_data(p, a)			\
> > > +	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
> > 
> > Hmm... This can potentially evaluate p twice.

I don't think sizeof evaluates its parameter? It should only defer its
type and then calculate the respective size.

> > Perhaps this variant is better:
> > 
> > #define opaque_struct_data(p, a)	ALIGN((p) + 1, (a)))

Still I like this better.

> > Besides, I don't think we should worry about @s evaluation in the main macro
> > which may be simplified to
> > 
> > #define opaque_struct_size(p, a, s)					\
> > 	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))
> > 
> > Thoughts?
> 
> Also we may leave the struct always be aligned which makes the above even
> simpler (but might waste bytes if @s = 0).

Depending on the value of a, the bytes "wasted" here might not be saved
anyhow because the memory allocator will round up the allocation size to
a multiple of some chunk size anyhow.

> #define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
> 
> (with the respective documentation update).

That would be my favourite.

Possible users of this helper include:

	__spi_alloc_controller() in drivers/spi/spi.c
	alloc_netdev_mqs in net/core/dev.c

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c117f50d0cf3..6cca86fd0ef9 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1617,6 +1617,28 @@  const struct device_type iio_device_type = {
 	.release = iio_dev_release,
 };
 
+/**
+ * opaque_struct_size() - Calculate size of opaque structure with trailing aligned data.
+ * @p: Pointer to the opaque structure.
+ * @a: Alignment in bytes before trailing data.
+ * @s: Data size in bytes (can be 0).
+ *
+ * Calculates size of memory needed for structure of @p followed by the aligned data
+ * of size @s. When @s is 0, the alignment @a is not taken into account and the result
+ * is an equivalent to sizeof(*(@p)).
+ *
+ * Returns: Number of bytes needed or SIZE_MAX on overflow.
+ */
+#define opaque_struct_size(p, a, s)	({		\
+	size_t _psize = sizeof(*(p));			\
+	size_t _asize = ALIGN(_psize, (a));		\
+	size_t _ssize = s;				\
+	_ssize ? size_add(_asize, _ssize) : _psize;	\
+})
+
+#define opaque_struct_data(p, a)			\
+	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
+
 /**
  * iio_device_alloc() - allocate an iio_dev from a driver
  * @parent:		Parent device.
@@ -1628,19 +1650,13 @@  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	struct iio_dev *indio_dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev_opaque);
-	if (sizeof_priv) {
-		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN);
-		alloc_size += sizeof_priv;
-	}
-
+	alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
 	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
 	if (!iio_dev_opaque)
 		return NULL;
 
 	indio_dev = &iio_dev_opaque->indio_dev;
-	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);
 
 	indio_dev->dev.parent = parent;
 	indio_dev->dev.type = &iio_device_type;