[PATCH 0/2] iio: consumers: ensure read buffers for labels and ext_info are page aligned

[PATCH 0/2] iio: consumers: ensure read buffers for labels and ext_info are page aligned

[Matteo Martelli]

This patch series is a follow up of [1], where I described an issue
related to the fact that devm_k*alloc() functions do not provide
alignment to the requested power of two size, leading to potential
errors when used together with sysfs_emit* helpers which expect
page-aligned buffers.

From that discussion, it became clear that this issue currently only
affects iio consumer drivers, as they can directly access providers
attribute formatted using sysfs_emit* helpers. For instance, the iio-mux
driver allocates a buffer with devm_kzalloc(PAGE_SIZE) to read provider
ext_info attributes, which could be handled via sysfs_emit* helpers.
This leads to an error in the provider ext_info read callback since the
allocated buffer is not page-aligned.

Summary:
- Patch 1: harden the consumers APIs to ensure read buffers are page
  aligned for attributes which could be formatted with sysfs_emit*
  helpers by the providers. Currently labels and ext_info attributes.

- Patch 2: fix iio-mux consumer by switching from devm_kzalloc to
  kzalloc for the ext_info buffer.

Tested with the iio-mux consumer driver alongside the pac1921 driver,
which provides an ext_info attribute (the shunt resistor in this case).
After applying patch-1, the error was detected during the iio-mux probe
rather than in the pac1921 ext_info read callback. After applying
patch-2, the error condition no longer occurred. Additionally, the extra
check in iio_read_channel_label() was tested with the iio_hwmon consumer
driver temporarily modified to allocate the buffer for retrieving
provider labels using devm_kzalloc(PAGE_SIZE) instead of
devm_get_free_pages(). The error was correctly detected during the
iio_hwmon probe when attempting to retrieve pac1921 channel labels.

[1]: https://lore.kernel.org/all/c486a1cf98a8b9ad093270543e8d2007@gmail.com

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
Matteo Martelli (2):
      iio: consumers: ensure read buffers for labels and ext_info are page aligned
      iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

 drivers/iio/inkern.c              | 11 +++++
 drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
 include/linux/iio/consumer.h      |  4 +-
 3 files changed, 59 insertions(+), 40 deletions(-)
---
base-commit: 20fd1383cd616d61b2a79967da1221dc6cfb8430
change-id: 20241127-iio-kmalloc-align-ac7bfcfe5ec0

Best regards,
-- 
Matteo Martelli <matteomartelli3@gmail.com>

[PATCH 1/2] iio: consumers: ensure read buffers for labels and ext_info are page aligned

[Matteo Martelli]

Attributes of iio providers are exposed via sysfs. Typically, providers
pass attribute values to the iio core, which handles formatting and
printing to sysfs. However, some attributes, such as labels or extended
info, are directly formatted and printed to sysfs by provider drivers
using sysfs_emit() and sysfs_emit_at(). These helpers assume the read
buffer, allocated by sysfs fop, is page-aligned. When these attributes
are accessed by consumer drivers, the read buffer is allocated by the
consumer and may not be page-aligned, leading to failures in the
provider's callback that utilizes sysfs_emit*.

Add a check to ensure that read buffers for labels and external info
attributes are page-aligned. Update the prototype documentation as well.

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
 drivers/iio/inkern.c         | 11 +++++++++++
 include/linux/iio/consumer.h |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 7f325b3ed08fae6674245312cf8f57bb151006c0..63707ed98e1d7aca1e446122bbf69c85c0dd06a2 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -7,6 +7,7 @@
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/minmax.h>
+#include <linux/mm.h>
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/slab.h>
@@ -989,6 +990,11 @@ ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
 {
 	const struct iio_chan_spec_ext_info *ext_info;
 
+	if (!buf || offset_in_page(buf)) {
+		pr_err("iio: invalid ext_info read buffer\n");
+		return -EINVAL;
+	}
+
 	ext_info = iio_lookup_ext_info(chan, attr);
 	if (!ext_info)
 		return -EINVAL;
@@ -1014,6 +1020,11 @@ EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
 
 ssize_t iio_read_channel_label(struct iio_channel *chan, char *buf)
 {
+	if (!buf || offset_in_page(buf)) {
+		pr_err("iio: invalid label read buffer\n");
+		return -EINVAL;
+	}
+
 	return do_iio_read_channel_label(chan->indio_dev, chan->channel, buf);
 }
 EXPORT_SYMBOL_GPL(iio_read_channel_label);
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 333d1d8ccb37f387fe531577ac5e0bfc7f752cec..6a44796164792b2dd930f8168b14de327a80a6f7 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -418,7 +418,7 @@ unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan);
  * @chan:		The channel being queried.
  * @attr:		The ext_info attribute to read.
  * @buf:		Where to store the attribute value. Assumed to hold
- *			at least PAGE_SIZE bytes.
+ *			at least PAGE_SIZE bytes and to be aligned at PAGE_SIZE.
  *
  * Returns the number of bytes written to buf (perhaps w/o zero termination;
  * it need not even be a string), or an error code.
