[PATCH RFC 0/3] iio: add helpers and accessors for active channels and masklength

[PATCH RFC 0/3] iio: add helpers and accessors for active channels and masklength

[Nuno Sa]

Hi Jonathan,

In [1], you suggested for an iterator for the active channels (so driver
don't directly access masklength). This RFC showcases that iterator and
goes one step further by giving an accessors for masklength so that
drivers can read that variable (we have drivers doing that). The
accessors uses ACCESS_PRIVATE() so it will warn us if some driver
directly access the variable making it more difficult to mess with it
(like changing it's value) without being noticed during review (or the
auto builders).

Anyways, before jumping in changing all the drivers using this, I guess
the questions are:

1) Is the iterator useful enough to add one (kind of like it and save a
line of code :))?
2) Do we care about going with the work of marking masklength private? 

If we go ahead the plan would be:

1) Add the helpers macros;
2) Convert all drivers that directly access 'masklength';
3) Annotate it as private.

[1]: https://lore.kernel.org/linux-iio/20240428142343.5067c898@jic23-huawei/

---
Nuno Sa (3):
      iio: core: add new helper to iterate active channels
      iio: imu: adis16475: make use of iio_for_each_active_channel()
      iio: core annotate masklength as private

 drivers/iio/imu/adis16475.c | 3 +--
 include/linux/iio/iio.h     | 8 +++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)
---
base-commit: cc1ce839526a65620778617da0b022bd88e8a139
change-id: 20240612-dev-iio-scan-private-86f4a0fd288f
--

Thanks!
- Nuno Sá

[PATCH RFC 1/3] iio: core: add new helper to iterate active channels

[Nuno Sa]

Add a new iio_for_each_active_channel() helper to iterate over the
active channels. Also add an helper to access masklength using
ACCESS_PRIVATE(). End goal is to annotate that variable as __private so that
we can get checker warnings if drivers directly access that variable.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/iio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 894309294182..a63888746707 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -860,6 +860,12 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
 int iio_str_to_fixpoint(const char *str, int fract_mult, int *integer,
 	int *fract);
 
