[PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[Javier Carrasco]

The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
set (optimized path for color channel readings), and it must be shifted
instead of leaving an empty channel for the temperature when it is off.

Once the channel index is fixed, the uninitialized channel must be set
to zero to avoid pushing uninitialized data.

Add available_scan_masks for all channels and only-color channels to let
the IIO core demux and repack the enabled channels.

Cc: stable@vger.kernel.org
Fixes: 403e5586b52e ("iio: light: as73211: New driver")
Tested-by: Christian Eggers <ceggers@arri.de>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
This issue was found after attempting to make the same mistake for
a driver I maintain, which was fortunately spotted by Jonathan [1].

Keeping old sensor values if the channel configuration changes is known
and not considered an issue, which is also mentioned in [1], so it has
not been addressed by this series. That keeps most of the drivers out
of the way because they store the scan element in iio private data,
which is kzalloc() allocated.

This series only addresses cases where uninitialized i.e. unknown data
is pushed to the userspace, either due to holes in structs or
uninitialized struct members/array elements.

While analyzing involved functions, I found and fixed some triviality
(wrong function name) in the documentation of iio_dev_opaque.

Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]
---
Changes in v3:
- as73211.c: add available_scan_masks for all channels and only-color
  channels to let the IIO core demux and repack the enabled channels.
- Link to v2: https://lore.kernel.org/r/20241204-iio_memset_scan_holes-v2-0-3f941592a76d@gmail.com

Changes in v2:
- as73211.c: shift channels if no temperature is available and
  initialize chan[3] to zero.
- Link to v1: https://lore.kernel.org/r/20241125-iio_memset_scan_holes-v1-0-0cb6e98d895c@gmail.com
---
 drivers/iio/light/as73211.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
index be0068081ebb..4be2e349a068 100644
--- a/drivers/iio/light/as73211.c
+++ b/drivers/iio/light/as73211.c
@@ -177,6 +177,12 @@ struct as73211_data {
 	BIT(AS73211_SCAN_INDEX_TEMP) | \
 	AS73211_SCAN_MASK_COLOR)
 