@@ -445,7 +445,7 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
  * iio_read_channel_label() - read label for a given channel
  * @chan:		The channel being queried.
  * @buf:		Where to store the attribute value. Assumed to hold
- *			at least PAGE_SIZE bytes.
+ *			at least PAGE_SIZE bytes and to be aligned at PAGE_SIZE.
  *
  * Returns the number of bytes written to buf, or an error code.
  */

-- 
2.47.0

[PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Matteo Martelli]

During channel configuration, the iio-mux driver allocates a page with
devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
buffer points to an offset of the page due to the devres header sitting
at the beginning of the allocated area. This leads to failure in the
provider driver when sysfs_emit* helpers are used to format the ext_info
attributes.

Switch to plain kzalloc version. The devres version is not strictly
necessary as the buffer is only accessed during the channel
configuration phase. Rely on __free cleanup to deallocate the buffer.
Also, move the ext_info handling into a new function to have the page
buffer definition and assignment in one statement as suggested by
cleanup documentation.

Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
---
 drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..c309d991490c63ba4299f1cda7102f10dcf54982 100644
--- a/drivers/iio/multiplexer/iio-mux.c
+++ b/drivers/iio/multiplexer/iio-mux.c
@@ -7,6 +7,7 @@
  * Author: Peter Rosin <peda@axentia.se>
  */
 
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/iio/consumer.h>
 #include <linux/iio/iio.h>
@@ -237,49 +238,18 @@ static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
 	return ret;
 }
 
-static int mux_configure_channel(struct device *dev, struct mux *mux,
-				 u32 state, const char *label, int idx)
+static int mux_configure_chan_ext_info(struct device *dev, struct mux *mux,
+				       int idx, int num_ext_info)
 {
 	struct mux_child *child = &mux->child[idx];
-	struct iio_chan_spec *chan = &mux->chan[idx];
 	struct iio_chan_spec const *pchan = mux->parent->channel;
-	char *page = NULL;
-	int num_ext_info;
 	int i;
 	int ret;
 
-	chan->indexed = 1;
-	chan->output = pchan->output;
-	chan->datasheet_name = label;
-	chan->ext_info = mux->ext_info;
-
-	ret = iio_get_channel_type(mux->parent, &chan->type);
-	if (ret < 0) {
-		dev_err(dev, "failed to get parent channel type\n");
-		return ret;
-	}
-
-	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
-		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
-	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
-		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
-
-	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
-		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
-
-	if (state >= mux_control_states(mux->control)) {
-		dev_err(dev, "too many channels\n");
-		return -EINVAL;
-	}
-
-	chan->channel = state;
+	char *page __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!page)
+		return -ENOMEM;
 
-	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
-	if (num_ext_info) {
-		page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
-		if (!page)
-			return -ENOMEM;
-	}
 	child->ext_info_cache = devm_kcalloc(dev,
 					     num_ext_info,
 					     sizeof(*child->ext_info_cache),
@@ -318,8 +288,46 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
 		child->ext_info_cache[i].size = ret;
 	}
 
-	if (page)
-		devm_kfree(dev, page);
+	return 0;
+}
+
+static int mux_configure_channel(struct device *dev, struct mux *mux, u32 state,
+				 const char *label, int idx)
+{
+	struct iio_chan_spec *chan = &mux->chan[idx];
+	struct iio_chan_spec const *pchan = mux->parent->channel;
+	int num_ext_info;
+	int ret;
+
+	chan->indexed = 1;
+	chan->output = pchan->output;
+	chan->datasheet_name = label;
+	chan->ext_info = mux->ext_info;
+
+	ret = iio_get_channel_type(mux->parent, &chan->type);
+	if (ret < 0) {
+		dev_err(dev, "failed to get parent channel type\n");
+		return ret;
+	}
+
+	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
+		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
+	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
+		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
+
+	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
+		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
+
+	if (state >= mux_control_states(mux->control)) {
+		dev_err(dev, "too many channels\n");
+		return -EINVAL;
+	}
+
+	chan->channel = state;
+
+	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
+	if (num_ext_info)
+		return mux_configure_chan_ext_info(dev, mux, idx, num_ext_info);
 
 	return 0;
 }

-- 
2.47.0

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Jonathan Cameron]

On Mon, 02 Dec 2024 16:11:08 +0100
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> During channel configuration, the iio-mux driver allocates a page with
> devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> buffer points to an offset of the page due to the devres header sitting
> at the beginning of the allocated area. This leads to failure in the
> provider driver when sysfs_emit* helpers are used to format the ext_info
> attributes.
> 
> Switch to plain kzalloc version. The devres version is not strictly
> necessary as the buffer is only accessed during the channel
> configuration phase. Rely on __free cleanup to deallocate the buffer.
> Also, move the ext_info handling into a new function to have the page
> buffer definition and assignment in one statement as suggested by
> cleanup documentation.
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
This seems fine to me, but the diff ended up a bit complex, so I'd like
Peter to take a look as well before I apply it.

Do you have a board that is hitting this?  If so, a fixes tag is definitely
appropriate. I think it is probably appropriate even it not.

Jonathan

