diff mbox series

[03/10] iio: magnetometer: rm3100: Stop abusing the ->currentmode

Message ID 20211215151344.163036-4-miquel.raynal@bootlin.com (mailing list archive)
State Changes Requested
Headers show
Series Light core IIO enhancements | expand

Commit Message

Miquel Raynal Dec. 15, 2021, 3:13 p.m. UTC 
This is an internal variable for the core, here it is set to a "default"
value by the driver in order to later be able to perform checks against
it. None of this is needed because this check actually cares about the
buffers being enabled or not. So it is an unproper side-channel access
to the information "are the buffers enabled?", returned officially by
the iio_buffer_enabled() helper. Use this helper instead.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

Comments

Alexandru Ardelean Dec. 16, 2021, 6:40 a.m. UTC | #1 
On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> This is an internal variable for the core, here it is set to a "default"
> value by the driver in order to later be able to perform checks against
> it. None of this is needed because this check actually cares about the
> buffers being enabled or not. So it is an unproper side-channel access
> to the information "are the buffers enabled?", returned officially by
> the iio_buffer_enabled() helper. Use this helper instead.

A few comments inline.

>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> index 13914273c999..be0057f82218 100644
> --- a/drivers/iio/magnetometer/rm3100-core.c
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
>         struct iio_dev *indio_dev = d;
>         struct rm3100_data *data = iio_priv(indio_dev);
>
> -       switch (indio_dev->currentmode) {
> -       case INDIO_DIRECT_MODE:
> +       if (!iio_buffer_enabled(indio_dev))
>                 complete(&data->measuring_done);
> -               break;
> -       case INDIO_BUFFER_TRIGGERED:

This is one of those semantic changes that looks correct, but may end
up getting comments that it should be validated by someone with
hardware [and for good reason].
Especially in places like Ref1 (below).

But I guess the iio_get_internal_mode() helper could still be used.
I guess I'd wait for more opinions on this.

> +       else
>                 iio_trigger_poll(data->drdy_trig);
> -               break;
> -       default:
> -               dev_err(indio_dev->dev.parent,
> -                       "device mode out of control, current mode: %d",
> -                       indio_dev->currentmode);
> -       }
>
>         return IRQ_WAKE_THREAD;
>  }
> @@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
>                         goto unlock_return;
>         }
>
> -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {

Ref1

> +       if (iio_buffer_enabled(indio_dev)) {
>                 /* Writing TMRC registers requires CMM reset. */
>                 ret = regmap_write(regmap, RM3100_REG_CMM, 0);
>                 if (ret < 0)
> @@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
>         indio_dev->channels = rm3100_channels;
>         indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
>         indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> -       indio_dev->currentmode = INDIO_DIRECT_MODE;

This is fine :)

>
>         if (!irq)
>                 data->use_interrupt = false;
> --
> 2.27.0
>
Miquel Raynal Dec. 16, 2021, 8:18 a.m. UTC | #2 
Hi Alexandru,

ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 08:40:12 +0200:

> On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > This is an internal variable for the core, here it is set to a "default"
> > value by the driver in order to later be able to perform checks against
> > it. None of this is needed because this check actually cares about the
> > buffers being enabled or not. So it is an unproper side-channel access
> > to the information "are the buffers enabled?", returned officially by
> > the iio_buffer_enabled() helper. Use this helper instead.  
> 
> A few comments inline.
> 
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
> >  1 file changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> > index 13914273c999..be0057f82218 100644
> > --- a/drivers/iio/magnetometer/rm3100-core.c
> > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > @@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
> >         struct iio_dev *indio_dev = d;
> >         struct rm3100_data *data = iio_priv(indio_dev);
> >
> > -       switch (indio_dev->currentmode) {
> > -       case INDIO_DIRECT_MODE:
> > +       if (!iio_buffer_enabled(indio_dev))
> >                 complete(&data->measuring_done);
> > -               break;
> > -       case INDIO_BUFFER_TRIGGERED:  
> 
> This is one of those semantic changes that looks correct, but may end
> up getting comments that it should be validated by someone with
> hardware [and for good reason].
> Especially in places like Ref1 (below).

I think here we are pretty safe assuming that the change will not break
the driver because this is a construction that is very common in IIO
drivers.

Below, as stated in the cover letter, this is just my own
understanding and I'll happily drop the change if someone
thinks/observes this is unsafe.

> But I guess the iio_get_internal_mode() helper could still be used.
> I guess I'd wait for more opinions on this.
> 
> > +       else
> >                 iio_trigger_poll(data->drdy_trig);
> > -               break;
> > -       default:
> > -               dev_err(indio_dev->dev.parent,
> > -                       "device mode out of control, current mode: %d",
> > -                       indio_dev->currentmode);
> > -       }
> >
> >         return IRQ_WAKE_THREAD;
> >  }
> > @@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
> >                         goto unlock_return;
> >         }
> >
> > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {  
> 
> Ref1
> 
> > +       if (iio_buffer_enabled(indio_dev)) {
> >                 /* Writing TMRC registers requires CMM reset. */
> >                 ret = regmap_write(regmap, RM3100_REG_CMM, 0);
> >                 if (ret < 0)
> > @@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
> >         indio_dev->channels = rm3100_channels;
> >         indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
> >         indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> > -       indio_dev->currentmode = INDIO_DIRECT_MODE;  
> 
> This is fine :)
> 
> >
> >         if (!irq)
> >                 data->use_interrupt = false;
> > --
> > 2.27.0
> >  


Thanks,
Miquèl
Jonathan Cameron Jan. 15, 2022, 3:58 p.m. UTC | #3 
On Wed, 15 Dec 2021 16:13:37 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> This is an internal variable for the core, here it is set to a "default"
> value by the driver in order to later be able to perform checks against
> it. None of this is needed because this check actually cares about the
> buffers being enabled or not. So it is an unproper side-channel access
> to the information "are the buffers enabled?", returned officially by
> the iio_buffer_enabled() helper. Use this helper instead.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Hi Miquel,

Make sure to +cc driver authors on v2.

Whilst I think this is safe we should definitely give them visibility.

Obviously some IIO drivers probably have authors who have long moved on
but this one is only 2018 vintage so Song Qiang might still have
access to hardware or be willing to do a review!

Jonathan


> ---
>  drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> index 13914273c999..be0057f82218 100644
> --- a/drivers/iio/magnetometer/rm3100-core.c
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
>  	struct iio_dev *indio_dev = d;
>  	struct rm3100_data *data = iio_priv(indio_dev);
>  
> -	switch (indio_dev->currentmode) {
> -	case INDIO_DIRECT_MODE:
> +	if (!iio_buffer_enabled(indio_dev))
>  		complete(&data->measuring_done);
> -		break;
> -	case INDIO_BUFFER_TRIGGERED:
> +	else
>  		iio_trigger_poll(data->drdy_trig);
> -		break;
> -	default:
> -		dev_err(indio_dev->dev.parent,
> -			"device mode out of control, current mode: %d",
> -			indio_dev->currentmode);
> -	}
>  
>  	return IRQ_WAKE_THREAD;
>  }
> @@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
>  			goto unlock_return;
>  	}
>  
> -	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +	if (iio_buffer_enabled(indio_dev)) {
>  		/* Writing TMRC registers requires CMM reset. */
>  		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
>  		if (ret < 0)
> @@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
>  	indio_dev->channels = rm3100_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
>  	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> -	indio_dev->currentmode = INDIO_DIRECT_MODE;
>  
>  	if (!irq)
>  		data->use_interrupt = false;
Miquel Raynal Jan. 28, 2022, 3 p.m. UTC | #4 
Hi Jonathan,

jic23@kernel.org wrote on Sat, 15 Jan 2022 15:58:32 +0000:

> On Wed, 15 Dec 2021 16:13:37 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > This is an internal variable for the core, here it is set to a "default"
> > value by the driver in order to later be able to perform checks against
> > it. None of this is needed because this check actually cares about the
> > buffers being enabled or not. So it is an unproper side-channel access
> > to the information "are the buffers enabled?", returned officially by
> > the iio_buffer_enabled() helper. Use this helper instead.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> Hi Miquel,
> 
> Make sure to +cc driver authors on v2.
> 
> Whilst I think this is safe we should definitely give them visibility.
> 
> Obviously some IIO drivers probably have authors who have long moved on
> but this one is only 2018 vintage so Song Qiang might still have
> access to hardware or be willing to do a review!

Right, I've added Song Qiang into a Cc: tag for this patch for the next
iteration.

Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 13914273c999..be0057f82218 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -141,18 +141,10 @@  static irqreturn_t rm3100_irq_handler(int irq, void *d)
 	struct iio_dev *indio_dev = d;
 	struct rm3100_data *data = iio_priv(indio_dev);
 
-	switch (indio_dev->currentmode) {
-	case INDIO_DIRECT_MODE:
+	if (!iio_buffer_enabled(indio_dev))
 		complete(&data->measuring_done);
-		break;
-	case INDIO_BUFFER_TRIGGERED:
+	else
 		iio_trigger_poll(data->drdy_trig);
-		break;
-	default:
-		dev_err(indio_dev->dev.parent,
-			"device mode out of control, current mode: %d",
-			indio_dev->currentmode);
-	}
 
 	return IRQ_WAKE_THREAD;
 }
@@ -377,7 +369,7 @@  static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 			goto unlock_return;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_buffer_enabled(indio_dev)) {
 		/* Writing TMRC registers requires CMM reset. */
 		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
 		if (ret < 0)
@@ -553,7 +545,6 @@  int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
 	indio_dev->channels = rm3100_channels;
 	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
 
 	if (!irq)
 		data->use_interrupt = false;