+static const unsigned long as73211_scan_masks[] = {
+	AS73211_SCAN_MASK_ALL,
+	AS73211_SCAN_MASK_COLOR,
+	0,
+};
+
 static const struct iio_chan_spec as73211_channels[] = {
 	{
 		.type = IIO_TEMP,
@@ -672,9 +678,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
 
 		/* AS73211 starts reading at address 2 */
 		ret = i2c_master_recv(data->client,
-				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
+				(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));
 		if (ret < 0)
 			goto done;
+
+		/* Avoid pushing uninitialized data */
+		scan.chan[3] = 0;
 	}
 
 	if (data_result) {
@@ -682,9 +691,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
 		 * Saturate all channels (in case of overflows). Temperature channel
 		 * is not affected by overflows.
 		 */
-		scan.chan[1] = cpu_to_le16(U16_MAX);
-		scan.chan[2] = cpu_to_le16(U16_MAX);
-		scan.chan[3] = cpu_to_le16(U16_MAX);
+		if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {
+			scan.chan[1] = cpu_to_le16(U16_MAX);
+			scan.chan[2] = cpu_to_le16(U16_MAX);
+			scan.chan[3] = cpu_to_le16(U16_MAX);
+		} else {
+			scan.chan[0] = cpu_to_le16(U16_MAX);
+			scan.chan[1] = cpu_to_le16(U16_MAX);
+			scan.chan[2] = cpu_to_le16(U16_MAX);
+		}
 	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
@@ -758,6 +773,7 @@ static int as73211_probe(struct i2c_client *client)
 	indio_dev->channels = data->spec_dev->channels;
 	indio_dev->num_channels = data->spec_dev->num_channels;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->available_scan_masks = as73211_scan_masks;
 
 	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
 	if (ret < 0)

---
base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
change-id: 20241123-iio_memset_scan_holes-a673833ef932

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[Christophe JAILLET]

Le 12/12/2024 à 18:56, Javier Carrasco a écrit :
> The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
> set (optimized path for color channel readings), and it must be shifted
> instead of leaving an empty channel for the temperature when it is off.
> 
> Once the channel index is fixed, the uninitialized channel must be set
> to zero to avoid pushing uninitialized data.
> 
> Add available_scan_masks for all channels and only-color channels to let
> the IIO core demux and repack the enabled channels.
> 
> Cc: stable@vger.kernel.org
> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> Tested-by: Christian Eggers <ceggers@arri.de>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

...

> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index be0068081ebb..4be2e349a068 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -177,6 +177,12 @@ struct as73211_data {
>   	BIT(AS73211_SCAN_INDEX_TEMP) | \
>   	AS73211_SCAN_MASK_COLOR)
>   
> +static const unsigned long as73211_scan_masks[] = {
> +	AS73211_SCAN_MASK_ALL,
> +	AS73211_SCAN_MASK_COLOR,
> +	0,
> +};
> +
>   static const struct iio_chan_spec as73211_channels[] = {
>   	{
>   		.type = IIO_TEMP,
> @@ -672,9 +678,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>   
>   		/* AS73211 starts reading at address 2 */

Should this comment be updated?

Or maybe moved close to "if (*indio_dev->active_scan_mask == 
AS73211_SCAN_MASK_ALL)" below?

>   		ret = i2c_master_recv(data->client,
> -				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
> +				(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));
>   		if (ret < 0)
>   			goto done;
> +
> +		/* Avoid pushing uninitialized data */
> +		scan.chan[3] = 0;
>   	}
>   
>   	if (data_result) {
> @@ -682,9 +691,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>   		 * Saturate all channels (in case of overflows). Temperature channel
>   		 * is not affected by overflows.
>   		 */
> -		scan.chan[1] = cpu_to_le16(U16_MAX);
> -		scan.chan[2] = cpu_to_le16(U16_MAX);
> -		scan.chan[3] = cpu_to_le16(U16_MAX);
> +		if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {

Should [0]...

> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);
> +			scan.chan[3] = cpu_to_le16(U16_MAX);
> +		} else {
> +			scan.chan[0] = cpu_to_le16(U16_MAX);
> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);

... and [3] be forced as-well?
(just a blind guess)

CJ

> +		}
>   	}
>   
>   	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
> @@ -758,6 +773,7 @@ static int as73211_probe(struct i2c_client *client)
>   	indio_dev->channels = data->spec_dev->channels;
>   	indio_dev->num_channels = data->spec_dev->num_channels;
>   	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = as73211_scan_masks;
>   
>   	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
>   	if (ret < 0)
> 
> ---
> base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
> change-id: 20241123-iio_memset_scan_holes-a673833ef932
> 
> Best regards,