> ---
>  drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..c309d991490c63ba4299f1cda7102f10dcf54982 100644
> --- a/drivers/iio/multiplexer/iio-mux.c
> +++ b/drivers/iio/multiplexer/iio-mux.c
> @@ -7,6 +7,7 @@
>   * Author: Peter Rosin <peda@axentia.se>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/err.h>
>  #include <linux/iio/consumer.h>
>  #include <linux/iio/iio.h>
> @@ -237,49 +238,18 @@ static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
>  	return ret;
>  }
>  
> -static int mux_configure_channel(struct device *dev, struct mux *mux,
> -				 u32 state, const char *label, int idx)
> +static int mux_configure_chan_ext_info(struct device *dev, struct mux *mux,
> +				       int idx, int num_ext_info)
>  {
>  	struct mux_child *child = &mux->child[idx];
> -	struct iio_chan_spec *chan = &mux->chan[idx];
>  	struct iio_chan_spec const *pchan = mux->parent->channel;
> -	char *page = NULL;
> -	int num_ext_info;
>  	int i;
>  	int ret;
>  
> -	chan->indexed = 1;
> -	chan->output = pchan->output;
> -	chan->datasheet_name = label;
> -	chan->ext_info = mux->ext_info;
> -
> -	ret = iio_get_channel_type(mux->parent, &chan->type);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to get parent channel type\n");
> -		return ret;
> -	}
> -
> -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> -
> -	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> -		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> -
> -	if (state >= mux_control_states(mux->control)) {
> -		dev_err(dev, "too many channels\n");
> -		return -EINVAL;
> -	}
> -
> -	chan->channel = state;
> +	char *page __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!page)
> +		return -ENOMEM;
>  
> -	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> -	if (num_ext_info) {
> -		page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> -		if (!page)
> -			return -ENOMEM;
> -	}
>  	child->ext_info_cache = devm_kcalloc(dev,
>  					     num_ext_info,
>  					     sizeof(*child->ext_info_cache),
> @@ -318,8 +288,46 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
>  		child->ext_info_cache[i].size = ret;
>  	}
>  
> -	if (page)
> -		devm_kfree(dev, page);
> +	return 0;
> +}
> +
> +static int mux_configure_channel(struct device *dev, struct mux *mux, u32 state,
> +				 const char *label, int idx)
> +{
> +	struct iio_chan_spec *chan = &mux->chan[idx];
> +	struct iio_chan_spec const *pchan = mux->parent->channel;
> +	int num_ext_info;
> +	int ret;
> +
> +	chan->indexed = 1;
> +	chan->output = pchan->output;
> +	chan->datasheet_name = label;
> +	chan->ext_info = mux->ext_info;
> +
> +	ret = iio_get_channel_type(mux->parent, &chan->type);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get parent channel type\n");
> +		return ret;
> +	}
> +
> +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> +
> +	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> +
> +	if (state >= mux_control_states(mux->control)) {
> +		dev_err(dev, "too many channels\n");
> +		return -EINVAL;
> +	}
> +
> +	chan->channel = state;
> +
> +	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> +	if (num_ext_info)
> +		return mux_configure_chan_ext_info(dev, mux, idx, num_ext_info);
>  
>  	return 0;
>  }
> 

Re: [PATCH 1/2] iio: consumers: ensure read buffers for labels and ext_info are page aligned

[Jonathan Cameron]

On Mon, 02 Dec 2024 16:11:07 +0100
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Attributes of iio providers are exposed via sysfs. Typically, providers
> pass attribute values to the iio core, which handles formatting and
> printing to sysfs. However, some attributes, such as labels or extended
> info, are directly formatted and printed to sysfs by provider drivers
> using sysfs_emit() and sysfs_emit_at(). These helpers assume the read
> buffer, allocated by sysfs fop, is page-aligned. When these attributes
> are accessed by consumer drivers, the read buffer is allocated by the
> consumer and may not be page-aligned, leading to failures in the
> provider's callback that utilizes sysfs_emit*.
> 
> Add a check to ensure that read buffers for labels and external info
> attributes are page-aligned. Update the prototype documentation as well.
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
This is good hardening independent of fixing any issues so I've picked this
patch up for the togreg branch of iio.git

Thanks,

Jonathan