+#define iio_dev_masklength(indio_dev)	ACCESS_PRIVATE(indio_dev, masklength)
+
+#define iio_for_each_active_channel(indio_dev, bit) \
+	for_each_set_bit((bit), (indio_dev)->active_scan_mask, \
+			 iio_dev_masklength(indio_dev))
+
 /**
  * IIO_DEGREE_TO_RAD() - Convert degree to rad
  * @deg: A value in degree

-- 
2.45.2

[PATCH RFC 2/3] iio: imu: adis16475: make use of iio_for_each_active_channel()

[Nuno Sa]

Use the new IIO accessor for iterating over the active channels.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index cf73c6f46c79..1cc2a66a78ff 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -1608,8 +1608,7 @@ static int adis16475_push_single_sample(struct iio_poll_func *pf)
 		return -EINVAL;
 	}
 
-	for_each_set_bit(bit, indio_dev->active_scan_mask,
-			 indio_dev->masklength) {
+	iio_for_each_active_channel(indio_dev, bit) {
 		/*
 		 * When burst mode is used, system flags is the first data
 		 * channel in the sequence, but the scan index is 7.

-- 
2.45.2

[PATCH RFC 3/3] iio: core annotate masklength as private

[Nuno Sa]

Annotate 'masklength' as private so that we can get checker warnings if
drivers directly access that variable.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/iio/iio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index a63888746707..94677240df64 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -609,7 +609,7 @@ struct iio_dev {
 	int				scan_bytes;
 
 	const unsigned long		*available_scan_masks;
-	unsigned			masklength;
+	unsigned			__private masklength;
 	const unsigned long		*active_scan_mask;
 	bool				scan_timestamp;
 	struct iio_trigger		*trig;

-- 
2.45.2

Re: [PATCH RFC 0/3] iio: add helpers and accessors for active channels and masklength

[Jonathan Cameron]

On Wed, 12 Jun 2024 16:20:47 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> Hi Jonathan,
> 
> In [1], you suggested for an iterator for the active channels (so driver
> don't directly access masklength). This RFC showcases that iterator and
> goes one step further by giving an accessors for masklength so that
> drivers can read that variable (we have drivers doing that). The
> accessors uses ACCESS_PRIVATE() so it will warn us if some driver
> directly access the variable making it more difficult to mess with it
> (like changing it's value) without being noticed during review (or the
> auto builders).
> 
> Anyways, before jumping in changing all the drivers using this, I guess
> the questions are:
> 
> 1) Is the iterator useful enough to add one (kind of like it and save a
> line of code :))?
> 2) Do we care about going with the work of marking masklength private? 
> 
> If we go ahead the plan would be:
> 
> 1) Add the helpers macros;
> 2) Convert all drivers that directly access 'masklength';
> 3) Annotate it as private.
> 
> [1]: https://lore.kernel.org/linux-iio/20240428142343.5067c898@jic23-huawei/

Cute. I'd not seen the __private bit before.

Looks good to me.  I think we should spin it a little differently.
1. Add macro and a dummy 

#define iio_dev_mask_length(indio_dev) (indio_dev)->mask_length

2. Convert drivers

3. What you have + the ACCESS_PRIVATE change.

that accessor still lets people change it rather than making
it strictly private.   I wonder if we need a little more complicated

static inline int iio_dev_mask_length(struct iio_dev *indio_dev)
{
	return ACCESS_PRIVATE()...
}

or can just review for anyone doing iio_dev_mask_length(indio_dev) = 4;




> 
> ---
> Nuno Sa (3):
>       iio: core: add new helper to iterate active channels
>       iio: imu: adis16475: make use of iio_for_each_active_channel()
>       iio: core annotate masklength as private
> 
>  drivers/iio/imu/adis16475.c | 3 +--
>  include/linux/iio/iio.h     | 8 +++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> ---
> base-commit: cc1ce839526a65620778617da0b022bd88e8a139
> change-id: 20240612-dev-iio-scan-private-86f4a0fd288f
> --
> 
> Thanks!
> - Nuno Sá
> 

Re: [PATCH RFC 0/3] iio: add helpers and accessors for active channels and masklength

[Nuno Sá]

On Sat, 2024-06-15 at 14:18 +0100, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 16:20:47 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Hi Jonathan,
> > 
> > In [1], you suggested for an iterator for the active channels (so driver
> > don't directly access masklength). This RFC showcases that iterator and
> > goes one step further by giving an accessors for masklength so that
> > drivers can read that variable (we have drivers doing that). The
> > accessors uses ACCESS_PRIVATE() so it will warn us if some driver
> > directly access the variable making it more difficult to mess with it
> > (like changing it's value) without being noticed during review (or the
> > auto builders).
> > 
> > Anyways, before jumping in changing all the drivers using this, I guess
> > the questions are:
> > 
> > 1) Is the iterator useful enough to add one (kind of like it and save a
> > line of code :))?
> > 2) Do we care about going with the work of marking masklength private? 
> > 
> > If we go ahead the plan would be:
> > 
> > 1) Add the helpers macros;
> > 2) Convert all drivers that directly access 'masklength';
> > 3) Annotate it as private.
> > 
> > [1]: https://lore.kernel.org/linux-iio/20240428142343.5067c898@jic23-huawei/
> 
> Cute. I'd not seen the __private bit before.

Yeah, I first noticed it in <linux/irq.h>
> 
> Looks good to me.  I think we should spin it a little differently.
> 1. Add macro and a dummy 
> 
> #define iio_dev_mask_length(indio_dev) (indio_dev)->mask_length
> 
> 2. Convert drivers
> 
> 3. What you have + the ACCESS_PRIVATE change.
> 

Agreed. Looks better

> that accessor still lets people change it rather than making
> it strictly private.   I wonder if we need a little more complicated
> 

I do prefer your inline function as it's a stronger guarantee... Though changing it
through the macro is also odd enough that should clearly pick the reviewers
attention.

- Nuno Sá