[PATCH v3 0/4] iio: core: A few code cleanups and documentation fixes

[PATCH v3 0/4] iio: core: A few code cleanups and documentation fixes

[Andy Shevchenko]

Just set of almost unrelated to each other cleanups against IIO
core implementation.

The positive LoCs diffstat due to first patch that adds a lot of
documentation for the newly added macro and existing kernel doc.

v3:
- dropped applied patches
- use switch-case for the supported clocks (Jonathan)
- redone opaque_struct_size() to be simpler (Uwe)
- dropped wrong hunk for krealloc_array() conversion (Jonathan)
- dropped initcall move (Jonathan)

v2:
- sprintf() --> sysfs_emit() (Nuno)
- added tag (Nuno)

Andy Shevchenko (4):
  iio: core: Use sysfs_match_string() helper
  iio: core: Add opaque_struct_size() helper and use it
  iio: core: Switch to krealloc_array()
  iio: core: Fix issues and style of the comments

 drivers/iio/industrialio-core.c | 158 ++++++++++++++++++--------------
 1 file changed, 90 insertions(+), 68 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v3 1/4] iio: core: Use sysfs_match_string() helper

[Andy Shevchenko]

Use sysfs_match_string() helper instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 71 +++++++++++++++------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 5f337f59330c..b153adc5bc84 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1397,50 +1397,42 @@ static ssize_t label_show(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(label);
 
+static const char * const clock_names[] = {
+	[CLOCK_REALTIME]	 	= "realtime",
+	[CLOCK_MONOTONIC]	 	= "monotonic",
+	[CLOCK_PROCESS_CPUTIME_ID]	= "process_cputime_id",
+	[CLOCK_THREAD_CPUTIME_ID]	= "thread_cputime_id",
+	[CLOCK_MONOTONIC_RAW]	 	= "monotonic_raw",
+	[CLOCK_REALTIME_COARSE]	 	= "realtime_coarse",
+	[CLOCK_MONOTONIC_COARSE] 	= "monotonic_coarse",
+	[CLOCK_BOOTTIME]	 	= "boottime",
+	[CLOCK_REALTIME_ALARM]		= "realtime_alarm",
+	[CLOCK_BOOTTIME_ALARM]		= "boottime_alarm",
+	[CLOCK_SGI_CYCLE]		= "sgi_cycle",
+	[CLOCK_TAI]		 	= "tai",
+};
+
 static ssize_t current_timestamp_clock_show(struct device *dev,
 					    struct device_attribute *attr,
 					    char *buf)
 {
 	const struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	const clockid_t clk = iio_device_get_clock(indio_dev);
-	const char *name;
-	ssize_t sz;
 
 	switch (clk) {
 	case CLOCK_REALTIME:
-		name = "realtime\n";
-		sz = sizeof("realtime\n");
-		break;
 	case CLOCK_MONOTONIC:
-		name = "monotonic\n";
-		sz = sizeof("monotonic\n");
-		break;
 	case CLOCK_MONOTONIC_RAW:
-		name = "monotonic_raw\n";
-		sz = sizeof("monotonic_raw\n");
-		break;
 	case CLOCK_REALTIME_COARSE:
-		name = "realtime_coarse\n";
-		sz = sizeof("realtime_coarse\n");
-		break;
 	case CLOCK_MONOTONIC_COARSE:
-		name = "monotonic_coarse\n";
-		sz = sizeof("monotonic_coarse\n");
-		break;
 	case CLOCK_BOOTTIME:
-		name = "boottime\n";
-		sz = sizeof("boottime\n");
-		break;
 	case CLOCK_TAI:
-		name = "tai\n";
-		sz = sizeof("tai\n");
 		break;
 	default:
 		BUG();
 	}
 
-	memcpy(buf, name, sz);
-	return sz;
+	return sysfs_emit(buf, "%s\n", clock_names[clk]);
 }
 
 static ssize_t current_timestamp_clock_store(struct device *dev,
@@ -1450,22 +1442,23 @@ static ssize_t current_timestamp_clock_store(struct device *dev,
 	clockid_t clk;
 	int ret;
 
-	if (sysfs_streq(buf, "realtime"))
-		clk = CLOCK_REALTIME;
-	else if (sysfs_streq(buf, "monotonic"))
-		clk = CLOCK_MONOTONIC;
-	else if (sysfs_streq(buf, "monotonic_raw"))
-		clk = CLOCK_MONOTONIC_RAW;
-	else if (sysfs_streq(buf, "realtime_coarse"))
-		clk = CLOCK_REALTIME_COARSE;
-	else if (sysfs_streq(buf, "monotonic_coarse"))
-		clk = CLOCK_MONOTONIC_COARSE;
-	else if (sysfs_streq(buf, "boottime"))
-		clk = CLOCK_BOOTTIME;
-	else if (sysfs_streq(buf, "tai"))
-		clk = CLOCK_TAI;
-	else
+	ret = sysfs_match_string(clock_names, buf);
+	if (ret < 0)
+		return ret;
+	clk = ret;
+
+	switch (clk) {
+	case CLOCK_REALTIME:
+	case CLOCK_MONOTONIC:
+	case CLOCK_MONOTONIC_RAW:
+	case CLOCK_REALTIME_COARSE:
+	case CLOCK_MONOTONIC_COARSE:
+	case CLOCK_BOOTTIME:
+	case CLOCK_TAI:
+		break;
+	default:
 		return -EINVAL;
+	}
 
 	ret = iio_device_set_clock(dev_to_iio_dev(dev), clk);
 	if (ret)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v3 1/4] iio: core: Use sysfs_match_string() helper

[Jonathan Cameron]

On Mon, 24 Jul 2023 14:02:01 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Use sysfs_match_string() helper instead of open coded variant.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
I'm not loving the match against a larger list in order to then throw
away the ones we don't like, but I guess it is harmless and will make
reuse easier if this ever becomes more generally useful.

Hopefully it won't because no one else will think: "Let's provide
an interface that gives userspace the option to pick from many weird
clock choices".

In case you haven't guessed, that's one decision from a long time back
I wish I'd made differently.   2 choices would have been plenty..

Applied to the togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 71 +++++++++++++++------------------
>  1 file changed, 32 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 5f337f59330c..b153adc5bc84 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1397,50 +1397,42 @@ static ssize_t label_show(struct device *dev, struct device_attribute *attr,
>  
>  static DEVICE_ATTR_RO(label);
>  
> +static const char * const clock_names[] = {
> +	[CLOCK_REALTIME]	 	= "realtime",
> +	[CLOCK_MONOTONIC]	 	= "monotonic",
> +	[CLOCK_PROCESS_CPUTIME_ID]	= "process_cputime_id",
> +	[CLOCK_THREAD_CPUTIME_ID]	= "thread_cputime_id",
> +	[CLOCK_MONOTONIC_RAW]	 	= "monotonic_raw",
> +	[CLOCK_REALTIME_COARSE]	 	= "realtime_coarse",
> +	[CLOCK_MONOTONIC_COARSE] 	= "monotonic_coarse",
> +	[CLOCK_BOOTTIME]	 	= "boottime",
> +	[CLOCK_REALTIME_ALARM]		= "realtime_alarm",
> +	[CLOCK_BOOTTIME_ALARM]		= "boottime_alarm",
> +	[CLOCK_SGI_CYCLE]		= "sgi_cycle",
> +	[CLOCK_TAI]		 	= "tai",
> +};
> +
>  static ssize_t current_timestamp_clock_show(struct device *dev,
>  					    struct device_attribute *attr,
>  					    char *buf)
>  {
>  	const struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	const clockid_t clk = iio_device_get_clock(indio_dev);
> -	const char *name;
> -	ssize_t sz;
>  
>  	switch (clk) {
>  	case CLOCK_REALTIME:
> -		name = "realtime\n";
> -		sz = sizeof("realtime\n");
> -		break;
>  	case CLOCK_MONOTONIC:
> -		name = "monotonic\n";
> -		sz = sizeof("monotonic\n");
> -		break;
>  	case CLOCK_MONOTONIC_RAW:
> -		name = "monotonic_raw\n";
> -		sz = sizeof("monotonic_raw\n");
> -		break;
>  	case CLOCK_REALTIME_COARSE:
> -		name = "realtime_coarse\n";
> -		sz = sizeof("realtime_coarse\n");
> -		break;
>  	case CLOCK_MONOTONIC_COARSE:
> -		name = "monotonic_coarse\n";
> -		sz = sizeof("monotonic_coarse\n");
> -		break;
>  	case CLOCK_BOOTTIME:
> -		name = "boottime\n";
> -		sz = sizeof("boottime\n");
> -		break;
>  	case CLOCK_TAI:
> -		name = "tai\n";
> -		sz = sizeof("tai\n");
>  		break;
>  	default:
>  		BUG();
>  	}
>  
> -	memcpy(buf, name, sz);
> -	return sz;
> +	return sysfs_emit(buf, "%s\n", clock_names[clk]);
>  }
>  
>  static ssize_t current_timestamp_clock_store(struct device *dev,
> @@ -1450,22 +1442,23 @@ static ssize_t current_timestamp_clock_store(struct device *dev,
>  	clockid_t clk;
>  	int ret;
>  
> -	if (sysfs_streq(buf, "realtime"))
> -		clk = CLOCK_REALTIME;
> -	else if (sysfs_streq(buf, "monotonic"))
> -		clk = CLOCK_MONOTONIC;
> -	else if (sysfs_streq(buf, "monotonic_raw"))
> -		clk = CLOCK_MONOTONIC_RAW;
> -	else if (sysfs_streq(buf, "realtime_coarse"))
> -		clk = CLOCK_REALTIME_COARSE;
> -	else if (sysfs_streq(buf, "monotonic_coarse"))
> -		clk = CLOCK_MONOTONIC_COARSE;
> -	else if (sysfs_streq(buf, "boottime"))
> -		clk = CLOCK_BOOTTIME;
> -	else if (sysfs_streq(buf, "tai"))
> -		clk = CLOCK_TAI;
> -	else
> +	ret = sysfs_match_string(clock_names, buf);
> +	if (ret < 0)
> +		return ret;
> +	clk = ret;
> +
> +	switch (clk) {
> +	case CLOCK_REALTIME:
> +	case CLOCK_MONOTONIC:
> +	case CLOCK_MONOTONIC_RAW:
> +	case CLOCK_REALTIME_COARSE:
> +	case CLOCK_MONOTONIC_COARSE:
> +	case CLOCK_BOOTTIME:
> +	case CLOCK_TAI:
> +		break;
> +	default:
>  		return -EINVAL;
> +	}
>  
>  	ret = iio_device_set_clock(dev_to_iio_dev(dev), clk);
>  	if (ret)

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

[Andy Shevchenko]

Introduce opaque_struct_size() helper, which may be moved
to overflow.h in the future, and use it in the IIO core.

Potential users could be (among possible others):

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

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

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index b153adc5bc84..118ca6b59504 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1607,6 +1607,24 @@ 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 (preferred not to be 0).
+ *
+ * Calculates size of memory needed for structure of @p followed by
+ * the aligned data of size @s.
+ *
+ * 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.
+ *
+ * Returns: Number of bytes needed or SIZE_MAX on overflow.
+ */
+#define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
+
+#define opaque_struct_data(p, a)	PTR_ALIGN((void *)((p) + 1), (a))
+
 /**
  * iio_device_alloc() - allocate an iio_dev from a driver
  * @parent:		Parent device.
@@ -1618,19 +1636,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;
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 02:02:02PM +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.
> 
> Potential users could be (among possible others):
> 
> 	__spi_alloc_controller() in drivers/spi/spi.c
> 	alloc_netdev_mqs in net/core/dev.c

...

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

This actually might need something like __safe_aling() which takes care about
possible overflow.

Whatever, I want to hear Kees on this.

-- 
With Best Regards,
Andy Shevchenko

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

[Kees Cook]

On Mon, Jul 24, 2023 at 02:11:24PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 24, 2023 at 02:02:02PM +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.
> > 
> > Potential users could be (among possible others):
> > 
> > 	__spi_alloc_controller() in drivers/spi/spi.c
> > 	alloc_netdev_mqs in net/core/dev.c

Can you include the specific replacement you're thinking for these? It's
almost clear to me, but I'm trying to understand the benefit over what's
already there.

> 
> ...
> 
> > +#define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
> 
> This actually might need something like __safe_aling() which takes care about
> possible overflow.
> 
> Whatever, I want to hear Kees on this.

i.e. if "a" were huge? What would sanity-checking of "a" look like in
this case? I'm not really sure how to handle a pathological alignment
request, but I'd agree it'd be nice to handle it. :)

-Kees

-- 
Kees Cook

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

[Jonathan Cameron]

On Mon, 24 Jul 2023 14:02:02 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Introduce opaque_struct_size() helper, which may be moved
> to overflow.h in the future, and use it in the IIO core.
> 
> Potential users could be (among possible others):
> 
> 	__spi_alloc_controller() in drivers/spi/spi.c
> 	alloc_netdev_mqs in net/core/dev.c
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Kees Cook <keescook@chromium.org>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index b153adc5bc84..118ca6b59504 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1607,6 +1607,24 @@ 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 (preferred not to be 0).
> + *
> + * Calculates size of memory needed for structure of @p followed by
> + * the aligned data of size @s.
> + *
> + * 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.

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)) 

Or do it at the call site below.

> + *
> + * Returns: Number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define opaque_struct_size(p, a, s)	size_add(ALIGN(sizeof(*(p)), (a)), (s))
> +
> +#define opaque_struct_data(p, a)	PTR_ALIGN((void *)((p) + 1), (a))
> +
>  /**
>   * iio_device_alloc() - allocate an iio_dev from a driver
>   * @parent:		Parent device.
> @@ -1618,19 +1636,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;
> -	}
> -
	if (sizeof_priv)
		alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
	else
		alloc_size = sizeof(struct iio_dev_opaque);


> +	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);

Would have been safer if original code set this to NULL if
sizeof_priv == 0

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

Jonathan

>  
>  	indio_dev->dev.parent = parent;
>  	indio_dev->dev.type = &iio_device_type;

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

[Andy Shevchenko]

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

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

[Jonathan Cameron]

On Mon, 31 Jul 2023 23:01:15 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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?

In two case the size of the extra space this is putting on the end of
the opaque structure is 0.  I've not checked how bit the main structure is - maybe
it doesn't make any real difference. Ugly even so!

> 
> > 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.

Good point...

> 
> > 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;
> 
Agreed - that would be nicer

[PATCH v3 3/4] iio: core: Switch to krealloc_array()

[Andy Shevchenko]

Let the krealloc_array() copy the original data and
check for a multiplication overflow.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 118ca6b59504..13d6b6ac5ccf 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1474,7 +1474,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
 	const struct attribute_group **new, **old = iio_dev_opaque->groups;
 	unsigned int cnt = iio_dev_opaque->groupcounter;
 
-	new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
+	new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL);
 	if (!new)
 		return -ENOMEM;
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v3 3/4] iio: core: Switch to krealloc_array()

[Jonathan Cameron]

On Mon, 24 Jul 2023 14:02:03 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Let the krealloc_array() copy the original data and
> check for a multiplication overflow.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 118ca6b59504..13d6b6ac5ccf 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1474,7 +1474,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
>  	const struct attribute_group **new, **old = iio_dev_opaque->groups;
>  	unsigned int cnt = iio_dev_opaque->groupcounter;
>  
> -	new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
> +	new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL);
>  	if (!new)
>  		return -ENOMEM;
>  

[PATCH v3 4/4] iio: core: Fix issues and style of the comments

[Andy Shevchenko]

The `scripts/kernel-doc -v -none -Wall` reports several issues
with the kernel doc in IIO core C file. Update the comments
accordingly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 13d6b6ac5ccf..7302a342dd48 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* The industrial I/O core
+/*
+ * The industrial I/O core
  *
  * Copyright (c) 2008 Jonathan Cameron
  *
@@ -183,7 +184,9 @@ static const char * const iio_chan_info_postfix[] = {
  * @indio_dev:		Device structure whose ID is being queried
  *
  * The IIO device ID is a unique index used for example for the naming
- * of the character device /dev/iio\:device[ID]
+ * of the character device /dev/iio\:device[ID].
+ *
+ * Returns: Unique ID for the device.
  */
 int iio_device_id(struct iio_dev *indio_dev)
 {
@@ -196,6 +199,8 @@ EXPORT_SYMBOL_GPL(iio_device_id);
 /**
  * iio_buffer_enabled() - helper function to test if the buffer is enabled
  * @indio_dev:		IIO device structure for device
+ *
+ * Returns: True, if the buffer is enabled.
  */
 bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {
@@ -225,6 +230,9 @@ EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
  * @si:			scan index to match
+ *
+ * Returns:
+ * Constant pointer to iio_chan_spec, if scan index matches, NULL on failure.
  */
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si)
@@ -249,7 +257,9 @@ EXPORT_SYMBOL(iio_read_const_attr);
 /**
  * iio_device_set_clock() - Set current timestamping clock for the device
  * @indio_dev: IIO device structure containing the device
- * @clock_id: timestamping clock posix identifier to set.
+ * @clock_id: timestamping clock POSIX identifier to set.
+ *
+ * Returns: 0 on success, or a negative error code.
  */
 int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
 {
@@ -275,6 +285,8 @@ EXPORT_SYMBOL(iio_device_set_clock);
 /**
  * iio_device_get_clock() - Retrieve current timestamping clock for the device
  * @indio_dev: IIO device structure containing the device
+ *
+ * Returns: Clock ID of the current timestamping clock for the device.
  */
 clockid_t iio_device_get_clock(const struct iio_dev *indio_dev)
 {
@@ -287,6 +299,8 @@ EXPORT_SYMBOL(iio_device_get_clock);
 /**
  * iio_get_time_ns() - utility function to get a time stamp for events etc
  * @indio_dev: device
+ *
+ * Returns: Timestamp of the event in nanoseconds.
  */
 s64 iio_get_time_ns(const struct iio_dev *indio_dev)
 {
@@ -593,7 +607,7 @@ EXPORT_SYMBOL_GPL(iio_show_mount_matrix);
  * If device is assigned no mounting matrix property, a default 3x3 identity
  * matrix will be filled in.
  *
- * Return: 0 if success, or a negative error code on failure.
+ * Returns: 0 if success, or a negative error code on failure.
  */
 int iio_read_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix)
 {
@@ -691,9 +705,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
  * @vals:	Pointer to the values, exact meaning depends on the
  *		type parameter.
  *
- * Return: 0 by default, a negative number on failure or the
- *	   total number of characters written for a type that belongs
- *	   to the IIO_VAL_* constant.
+ * Returns:
+ * 0 by default, a negative number on failure or the total number of characters
+ * written for a type that belongs to the IIO_VAL_* constant.
  */
 ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
 {
@@ -846,8 +860,8 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
  * @fract: The fractional part of the number
  * @scale_db: True if this should parse as dB
  *
- * Returns 0 on success, or a negative error code if the string could not be
- * parsed.
+ * Returns:
+ * 0 on success, or a negative error code if the string could not be parsed.
  */
 static int __iio_str_to_fixpoint(const char *str, int fract_mult,
 				 int *integer, int *fract, bool scale_db)
@@ -916,8 +930,8 @@ static int __iio_str_to_fixpoint(const char *str, int fract_mult,
  * @integer: The integer part of the number
  * @fract: The fractional part of the number
  *
- * Returns 0 on success, or a negative error code if the string could not be
- * parsed.
+ * Returns:
+ * 0 on success, or a negative error code if the string could not be parsed.
  */
 int iio_str_to_fixpoint(const char *str, int fract_mult,
 			int *integer, int *fract)
@@ -1629,7 +1643,10 @@ const struct device_type iio_device_type = {
  * iio_device_alloc() - allocate an iio_dev from a driver
  * @parent:		Parent device.
  * @sizeof_priv:	Space to allocate for private structure.
- **/
+ *
+ * Returns:
+ * Pointer to allocated iio_dev on success, NULL on failure.
+ */
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 {
 	struct iio_dev_opaque *iio_dev_opaque;
@@ -1679,7 +1696,7 @@ EXPORT_SYMBOL(iio_device_alloc);
 /**
  * iio_device_free() - free an iio_dev from a driver
  * @dev:		the iio_dev associated with the device
- **/
+ */
 void iio_device_free(struct iio_dev *dev)
 {
 	if (dev)
@@ -1700,7 +1717,7 @@ static void devm_iio_device_release(void *iio_dev)
  * Managed iio_device_alloc. iio_dev allocated with this function is
  * automatically freed on driver detach.
  *
- * RETURNS:
+ * Returns:
  * Pointer to allocated iio_dev on success, NULL on failure.
  */
 struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
@@ -1727,8 +1744,8 @@ EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
  * @filp:	File structure for iio device used to keep and later access
  *		private data
  *
- * Return: 0 on success or -EBUSY if the device is already opened
- **/
+ * Returns: 0 on success or -EBUSY if the device is already opened
+ */
 static int iio_chrdev_open(struct inode *inode, struct file *filp)
 {
 	struct iio_dev_opaque *iio_dev_opaque =
@@ -1761,7 +1778,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
  * @inode:	Inode structure pointer for the char device
  * @filp:	File structure pointer for the char device
  *
- * Return: 0 for successful release
+ * Returns: 0 for successful release.
  */
 static int iio_chrdev_release(struct inode *inode, struct file *filp)
 {
@@ -1800,7 +1817,7 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 	mutex_lock(&iio_dev_opaque->info_exist_lock);
 
-	/**
+	/*
 	 * The NULL check here is required to prevent crashing when a device
 	 * is being removed while userspace would still have open file handles
 	 * to try to access this device.
@@ -1978,7 +1995,7 @@ EXPORT_SYMBOL(__iio_device_register);
 /**
  * iio_device_unregister() - unregister a device from the IIO subsystem
  * @indio_dev:		Device structure representing the device.
- **/
+ */
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
@@ -2029,7 +2046,7 @@ EXPORT_SYMBOL_GPL(__devm_iio_device_register);
  *
  * Use with iio_device_release_direct_mode()
  *
- * Returns: 0 on success, -EBUSY on failure
+ * Returns: 0 on success, -EBUSY on failure.
  */
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
 {
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v3 4/4] iio: core: Fix issues and style of the comments

[Jonathan Cameron]

On Mon, 24 Jul 2023 14:02:04 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The `scripts/kernel-doc -v -none -Wall` reports several issues
> with the kernel doc in IIO core C file. Update the comments
> accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

Applied to the togreg branch of iio.git, but initially pushed out as testing
to let 0-day see if it can find anything we missed.

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 13d6b6ac5ccf..7302a342dd48 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* The industrial I/O core
> +/*
> + * The industrial I/O core
>   *
>   * Copyright (c) 2008 Jonathan Cameron
>   *
> @@ -183,7 +184,9 @@ static const char * const iio_chan_info_postfix[] = {
>   * @indio_dev:		Device structure whose ID is being queried
>   *
>   * The IIO device ID is a unique index used for example for the naming
> - * of the character device /dev/iio\:device[ID]
> + * of the character device /dev/iio\:device[ID].
> + *
> + * Returns: Unique ID for the device.
>   */
>  int iio_device_id(struct iio_dev *indio_dev)
>  {
> @@ -196,6 +199,8 @@ EXPORT_SYMBOL_GPL(iio_device_id);
>  /**
>   * iio_buffer_enabled() - helper function to test if the buffer is enabled
>   * @indio_dev:		IIO device structure for device
> + *
> + * Returns: True, if the buffer is enabled.
>   */
>  bool iio_buffer_enabled(struct iio_dev *indio_dev)
>  {
> @@ -225,6 +230,9 @@ EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
>   * iio_find_channel_from_si() - get channel from its scan index
>   * @indio_dev:		device
>   * @si:			scan index to match
> + *
> + * Returns:
> + * Constant pointer to iio_chan_spec, if scan index matches, NULL on failure.
>   */
>  const struct iio_chan_spec
>  *iio_find_channel_from_si(struct iio_dev *indio_dev, int si)
> @@ -249,7 +257,9 @@ EXPORT_SYMBOL(iio_read_const_attr);
>  /**
>   * iio_device_set_clock() - Set current timestamping clock for the device
>   * @indio_dev: IIO device structure containing the device
> - * @clock_id: timestamping clock posix identifier to set.
> + * @clock_id: timestamping clock POSIX identifier to set.
> + *
> + * Returns: 0 on success, or a negative error code.
>   */
>  int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
>  {
> @@ -275,6 +285,8 @@ EXPORT_SYMBOL(iio_device_set_clock);
>  /**
>   * iio_device_get_clock() - Retrieve current timestamping clock for the device
>   * @indio_dev: IIO device structure containing the device
> + *
> + * Returns: Clock ID of the current timestamping clock for the device.
>   */
>  clockid_t iio_device_get_clock(const struct iio_dev *indio_dev)
>  {
> @@ -287,6 +299,8 @@ EXPORT_SYMBOL(iio_device_get_clock);
>  /**
>   * iio_get_time_ns() - utility function to get a time stamp for events etc
>   * @indio_dev: device
> + *
> + * Returns: Timestamp of the event in nanoseconds.
>   */
>  s64 iio_get_time_ns(const struct iio_dev *indio_dev)
>  {
> @@ -593,7 +607,7 @@ EXPORT_SYMBOL_GPL(iio_show_mount_matrix);
>   * If device is assigned no mounting matrix property, a default 3x3 identity
>   * matrix will be filled in.
>   *
> - * Return: 0 if success, or a negative error code on failure.
> + * Returns: 0 if success, or a negative error code on failure.
>   */
>  int iio_read_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix)
>  {
> @@ -691,9 +705,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
>   * @vals:	Pointer to the values, exact meaning depends on the
>   *		type parameter.
>   *
> - * Return: 0 by default, a negative number on failure or the
> - *	   total number of characters written for a type that belongs
> - *	   to the IIO_VAL_* constant.
> + * Returns:
> + * 0 by default, a negative number on failure or the total number of characters
> + * written for a type that belongs to the IIO_VAL_* constant.
>   */
>  ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
>  {
> @@ -846,8 +860,8 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
>   * @fract: The fractional part of the number
>   * @scale_db: True if this should parse as dB
>   *
> - * Returns 0 on success, or a negative error code if the string could not be
> - * parsed.
> + * Returns:
> + * 0 on success, or a negative error code if the string could not be parsed.
>   */
>  static int __iio_str_to_fixpoint(const char *str, int fract_mult,
>  				 int *integer, int *fract, bool scale_db)
> @@ -916,8 +930,8 @@ static int __iio_str_to_fixpoint(const char *str, int fract_mult,
>   * @integer: The integer part of the number
>   * @fract: The fractional part of the number
>   *
> - * Returns 0 on success, or a negative error code if the string could not be
> - * parsed.
> + * Returns:
> + * 0 on success, or a negative error code if the string could not be parsed.
>   */
>  int iio_str_to_fixpoint(const char *str, int fract_mult,
>  			int *integer, int *fract)
> @@ -1629,7 +1643,10 @@ const struct device_type iio_device_type = {
>   * iio_device_alloc() - allocate an iio_dev from a driver
>   * @parent:		Parent device.
>   * @sizeof_priv:	Space to allocate for private structure.
> - **/
> + *
> + * Returns:
> + * Pointer to allocated iio_dev on success, NULL on failure.
> + */
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque;
> @@ -1679,7 +1696,7 @@ EXPORT_SYMBOL(iio_device_alloc);
>  /**
>   * iio_device_free() - free an iio_dev from a driver
>   * @dev:		the iio_dev associated with the device
> - **/
> + */
>  void iio_device_free(struct iio_dev *dev)
>  {
>  	if (dev)
> @@ -1700,7 +1717,7 @@ static void devm_iio_device_release(void *iio_dev)
>   * Managed iio_device_alloc. iio_dev allocated with this function is
>   * automatically freed on driver detach.
>   *
> - * RETURNS:
> + * Returns:
>   * Pointer to allocated iio_dev on success, NULL on failure.
>   */
>  struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)
> @@ -1727,8 +1744,8 @@ EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
>   * @filp:	File structure for iio device used to keep and later access
>   *		private data
>   *
> - * Return: 0 on success or -EBUSY if the device is already opened
> - **/
> + * Returns: 0 on success or -EBUSY if the device is already opened
> + */
>  static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque =
> @@ -1761,7 +1778,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>   * @inode:	Inode structure pointer for the char device
>   * @filp:	File structure pointer for the char device
>   *
> - * Return: 0 for successful release
> + * Returns: 0 for successful release.
>   */
>  static int iio_chrdev_release(struct inode *inode, struct file *filp)
>  {
> @@ -1800,7 +1817,7 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  
>  	mutex_lock(&iio_dev_opaque->info_exist_lock);
>  
> -	/**
> +	/*
>  	 * The NULL check here is required to prevent crashing when a device
>  	 * is being removed while userspace would still have open file handles
>  	 * to try to access this device.
> @@ -1978,7 +1995,7 @@ EXPORT_SYMBOL(__iio_device_register);
>  /**
>   * iio_device_unregister() - unregister a device from the IIO subsystem
>   * @indio_dev:		Device structure representing the device.
> - **/
> + */
>  void iio_device_unregister(struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> @@ -2029,7 +2046,7 @@ EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>   *
>   * Use with iio_device_release_direct_mode()
>   *
> - * Returns: 0 on success, -EBUSY on failure
> + * Returns: 0 on success, -EBUSY on failure.
>   */
>  int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
>  {