> ---
>  drivers/iio/inkern.c         | 11 +++++++++++
>  include/linux/iio/consumer.h |  4 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 7f325b3ed08fae6674245312cf8f57bb151006c0..63707ed98e1d7aca1e446122bbf69c85c0dd06a2 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -7,6 +7,7 @@
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/minmax.h>
> +#include <linux/mm.h>
>  #include <linux/mutex.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> @@ -989,6 +990,11 @@ ssize_t iio_read_channel_ext_info(struct iio_channel *chan,
>  {
>  	const struct iio_chan_spec_ext_info *ext_info;
>  
> +	if (!buf || offset_in_page(buf)) {
> +		pr_err("iio: invalid ext_info read buffer\n");
> +		return -EINVAL;
> +	}
> +
>  	ext_info = iio_lookup_ext_info(chan, attr);
>  	if (!ext_info)
>  		return -EINVAL;
> @@ -1014,6 +1020,11 @@ EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
>  
>  ssize_t iio_read_channel_label(struct iio_channel *chan, char *buf)
>  {
> +	if (!buf || offset_in_page(buf)) {
> +		pr_err("iio: invalid label read buffer\n");
> +		return -EINVAL;
> +	}
> +
>  	return do_iio_read_channel_label(chan->indio_dev, chan->channel, buf);
>  }
>  EXPORT_SYMBOL_GPL(iio_read_channel_label);
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 333d1d8ccb37f387fe531577ac5e0bfc7f752cec..6a44796164792b2dd930f8168b14de327a80a6f7 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -418,7 +418,7 @@ unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan);
>   * @chan:		The channel being queried.
>   * @attr:		The ext_info attribute to read.
>   * @buf:		Where to store the attribute value. Assumed to hold
> - *			at least PAGE_SIZE bytes.
> + *			at least PAGE_SIZE bytes and to be aligned at PAGE_SIZE.
>   *
>   * Returns the number of bytes written to buf (perhaps w/o zero termination;
>   * it need not even be a string), or an error code.
> @@ -445,7 +445,7 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
>   * iio_read_channel_label() - read label for a given channel
>   * @chan:		The channel being queried.
>   * @buf:		Where to store the attribute value. Assumed to hold
> - *			at least PAGE_SIZE bytes.
> + *			at least PAGE_SIZE bytes and to be aligned at PAGE_SIZE.
>   *
>   * Returns the number of bytes written to buf, or an error code.
>   */
> 

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Matteo Martelli]

On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 02 Dec 2024 16:11:08 +0100
> Matteo Martelli <matteomartelli3@gmail.com> wrote:
> 
> > During channel configuration, the iio-mux driver allocates a page with
> > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > buffer points to an offset of the page due to the devres header sitting
> > at the beginning of the allocated area. This leads to failure in the
> > provider driver when sysfs_emit* helpers are used to format the ext_info
> > attributes.
> > 
> > Switch to plain kzalloc version. The devres version is not strictly
> > necessary as the buffer is only accessed during the channel
> > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > Also, move the ext_info handling into a new function to have the page
> > buffer definition and assignment in one statement as suggested by
> > cleanup documentation.
> > 
> > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> This seems fine to me, but the diff ended up a bit complex, so I'd like
> Peter to take a look as well before I apply it.

For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
but since devres doesn't seem necessary in this case, I think this patch
provides a cleaner solution at the end.

> 
> Do you have a board that is hitting this?  If so, a fixes tag is definitely
> appropriate. I think it is probably appropriate even it not.

I am not sure if any existing board is affected as I encountered this
issue while experimenting with consumer drivers, thus using a custom DT
on top of sun50i-a64-pine64.dts just for testing. The following DT files
might be affected but only if the iio channel controlled by the iio_mux
multiplexer owns an ext_info attribute which is also exposed on sysfs.

$ grep -Rl 'io-channel-mux' arch
arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
arch/arm/boot/dts/microchip/at91-tse850-3.dts
arch/arm/boot/dts/microchip/at91-natte.dtsi
arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts

I am also not sure what would be the reference commit for the Fixes tag.
The related ext_info attributes handling was introduced in the first
commit of the iio_mux implementation. If that applies, following the
corresponding Fixes tag.

Fixes: 7ba9df54b091 ("iio: multiplexer: new iio category and iio-mux driver")

> 
> Jonathan
> 

