[PATCH v2] iio: proximity: isl29501: fix buffered read on big-endian systems

[PATCH v2] iio: proximity: isl29501: fix buffered read on big-endian systems

[David Lechner]

Fix passing a u32 value as a u16 buffer scan item. This works on little-
endian systems, but not on big-endian systems.

A new local variable is introduced for getting the register value and
the array is changed to a struct to make the data layout more explicit
rather than just changing the type and having to recalculate the proper
length needed for the timestamp.

Fixes: 1c28799257bc ("iio: light: isl29501: Add support for the ISL29501 ToF sensor.")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
Changes in v2:
- Use u16 to match channel scan_type and introduce new local u32 variable
  for getting the register value.
- Reword subject and commit message since we now consider this a bug fix.
- Fix not zero-initializing the new struct.
- Link to v1: https://lore.kernel.org/r/20250711-iio-use-more-iio_declare_buffer_with_ts-7-v1-1-a3f253ac2e4a@baylibre.com
---
 drivers/iio/proximity/isl29501.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/proximity/isl29501.c b/drivers/iio/proximity/isl29501.c
index d1510fe2405088adc0998e28aa9f36e0186fafae..f69db6f2f380313b8444ee21399ee3a9faed6f04 100644
--- a/drivers/iio/proximity/isl29501.c
+++ b/drivers/iio/proximity/isl29501.c
@@ -938,12 +938,18 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct isl29501_private *isl29501 = iio_priv(indio_dev);
 	const unsigned long *active_mask = indio_dev->active_scan_mask;
-	u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
-
-	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
-		isl29501_register_read(isl29501, REG_DISTANCE, buffer);
+	u32 value;
+	struct {
+		u16 data;
+		aligned_s64 ts;
+	} scan = { };
+
+	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask)) {
+		isl29501_register_read(isl29501, REG_DISTANCE, &value);
+		scan.data = value;
+	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;

---
base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
change-id: 20250711-iio-use-more-iio_declare_buffer_with_ts-7-880ddf1d3070

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH v2] iio: proximity: isl29501: fix buffered read on big-endian systems

[Jonathan Cameron]

On Tue, 22 Jul 2025 15:54:21 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Fix passing a u32 value as a u16 buffer scan item. This works on little-
> endian systems, but not on big-endian systems.
> 
> A new local variable is introduced for getting the register value and
> the array is changed to a struct to make the data layout more explicit
> rather than just changing the type and having to recalculate the proper
> length needed for the timestamp.
> 
> Fixes: 1c28799257bc ("iio: light: isl29501: Add support for the ISL29501 ToF sensor.")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
ok. This probably is the best minimal fix but there is a bunch of other type
confusion around this in the driver (not as far as I can see actual bugs though).

Might be good to circle back and make the val parameter of isl29501_register_read()
a u16 as a follow up.

Applied to my temporary fixes-togreg-for-6.17 branch on iio.git and marked
for stable.

Thanks,

Jonathan



> ---
> Changes in v2:
> - Use u16 to match channel scan_type and introduce new local u32 variable
>   for getting the register value.
> - Reword subject and commit message since we now consider this a bug fix.
> - Fix not zero-initializing the new struct.
> - Link to v1: https://lore.kernel.org/r/20250711-iio-use-more-iio_declare_buffer_with_ts-7-v1-1-a3f253ac2e4a@baylibre.com
> ---
>  drivers/iio/proximity/isl29501.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/proximity/isl29501.c b/drivers/iio/proximity/isl29501.c
> index d1510fe2405088adc0998e28aa9f36e0186fafae..f69db6f2f380313b8444ee21399ee3a9faed6f04 100644
> --- a/drivers/iio/proximity/isl29501.c
> +++ b/drivers/iio/proximity/isl29501.c
> @@ -938,12 +938,18 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct isl29501_private *isl29501 = iio_priv(indio_dev);
>  	const unsigned long *active_mask = indio_dev->active_scan_mask;
> -	u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
> -
> -	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
> -		isl29501_register_read(isl29501, REG_DISTANCE, buffer);
> +	u32 value;
> +	struct {
> +		u16 data;
> +		aligned_s64 ts;
> +	} scan = { };
> +
> +	if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask)) {
> +		isl29501_register_read(isl29501, REG_DISTANCE, &value); 
> +		scan.data = value;
> +	}
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
> 
> ---
> base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
> change-id: 20250711-iio-use-more-iio_declare_buffer_with_ts-7-880ddf1d3070
> 
> Best regards,