Re: [PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[Javier Carrasco]

Hi Christophe, thank you for your prompt feedback.

On Thu Dec 12, 2024 at 7:15 PM CET, Christophe JAILLET wrote:
> Le 12/12/2024 à 18:56, Javier Carrasco a écrit :
> > The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
> > set (optimized path for color channel readings), and it must be shifted
> > instead of leaving an empty channel for the temperature when it is off.
> >
> > Once the channel index is fixed, the uninitialized channel must be set
> > to zero to avoid pushing uninitialized data.
> >
> > Add available_scan_masks for all channels and only-color channels to let
> > the IIO core demux and repack the enabled channels.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> > Tested-by: Christian Eggers <ceggers@arri.de>
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> ...
>
> > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> > index be0068081ebb..4be2e349a068 100644
> > --- a/drivers/iio/light/as73211.c
> > +++ b/drivers/iio/light/as73211.c
> > @@ -177,6 +177,12 @@ struct as73211_data {
> >   	BIT(AS73211_SCAN_INDEX_TEMP) | \
> >   	AS73211_SCAN_MASK_COLOR)
> >
> > +static const unsigned long as73211_scan_masks[] = {
> > +	AS73211_SCAN_MASK_ALL,
> > +	AS73211_SCAN_MASK_COLOR,
> > +	0,
> > +};
> > +
> >   static const struct iio_chan_spec as73211_channels[] = {
> >   	{
> >   		.type = IIO_TEMP,
> > @@ -672,9 +678,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
> >
> >   		/* AS73211 starts reading at address 2 */
>
> Should this comment be updated?
>
> Or maybe moved close to "if (*indio_dev->active_scan_mask ==
> AS73211_SCAN_MASK_ALL)" below?
>

The comment is still true, as address = 1 stores the temperature, and
the first color value can be found at address = 2. I think it used to
be more relevant (even if it was not the correct approach) when the
first received element was stored in chan[1]. Nevertheless, as it might
not be obvious without knowing the address map, it could stay where it
is.

> >   		ret = i2c_master_recv(data->client,
> > -				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
> > +				(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));
> >   		if (ret < 0)
> >   			goto done;
> > +
> > +		/* Avoid pushing uninitialized data */
> > +		scan.chan[3] = 0;
> >   	}
> >
> >   	if (data_result) {
> > @@ -682,9 +691,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
> >   		 * Saturate all channels (in case of overflows). Temperature channel
> >   		 * is not affected by overflows.
> >   		 */
> > -		scan.chan[1] = cpu_to_le16(U16_MAX);
> > -		scan.chan[2] = cpu_to_le16(U16_MAX);
> > -		scan.chan[3] = cpu_to_le16(U16_MAX);
> > +		if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {
>
> Should [0]...
>
> > +			scan.chan[1] = cpu_to_le16(U16_MAX);
> > +			scan.chan[2] = cpu_to_le16(U16_MAX);
> > +			scan.chan[3] = cpu_to_le16(U16_MAX);
> > +		} else {
> > +			scan.chan[0] = cpu_to_le16(U16_MAX);
> > +			scan.chan[1] = cpu_to_le16(U16_MAX);
> > +			scan.chan[2] = cpu_to_le16(U16_MAX);
>
> ... and [3] be forced as-well?
> (just a blind guess)
>
> CJ
>

in the first case (all channels are read), the temperature (scan[0])
only has 12 bits, and there are no overflows. In the second case,
scan.chan[3] is set to zero as it is not used, and there is no need to
force the U16_MAX value.

Best regards,
Javier Carrasco

Re: [PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[Jonathan Cameron]

On Thu, 12 Dec 2024 18:56:32 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
> set (optimized path for color channel readings), and it must be shifted
> instead of leaving an empty channel for the temperature when it is off.
> 
> Once the channel index is fixed, the uninitialized channel must be set
> to zero to avoid pushing uninitialized data.
> 
> Add available_scan_masks for all channels and only-color channels to let
> the IIO core demux and repack the enabled channels.
> 
> Cc: stable@vger.kernel.org
> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> Tested-by: Christian Eggers <ceggers@arri.de>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> This issue was found after attempting to make the same mistake for
> a driver I maintain, which was fortunately spotted by Jonathan [1].
> 
> Keeping old sensor values if the channel configuration changes is known
> and not considered an issue, which is also mentioned in [1], so it has
> not been addressed by this series. That keeps most of the drivers out
> of the way because they store the scan element in iio private data,
> which is kzalloc() allocated.
> 
> This series only addresses cases where uninitialized i.e. unknown data
> is pushed to the userspace, either due to holes in structs or
> uninitialized struct members/array elements.
> 
> While analyzing involved functions, I found and fixed some triviality
> (wrong function name) in the documentation of iio_dev_opaque.
> 
> Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]
> ---
> Changes in v3:
> - as73211.c: add available_scan_masks for all channels and only-color
>   channels to let the IIO core demux and repack the enabled channels.
> - Link to v2: https://lore.kernel.org/r/20241204-iio_memset_scan_holes-v2-0-3f941592a76d@gmail.com
> 
> Changes in v2:
> - as73211.c: shift channels if no temperature is available and
>   initialize chan[3] to zero.
> - Link to v1: https://lore.kernel.org/r/20241125-iio_memset_scan_holes-v1-0-0cb6e98d895c@gmail.com
> ---
>  drivers/iio/light/as73211.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index be0068081ebb..4be2e349a068 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -177,6 +177,12 @@ struct as73211_data {
>  	BIT(AS73211_SCAN_INDEX_TEMP) | \
>  	AS73211_SCAN_MASK_COLOR)
>  
> +static const unsigned long as73211_scan_masks[] = {
> +	AS73211_SCAN_MASK_ALL,
> +	AS73211_SCAN_MASK_COLOR,

I probably mislead you on this :(
Needs to be the other way around as the core code starts at first
entry whilst trying to find a mask that is a superset of what is turned on.
here that means it will always use the first one.
See iio_scan_mask_match() - strict isn't set int this case.


	
> +	0,
No need for comma on the 0. It's a terminating entry so we don't
want anyone to think they can add things after this

> +};
> +
>  static const struct iio_chan_spec as73211_channels[] = {
>  	{
>  		.type = IIO_TEMP,
> @@ -672,9 +678,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>  
>  		/* AS73211 starts reading at address 2 */
>  		ret = i2c_master_recv(data->client,
> -				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
> +				(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));
>  		if (ret < 0)
>  			goto done;
> +
> +		/* Avoid pushing uninitialized data */
> +		scan.chan[3] = 0;
>  	}
>  
>  	if (data_result) {
> @@ -682,9 +691,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>  		 * Saturate all channels (in case of overflows). Temperature channel
>  		 * is not affected by overflows.
>  		 */
> -		scan.chan[1] = cpu_to_le16(U16_MAX);
> -		scan.chan[2] = cpu_to_le16(U16_MAX);
> -		scan.chan[3] = cpu_to_le16(U16_MAX);
> +		if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {
> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);
> +			scan.chan[3] = cpu_to_le16(U16_MAX);
> +		} else {
> +			scan.chan[0] = cpu_to_le16(U16_MAX);
> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);
> +		}
>  	}
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
> @@ -758,6 +773,7 @@ static int as73211_probe(struct i2c_client *client)
>  	indio_dev->channels = data->spec_dev->channels;
>  	indio_dev->num_channels = data->spec_dev->num_channels;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = as73211_scan_masks;
>  
>  	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
>  	if (ret < 0)
> 
> ---
> base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
> change-id: 20241123-iio_memset_scan_holes-a673833ef932
> 
> Best regards,

Re: [PATCH v3] iio: light: as73211: fix channel handling in only-color triggered buffer

[Javier Carrasco]

On Sat Dec 14, 2024 at 3:22 PM CET, Jonathan Cameron wrote:
> On Thu, 12 Dec 2024 18:56:32 +0100
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>
> > The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
> > set (optimized path for color channel readings), and it must be shifted
> > instead of leaving an empty channel for the temperature when it is off.
> >
> > Once the channel index is fixed, the uninitialized channel must be set
> > to zero to avoid pushing uninitialized data.
> >
> > Add available_scan_masks for all channels and only-color channels to let
> > the IIO core demux and repack the enabled channels.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> > Tested-by: Christian Eggers <ceggers@arri.de>
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
> > This issue was found after attempting to make the same mistake for
> > a driver I maintain, which was fortunately spotted by Jonathan [1].
> >
> > Keeping old sensor values if the channel configuration changes is known
> > and not considered an issue, which is also mentioned in [1], so it has
> > not been addressed by this series. That keeps most of the drivers out
> > of the way because they store the scan element in iio private data,
> > which is kzalloc() allocated.
> >
> > This series only addresses cases where uninitialized i.e. unknown data
> > is pushed to the userspace, either due to holes in structs or
> > uninitialized struct members/array elements.
> >
> > While analyzing involved functions, I found and fixed some triviality
> > (wrong function name) in the documentation of iio_dev_opaque.
> >
> > Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]
> > ---
> > Changes in v3:
> > - as73211.c: add available_scan_masks for all channels and only-color
> >   channels to let the IIO core demux and repack the enabled channels.
> > - Link to v2: https://lore.kernel.org/r/20241204-iio_memset_scan_holes-v2-0-3f941592a76d@gmail.com
> >
> > Changes in v2:
> > - as73211.c: shift channels if no temperature is available and
> >   initialize chan[3] to zero.
> > - Link to v1: https://lore.kernel.org/r/20241125-iio_memset_scan_holes-v1-0-0cb6e98d895c@gmail.com
> > ---
> >  drivers/iio/light/as73211.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> > index be0068081ebb..4be2e349a068 100644
> > --- a/drivers/iio/light/as73211.c
> > +++ b/drivers/iio/light/as73211.c
> > @@ -177,6 +177,12 @@ struct as73211_data {
> >  	BIT(AS73211_SCAN_INDEX_TEMP) | \
> >  	AS73211_SCAN_MASK_COLOR)
> >
> > +static const unsigned long as73211_scan_masks[] = {
> > +	AS73211_SCAN_MASK_ALL,
> > +	AS73211_SCAN_MASK_COLOR,
>
> I probably mislead you on this :(
> Needs to be the other way around as the core code starts at first
> entry whilst trying to find a mask that is a superset of what is turned on.
> here that means it will always use the first one.
> See iio_scan_mask_match() - strict isn't set int this case.
>

Thanks for your feedback. I copied your suggestion, but it was my fault
that I did not make sure it was right for the intended behavior.

I will send a new version with that fixed soon.

Best regards,
Javier Carrasco