Best regards,
Matteo
> > ---
> >  drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
> >  1 file changed, 46 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> > index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..c309d991490c63ba4299f1cda7102f10dcf54982 100644
> > --- a/drivers/iio/multiplexer/iio-mux.c
> > +++ b/drivers/iio/multiplexer/iio-mux.c
> > @@ -7,6 +7,7 @@
> >   * Author: Peter Rosin <peda@axentia.se>
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/err.h>
> >  #include <linux/iio/consumer.h>
> >  #include <linux/iio/iio.h>
> > @@ -237,49 +238,18 @@ static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
> >  	return ret;
> >  }
> >  
> > -static int mux_configure_channel(struct device *dev, struct mux *mux,
> > -				 u32 state, const char *label, int idx)
> > +static int mux_configure_chan_ext_info(struct device *dev, struct mux *mux,
> > +				       int idx, int num_ext_info)
> >  {
> >  	struct mux_child *child = &mux->child[idx];
> > -	struct iio_chan_spec *chan = &mux->chan[idx];
> >  	struct iio_chan_spec const *pchan = mux->parent->channel;
> > -	char *page = NULL;
> > -	int num_ext_info;
> >  	int i;
> >  	int ret;
> >  
> > -	chan->indexed = 1;
> > -	chan->output = pchan->output;
> > -	chan->datasheet_name = label;
> > -	chan->ext_info = mux->ext_info;
> > -
> > -	ret = iio_get_channel_type(mux->parent, &chan->type);
> > -	if (ret < 0) {
> > -		dev_err(dev, "failed to get parent channel type\n");
> > -		return ret;
> > -	}
> > -
> > -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > -
> > -	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > -		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > -
> > -	if (state >= mux_control_states(mux->control)) {
> > -		dev_err(dev, "too many channels\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	chan->channel = state;
> > +	char *page __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!page)
> > +		return -ENOMEM;
> >  
> > -	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > -	if (num_ext_info) {
> > -		page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> > -		if (!page)
> > -			return -ENOMEM;
> > -	}
> >  	child->ext_info_cache = devm_kcalloc(dev,
> >  					     num_ext_info,
> >  					     sizeof(*child->ext_info_cache),
> > @@ -318,8 +288,46 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
> >  		child->ext_info_cache[i].size = ret;
> >  	}
> >  
> > -	if (page)
> > -		devm_kfree(dev, page);
> > +	return 0;
> > +}
> > +
> > +static int mux_configure_channel(struct device *dev, struct mux *mux, u32 state,
> > +				 const char *label, int idx)
> > +{
> > +	struct iio_chan_spec *chan = &mux->chan[idx];
> > +	struct iio_chan_spec const *pchan = mux->parent->channel;
> > +	int num_ext_info;
> > +	int ret;
> > +
> > +	chan->indexed = 1;
> > +	chan->output = pchan->output;
> > +	chan->datasheet_name = label;
> > +	chan->ext_info = mux->ext_info;
> > +
> > +	ret = iio_get_channel_type(mux->parent, &chan->type);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to get parent channel type\n");
> > +		return ret;
> > +	}
> > +
> > +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > +
> > +	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > +
> > +	if (state >= mux_control_states(mux->control)) {
> > +		dev_err(dev, "too many channels\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	chan->channel = state;
> > +
> > +	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > +	if (num_ext_info)
> > +		return mux_configure_chan_ext_info(dev, mux, idx, num_ext_info);
> >  
> >  	return 0;
> >  }
> > 
> 

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Jonathan Cameron]

On Mon, 09 Dec 2024 11:39:55 +0100
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Mon, 02 Dec 2024 16:11:08 +0100
> > Matteo Martelli <matteomartelli3@gmail.com> wrote:
> >   
> > > During channel configuration, the iio-mux driver allocates a page with
> > > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > > buffer points to an offset of the page due to the devres header sitting
> > > at the beginning of the allocated area. This leads to failure in the
> > > provider driver when sysfs_emit* helpers are used to format the ext_info
> > > attributes.
> > > 
> > > Switch to plain kzalloc version. The devres version is not strictly
> > > necessary as the buffer is only accessed during the channel
> > > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > > Also, move the ext_info handling into a new function to have the page
> > > buffer definition and assignment in one statement as suggested by
> > > cleanup documentation.
> > > 
> > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>  
> > This seems fine to me, but the diff ended up a bit complex, so I'd like
> > Peter to take a look as well before I apply it.  
> 
> For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
> but since devres doesn't seem necessary in this case, I think this patch
> provides a cleaner solution at the end.

The approach is fine I think, I'd just like a second opinion so will
give Peter some time to get to it before applying.

> 
> > 
> > Do you have a board that is hitting this?  If so, a fixes tag is definitely
> > appropriate. I think it is probably appropriate even it not.  
> 
> I am not sure if any existing board is affected as I encountered this
> issue while experimenting with consumer drivers, thus using a custom DT
> on top of sun50i-a64-pine64.dts just for testing. The following DT files
> might be affected but only if the iio channel controlled by the iio_mux
> multiplexer owns an ext_info attribute which is also exposed on sysfs.
> 
> $ grep -Rl 'io-channel-mux' arch
> arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
> arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
> arch/arm/boot/dts/microchip/at91-tse850-3.dts
> arch/arm/boot/dts/microchip/at91-natte.dtsi
> arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
> arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
> arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
> arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
> arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
> arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> 
> I am also not sure what would be the reference commit for the Fixes tag.
> The related ext_info attributes handling was introduced in the first
> commit of the iio_mux implementation. If that applies, following the
> corresponding Fixes tag.
> 
> Fixes: 7ba9df54b091 ("iio: multiplexer: new iio category and iio-mux driver")
That works I think.

Thanks,

> 
> > 
> > Jonathan
> >   
> 
> Best regards,
> Matteo
> > > ---
> > >  drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
> > >  1 file changed, 46 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> > > index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..c309d991490c63ba4299f1cda7102f10dcf54982 100644
> > > --- a/drivers/iio/multiplexer/iio-mux.c
> > > +++ b/drivers/iio/multiplexer/iio-mux.c
> > > @@ -7,6 +7,7 @@
> > >   * Author: Peter Rosin <peda@axentia.se>
> > >   */
> > >  
> > > +#include <linux/cleanup.h>
> > >  #include <linux/err.h>
> > >  #include <linux/iio/consumer.h>
> > >  #include <linux/iio/iio.h>
> > > @@ -237,49 +238,18 @@ static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
> > >  	return ret;
> > >  }
> > >  
> > > -static int mux_configure_channel(struct device *dev, struct mux *mux,
> > > -				 u32 state, const char *label, int idx)
> > > +static int mux_configure_chan_ext_info(struct device *dev, struct mux *mux,
> > > +				       int idx, int num_ext_info)
> > >  {
> > >  	struct mux_child *child = &mux->child[idx];
> > > -	struct iio_chan_spec *chan = &mux->chan[idx];
> > >  	struct iio_chan_spec const *pchan = mux->parent->channel;
> > > -	char *page = NULL;
> > > -	int num_ext_info;
> > >  	int i;
> > >  	int ret;
> > >  
> > > -	chan->indexed = 1;
> > > -	chan->output = pchan->output;
> > > -	chan->datasheet_name = label;
> > > -	chan->ext_info = mux->ext_info;
> > > -
> > > -	ret = iio_get_channel_type(mux->parent, &chan->type);
> > > -	if (ret < 0) {
> > > -		dev_err(dev, "failed to get parent channel type\n");
> > > -		return ret;
> > > -	}
> > > -
> > > -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > > -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > > -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > > -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > > -
> > > -	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > > -		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > > -
> > > -	if (state >= mux_control_states(mux->control)) {
> > > -		dev_err(dev, "too many channels\n");
> > > -		return -EINVAL;
> > > -	}
> > > -
> > > -	chan->channel = state;
> > > +	char *page __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > +	if (!page)
> > > +		return -ENOMEM;
> > >  
> > > -	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > > -	if (num_ext_info) {
> > > -		page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> > > -		if (!page)
> > > -			return -ENOMEM;
> > > -	}
> > >  	child->ext_info_cache = devm_kcalloc(dev,
> > >  					     num_ext_info,
> > >  					     sizeof(*child->ext_info_cache),
> > > @@ -318,8 +288,46 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
> > >  		child->ext_info_cache[i].size = ret;
> > >  	}
> > >  
> > > -	if (page)
> > > -		devm_kfree(dev, page);
> > > +	return 0;
> > > +}
> > > +
> > > +static int mux_configure_channel(struct device *dev, struct mux *mux, u32 state,
> > > +				 const char *label, int idx)
> > > +{
> > > +	struct iio_chan_spec *chan = &mux->chan[idx];
> > > +	struct iio_chan_spec const *pchan = mux->parent->channel;
> > > +	int num_ext_info;
> > > +	int ret;
> > > +
> > > +	chan->indexed = 1;
> > > +	chan->output = pchan->output;
> > > +	chan->datasheet_name = label;
> > > +	chan->ext_info = mux->ext_info;
> > > +
> > > +	ret = iio_get_channel_type(mux->parent, &chan->type);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "failed to get parent channel type\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > > +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > > +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > > +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > > +
> > > +	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > > +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > > +
> > > +	if (state >= mux_control_states(mux->control)) {
> > > +		dev_err(dev, "too many channels\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	chan->channel = state;
> > > +
> > > +	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > > +	if (num_ext_info)
> > > +		return mux_configure_chan_ext_info(dev, mux, idx, num_ext_info);
> > >  
> > >  	return 0;
> > >  }
> > >   
> >   
> 

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[David Lechner]

On 12/2/24 9:11 AM, Matteo Martelli wrote:
> During channel configuration, the iio-mux driver allocates a page with
> devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> buffer points to an offset of the page due to the devres header sitting
> at the beginning of the allocated area. This leads to failure in the
> provider driver when sysfs_emit* helpers are used to format the ext_info
> attributes.
> 
> Switch to plain kzalloc version. The devres version is not strictly
> necessary as the buffer is only accessed during the channel
> configuration phase. Rely on __free cleanup to deallocate the buffer.
> Also, move the ext_info handling into a new function to have the page
> buffer definition and assignment in one statement as suggested by
> cleanup documentation.
> 
> Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Jonathan Cameron]

On Wed, 11 Dec 2024 18:17:54 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 09 Dec 2024 11:39:55 +0100
> Matteo Martelli <matteomartelli3@gmail.com> wrote:
> 
> > On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > On Mon, 02 Dec 2024 16:11:08 +0100
> > > Matteo Martelli <matteomartelli3@gmail.com> wrote:
> > >     
> > > > During channel configuration, the iio-mux driver allocates a page with
> > > > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > > > buffer points to an offset of the page due to the devres header sitting
> > > > at the beginning of the allocated area. This leads to failure in the
> > > > provider driver when sysfs_emit* helpers are used to format the ext_info
> > > > attributes.
> > > > 
> > > > Switch to plain kzalloc version. The devres version is not strictly
> > > > necessary as the buffer is only accessed during the channel
> > > > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > > > Also, move the ext_info handling into a new function to have the page
> > > > buffer definition and assignment in one statement as suggested by
> > > > cleanup documentation.
> > > > 
> > > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>    
> > > This seems fine to me, but the diff ended up a bit complex, so I'd like
> > > Peter to take a look as well before I apply it.    
> > 
> > For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
> > but since devres doesn't seem necessary in this case, I think this patch
> > provides a cleaner solution at the end.  
> 
> The approach is fine I think, I'd just like a second opinion so will
> give Peter some time to get to it before applying.

I want to get some build coverage at least on this, so I've applied it
for now to the togreg branch of iio.git and pushed out as testing.
Still a few more days for Peter to shout if he wants more time to looks at it!

Jonathan
> 
> >   
> > > 
> > > Do you have a board that is hitting this?  If so, a fixes tag is definitely
> > > appropriate. I think it is probably appropriate even it not.    
> > 
> > I am not sure if any existing board is affected as I encountered this
> > issue while experimenting with consumer drivers, thus using a custom DT
> > on top of sun50i-a64-pine64.dts just for testing. The following DT files
> > might be affected but only if the iio channel controlled by the iio_mux
> > multiplexer owns an ext_info attribute which is also exposed on sysfs.
> > 
> > $ grep -Rl 'io-channel-mux' arch
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
> > arch/arm/boot/dts/microchip/at91-tse850-3.dts
> > arch/arm/boot/dts/microchip/at91-natte.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
> > arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> > 
> > I am also not sure what would be the reference commit for the Fixes tag.
> > The related ext_info attributes handling was introduced in the first
> > commit of the iio_mux implementation. If that applies, following the
> > corresponding Fixes tag.
> > 
> > Fixes: 7ba9df54b091 ("iio: multiplexer: new iio category and iio-mux driver")  
> That works I think.
> 
> Thanks,
> 
> >   
> > > 
> > > Jonathan
> > >     
> > 
> > Best regards,
> > Matteo  
> > > > ---
> > > >  drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
> > > >  1 file changed, 46 insertions(+), 38 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> > > > index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..c309d991490c63ba4299f1cda7102f10dcf54982 100644
> > > > --- a/drivers/iio/multiplexer/iio-mux.c
> > > > +++ b/drivers/iio/multiplexer/iio-mux.c
> > > > @@ -7,6 +7,7 @@
> > > >   * Author: Peter Rosin <peda@axentia.se>
> > > >   */
> > > >  
> > > > +#include <linux/cleanup.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/iio/consumer.h>
> > > >  #include <linux/iio/iio.h>
> > > > @@ -237,49 +238,18 @@ static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > -static int mux_configure_channel(struct device *dev, struct mux *mux,
> > > > -				 u32 state, const char *label, int idx)
> > > > +static int mux_configure_chan_ext_info(struct device *dev, struct mux *mux,
> > > > +				       int idx, int num_ext_info)
> > > >  {
> > > >  	struct mux_child *child = &mux->child[idx];
> > > > -	struct iio_chan_spec *chan = &mux->chan[idx];
> > > >  	struct iio_chan_spec const *pchan = mux->parent->channel;
> > > > -	char *page = NULL;
> > > > -	int num_ext_info;
> > > >  	int i;
> > > >  	int ret;
> > > >  
> > > > -	chan->indexed = 1;
> > > > -	chan->output = pchan->output;
> > > > -	chan->datasheet_name = label;
> > > > -	chan->ext_info = mux->ext_info;
> > > > -
> > > > -	ret = iio_get_channel_type(mux->parent, &chan->type);
> > > > -	if (ret < 0) {
> > > > -		dev_err(dev, "failed to get parent channel type\n");
> > > > -		return ret;
> > > > -	}
> > > > -
> > > > -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > > > -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > > > -	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > > > -		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > > > -
> > > > -	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > > > -		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > > > -
> > > > -	if (state >= mux_control_states(mux->control)) {
> > > > -		dev_err(dev, "too many channels\n");
> > > > -		return -EINVAL;
> > > > -	}
> > > > -
> > > > -	chan->channel = state;
> > > > +	char *page __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > > +	if (!page)
> > > > +		return -ENOMEM;
> > > >  
> > > > -	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > > > -	if (num_ext_info) {
> > > > -		page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> > > > -		if (!page)
> > > > -			return -ENOMEM;
> > > > -	}
> > > >  	child->ext_info_cache = devm_kcalloc(dev,
> > > >  					     num_ext_info,
> > > >  					     sizeof(*child->ext_info_cache),
> > > > @@ -318,8 +288,46 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
> > > >  		child->ext_info_cache[i].size = ret;
> > > >  	}
> > > >  
> > > > -	if (page)
> > > > -		devm_kfree(dev, page);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int mux_configure_channel(struct device *dev, struct mux *mux, u32 state,
> > > > +				 const char *label, int idx)
> > > > +{
> > > > +	struct iio_chan_spec *chan = &mux->chan[idx];
> > > > +	struct iio_chan_spec const *pchan = mux->parent->channel;
> > > > +	int num_ext_info;
> > > > +	int ret;
> > > > +
> > > > +	chan->indexed = 1;
> > > > +	chan->output = pchan->output;
> > > > +	chan->datasheet_name = label;
> > > > +	chan->ext_info = mux->ext_info;
> > > > +
> > > > +	ret = iio_get_channel_type(mux->parent, &chan->type);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "failed to get parent channel type\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > > > +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > > > +	if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > > > +		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > > > +
> > > > +	if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > > > +		chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > > > +
> > > > +	if (state >= mux_control_states(mux->control)) {
> > > > +		dev_err(dev, "too many channels\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	chan->channel = state;
> > > > +
> > > > +	num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > > > +	if (num_ext_info)
> > > > +		return mux_configure_chan_ext_info(dev, mux, idx, num_ext_info);
> > > >  
> > > >  	return 0;
> > > >  }
> > > >     
> > >     
> >   
> 
> 

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Andy Shevchenko]

Mon, Dec 09, 2024 at 11:39:55AM +0100, Matteo Martelli kirjoitti:
> On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@kernel.org> wrote:
> > On Mon, 02 Dec 2024 16:11:08 +0100
> > Matteo Martelli <matteomartelli3@gmail.com> wrote:
> > 
> > > During channel configuration, the iio-mux driver allocates a page with
> > > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > > buffer points to an offset of the page due to the devres header sitting
> > > at the beginning of the allocated area. This leads to failure in the
> > > provider driver when sysfs_emit* helpers are used to format the ext_info
> > > attributes.
> > > 
> > > Switch to plain kzalloc version. The devres version is not strictly
> > > necessary as the buffer is only accessed during the channel
> > > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > > Also, move the ext_info handling into a new function to have the page
> > > buffer definition and assignment in one statement as suggested by
> > > cleanup documentation.
> > > 
> > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>
> > This seems fine to me, but the diff ended up a bit complex, so I'd like
> > Peter to take a look as well before I apply it.
> 
> For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
> but since devres doesn't seem necessary in this case, I think this patch
> provides a cleaner solution at the end.

I am here just to ask why we don't use get_free_page() or analogue to begin
with, but it seems the thread has a remark WRT this.

> > Do you have a board that is hitting this?  If so, a fixes tag is definitely
> > appropriate. I think it is probably appropriate even it not.
> 
> I am not sure if any existing board is affected as I encountered this
> issue while experimenting with consumer drivers, thus using a custom DT
> on top of sun50i-a64-pine64.dts just for testing. The following DT files
> might be affected but only if the iio channel controlled by the iio_mux
> multiplexer owns an ext_info attribute which is also exposed on sysfs.
> 
> $ grep -Rl 'io-channel-mux' arch

Btw, `git grep ...` is _much_ faster.

	git grep -lw io-channel-mux -- arch/

and won't give unrelated lines (e.g., *.dtb).

> arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
> arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
> arch/arm/boot/dts/microchip/at91-tse850-3.dts
> arch/arm/boot/dts/microchip/at91-natte.dtsi
> arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
> arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
> arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
> arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
> arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
> arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to ensure page alignment

[Jonathan Cameron]

On Sun, 12 Jan 2025 18:24:30 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Mon, Dec 09, 2024 at 11:39:55AM +0100, Matteo Martelli kirjoitti:
> > On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > On Mon, 02 Dec 2024 16:11:08 +0100
> > > Matteo Martelli <matteomartelli3@gmail.com> wrote:
> > >   
> > > > During channel configuration, the iio-mux driver allocates a page with
> > > > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > > > buffer points to an offset of the page due to the devres header sitting
> > > > at the beginning of the allocated area. This leads to failure in the
> > > > provider driver when sysfs_emit* helpers are used to format the ext_info
> > > > attributes.
> > > > 
> > > > Switch to plain kzalloc version. The devres version is not strictly
> > > > necessary as the buffer is only accessed during the channel
> > > > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > > > Also, move the ext_info handling into a new function to have the page
> > > > buffer definition and assignment in one statement as suggested by
> > > > cleanup documentation.
> > > > 
> > > > Signed-off-by: Matteo Martelli <matteomartelli3@gmail.com>  
> > > This seems fine to me, but the diff ended up a bit complex, so I'd like
> > > Peter to take a look as well before I apply it.  
> > 
> > For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
> > but since devres doesn't seem necessary in this case, I think this patch
> > provides a cleaner solution at the end.  
> 
> I am here just to ask why we don't use get_free_page() or analogue to begin
> with, but it seems the thread has a remark WRT this.
This patch has now gone upstream, but a follow up to switch to
get_free_page() would be fine as a cleanup.

I just wanted to land the fix reasonably quickly rather than waiting
for the perfect answer.

Jonathan

> 
> > > Do you have a board that is hitting this?  If so, a fixes tag is definitely
> > > appropriate. I think it is probably appropriate even it not.  
> > 
> > I am not sure if any existing board is affected as I encountered this
> > issue while experimenting with consumer drivers, thus using a custom DT
> > on top of sun50i-a64-pine64.dts just for testing. The following DT files
> > might be affected but only if the iio channel controlled by the iio_mux
> > multiplexer owns an ext_info attribute which is also exposed on sysfs.
> > 
> > $ grep -Rl 'io-channel-mux' arch  
> 
> Btw, `git grep ...` is _much_ faster.
> 
> 	git grep -lw io-channel-mux -- arch/
> 
> and won't give unrelated lines (e.g., *.dtb).
> 
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
> > arch/arm/boot/dts/microchip/at91-tse850-3.dts
> > arch/arm/boot/dts/microchip/at91-natte.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
> > arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts  
>