Re: [PATCH v2] iio: proximity: isl29501: fix buffered read on big-endian systems

[Geert Uytterhoeven]

Hi David,

On Tue, 22 Jul 2025 at 22:55, David Lechner <dlechner@baylibre.com> wrote:
> Fix passing a u32 value as a u16 buffer scan item. This works on little-
> endian systems, but not on big-endian systems.
>
> A new local variable is introduced for getting the register value and
> the array is changed to a struct to make the data layout more explicit
> rather than just changing the type and having to recalculate the proper
> length needed for the timestamp.
>
> Fixes: 1c28799257bc ("iio: light: isl29501: Add support for the ISL29501 ToF sensor.")
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Thanks for your patch, which is now commit de18e978d0cda23e ("iio:
proximity: isl29501: fix buffered read on big-endian systems")
in v6.17-rc3.

> --- a/drivers/iio/proximity/isl29501.c
> +++ b/drivers/iio/proximity/isl29501.c
> @@ -938,12 +938,18 @@ static irqreturn_t isl29501_trigger_handler(int irq, void *p)
>         struct iio_dev *indio_dev = pf->indio_dev;
>         struct isl29501_private *isl29501 = iio_priv(indio_dev);
>         const unsigned long *active_mask = indio_dev->active_scan_mask;
> -       u32 buffer[4] __aligned(8) = {}; /* 1x16-bit + naturally aligned ts */
> -
> -       if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask))
> -               isl29501_register_read(isl29501, REG_DISTANCE, buffer);
> +       u32 value;
> +       struct {
> +               u16 data;
> +               aligned_s64 ts;
> +       } scan = { };

This still looks rather obfuse to me: you rely on the implicit
presence of a 6-byte hole between data and ts, and on the implicit
64-bit alignment of data.

What about making this explicit?

    struct {
            u16 data;
            u16 unused[3];
            s64 ts;
    } __aligned(8) scan = { };

> +
> +       if (test_bit(ISL29501_DISTANCE_SCAN_INDEX, active_mask)) {
> +               isl29501_register_read(isl29501, REG_DISTANCE, &value);
> +               scan.data = value;
> +       }
>
> -       iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
> +       iio_push_to_buffers_with_timestamp(indio_dev, &scan, pf->timestamp);
>         iio_trigger_notify_done(indio_dev->trig);
>
>         return IRQ_HANDLED;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] iio: proximity: isl29501: fix buffered read on big-endian systems

[Andy Shevchenko]

On Fri, Aug 29, 2025 at 12:48 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, 22 Jul 2025 at 22:55, David Lechner <dlechner@baylibre.com> wrote:

...

> > +       struct {
> > +               u16 data;
> > +               aligned_s64 ts;
> > +       } scan = { };
>
> This still looks rather obfuse to me: you rely on the implicit
> presence of a 6-byte hole between data and ts, and on the implicit
> 64-bit alignment of data.
>
> What about making this explicit?

It's problematic as it's non-uniform. In each driver the author should
carefully think about this and it appears that many just made the same
mistake(s) over and over. The proposed fix doesn't rely on the actual
type and members before ts. That said, NAK to your proposal and ACK
for the original patch.

\>     struct {
>             u16 data;
>             u16 unused[3];
>             s64 ts;
>     } __aligned(8) scan = { };


-- 
With Best Regards,
Andy Shevchenko