iio: core: A few code cleanups and documentation fixes

PATCH Applied Andy Shevchenko 4 participants 13 messages Jul 24, 2023 – Aug 1, 2023 IIO

lore

Share & tools

lore

Andy Shevchenko

author

2023-07-24 11:02 UTC #0 lore raw

v3 cover iio: core: A few code cleanups and documentation fixes

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

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-07-24 11:02 UTC #1 lore raw

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

Use sysfs_match_string() helper instead of open coded variant.

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Nuno Sa <[email protected]>

---
 drivers/iio/industrialio-core.c | 71 +++++++++++++++------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

drivers/iio/industrialio-core.c +32 ~~-39~~

index 5f337f59330c..b153adc5bc84 100644
@@ -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)

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer Applied

2023-07-29 11:37 UTC #2 lore raw

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

Quoted 4 lines · On Mon, 24 Jul 2023 14:02:01 +0300 Andy Shevchenko…

Use sysfs_match_string() helper instead of open coded variant.
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Nuno Sa <[email protected]>

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

Quoted 114 lines

---
 drivers/iio/industrialio-core.c | 71 +++++++++++++++------------------
 1 file changed, 32 insertions(+), 39 deletions(-)

drivers/iio/industrialio-core.c +32 ~~-39~~

index 5f337f59330c..b153adc5bc84 100644
@@ -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)

Andy Shevchenko

author

2023-07-24 11:02 UTC #3 lore raw

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

In reply to 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 <[email protected]>

Cc Uwe Kleine-König <[email protected]>

Cc Kees Cook <[email protected]>

Reviewed-by Nuno Sa <[email protected]>

---
 drivers/iio/industrialio-core.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

drivers/iio/industrialio-core.c +20 -8

index b153adc5bc84..118ca6b59504 100644
@@ -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;

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-07-24 11:11 UTC #4 lore raw

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

Quoted 7 lines · 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

...

Quoted 1 line

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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Kees Cook

2023-07-27 18:16 UTC #5 lore raw

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

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · 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

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.

Quoted 9 lines

...
Quoted 1 line
+#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

Signature · 1 line

Kees Cook

Jonathan Cameron

maintainer

2023-07-29 11:46 UTC #6 lore raw

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

In reply to Andy Shevchenko

Quoted 25 lines · On Mon, 24 Jul 2023 14:02:02 +0300 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 <[email protected]>
Cc Uwe Kleine-König <[email protected]>
Cc Kees Cook <[email protected]>
Reviewed-by Nuno Sa <[email protected]>
---
 drivers/iio/industrialio-core.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)
drivers/iio/industrialio-core.c +1 -0
index b153adc5bc84..118ca6b59504 100644
@@ -1607,6 +1607,24 @@ const struct device_type iio_device_type = {
 	.release = iio_dev_release,
 };
 
+/**

drivers/iio/industrialio-core.c const struct device_type iio_device_type = { · line 1620

Quoted 10 lines

drivers/iio/industrialio-core.c +10 -0

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

Quoted 11 lines

drivers/iio/industrialio-core.c +7 -0

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

drivers/iio/industrialio-core.c struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) · line 1626

Quoted 9 lines

drivers/iio/industrialio-core.c +0 -6

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

drivers/iio/industrialio-core.c struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) · line 1645

Quoted 9 lines

drivers/iio/industrialio-core.c +2 -2

+	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

Quoted 3 lines

drivers/iio/industrialio-core.c +0 -0

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

Andy Shevchenko

author

2023-07-31 20:01 UTC #7 lore raw

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

Quoted 2 lines · Jonathan Cameron

On Mon, 24 Jul 2023 14:02:02 +0300
Andy Shevchenko <[email protected]> wrote:

...

Quoted 5 lines

Quoted 2 lines
+ * 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?

Quoted 3 lines

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.

Quoted 1 line

Or do it at the call site below.

Looks much better to me.

...

Quoted 4 lines

	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.

...

Quoted 6 lines

drivers/iio/industrialio-core.c struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) · line 1645
Quoted 3 lines
drivers/iio/industrialio-core.c +1 -2
-	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.

Quoted 2 lines

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;

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jonathan Cameron

2023-08-01 16:45 UTC #8 lore raw

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

Quoted 6 lines · On Mon, 31 Jul 2023 23:01:15 +0300 Andy Shevchenko…

Quoted 2 lines · Jonathan Cameron
On Mon, 24 Jul 2023 14:02:02 +0300
Andy Shevchenko <[email protected]> wrote:
...

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
+ * 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!

Quoted 6 lines

Quoted 4 lines
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...

Quoted 28 lines

Quoted 2 lines
Or do it at the call site below.
Looks much better to me.
...
Quoted 4 lines
	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.
...
Quoted 6 lines
drivers/iio/industrialio-core.c struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) · line 1645
Quoted 3 lines
drivers/iio/industrialio-core.c +1 -2
-	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.
Quoted 2 lines
A driver doing that should never have used iio_priv() but nicer if it was NULL rather than off the end of the allocation.

Quoted 8 lines

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

Andy Shevchenko

author

2023-07-24 11:02 UTC #9 lore raw

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

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Nuno Sa <[email protected]>

---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/industrialio-core.c +1 -1

index 118ca6b59504..13d6b6ac5ccf 100644
@@ -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;

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer Applied

2023-07-29 11:48 UTC #10 lore raw

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

Quoted 5 lines · On Mon, 24 Jul 2023 14:02:03 +0300 Andy Shevchenko…

Let the krealloc_array() copy the original data and check for a multiplication overflow.
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Nuno Sa <[email protected]>

Applied

Thanks,

Jonathan

Quoted 17 lines

---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/industrialio-core.c +1 -1

index 118ca6b59504..13d6b6ac5ccf 100644
@@ -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;

Andy Shevchenko

author

2023-07-24 11:02 UTC #11 lore raw

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

In reply to 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 <[email protected]>

Reviewed-by Nuno Sa <[email protected]>

Reviewed-by Randy Dunlap <[email protected]>

---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

drivers/iio/industrialio-core.c +37 ~~-20~~

index 13d6b6ac5ccf..7302a342dd48 100644
@@ -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)
 {

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer

2023-07-29 11:49 UTC #12 lore raw

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

Quoted 7 lines · On Mon, 24 Jul 2023 14:02:04 +0300 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 <[email protected]>
Reviewed-by Nuno Sa <[email protected]>
Reviewed-by Randy Dunlap <[email protected]>

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

Quoted 196 lines

---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

drivers/iio/industrialio-core.c +37 ~~-20~~

index 13d6b6ac5ccf..7302a342dd48 100644
@@ -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)
 {