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

Message

Nuno Sa June 12, 2024, 2:20 p.m. UTC

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á

Comments

Jonathan Cameron June 15, 2024, 1:18 p.m. UTC | #1

On Wed, 12 Jun 2024 16:20:47 +0200
Nuno Sa <[e-mail]> 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á
>

Nuno Sá June 17, 2024, 6:34 a.m. UTC | #2

On Sat, 2024-06-15 at 14:18 +0100, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 16:20:47 +0200
> Nuno Sa <[e-mail]> 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á