* [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
@ 2020-12-08 14:36 Lorenzo Bianconi
2020-12-09 14:41 ` Andy Shevchenko
2020-12-13 15:04 ` Jonathan Cameron
0 siblings, 2 replies; 10+ messages in thread
From: Lorenzo Bianconi @ 2020-12-08 14:36 UTC (permalink / raw)
To: jic23; +Cc: lorenzo.bianconi, linux-iio, linus.walleij, denis.ciocca
Return a boolean value in st_sensors_new_samples_available routine in
order to avoid an infinite loop in st_sensors_irq_thread if
stat_drdy.addr is not defined or stat_drdy read fails
Fixes: 90efe05562921 ("iio: st_sensors: harden interrupt handling")
Reported-by: Jonathan Cameron <jic23@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes since v2:
- return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
status register
Changes since v1:
- return true if the sensor does not have stat_drdy register
---
.../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 0507283bd4c1..2dbd2646e44e 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -23,35 +23,31 @@
* @sdata: Sensor data.
*
* returns:
- * 0 - no new samples available
- * 1 - new samples available
- * negative - error or unknown
+ * false - no new samples available or read error
+ * true - new samples available
*/
-static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
- struct st_sensor_data *sdata)
+static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
+ struct st_sensor_data *sdata)
{
int ret, status;
/* How would I know if I can't check it? */
if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
- return -EINVAL;
+ return true;
/* No scan mask, no interrupt */
if (!indio_dev->active_scan_mask)
- return 0;
+ return false;
ret = regmap_read(sdata->regmap,
sdata->sensor_settings->drdy_irq.stat_drdy.addr,
&status);
if (ret < 0) {
dev_err(sdata->dev, "error checking samples available\n");
- return ret;
+ return false;
}
- if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
- return 1;
-
- return 0;
+ return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
}
/**
@@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
/* Tell the interrupt handler that we're dealing with edges */
if (irq_trig == IRQF_TRIGGER_FALLING ||
- irq_trig == IRQF_TRIGGER_RISING)
+ irq_trig == IRQF_TRIGGER_RISING) {
+ if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
+ dev_err(&indio_dev->dev,
+ "edge IRQ not supported w/o stat register.\n");
+ err = -EOPNOTSUPP;
+ goto iio_trigger_free;
+ }
sdata->edge_irq = true;
- else
+ } else {
/*
* If we're not using edges (i.e. level interrupts) we
* just mask off the IRQ, handle one interrupt, then
@@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
* interrupt handler top half again and start over.
*/
irq_trig |= IRQF_ONESHOT;
+ }
/*
* If the interrupt pin is Open Drain, by definition this
--
2.28.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-08 14:36 [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread Lorenzo Bianconi
@ 2020-12-09 14:41 ` Andy Shevchenko
2020-12-09 14:42 ` Andy Shevchenko
2020-12-13 15:04 ` Jonathan Cameron
1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-12-09 14:41 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Jonathan Cameron, lorenzo.bianconi, linux-iio, Linus Walleij,
Denis Ciocca
On Tue, Dec 8, 2020 at 4:38 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> Return a boolean value in st_sensors_new_samples_available routine in
> order to avoid an infinite loop in st_sensors_irq_thread if
> stat_drdy.addr is not defined or stat_drdy read fails
...
> + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> + dev_err(&indio_dev->dev,
> + "edge IRQ not supported w/o stat register.\n");
> + err = -EOPNOTSUPP;
Wrong error code. You must not return it to user space (or you should
fix all call sites to be sure this won't be returned).
> + goto iio_trigger_free;
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-09 14:41 ` Andy Shevchenko
@ 2020-12-09 14:42 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-12-09 14:42 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Jonathan Cameron, lorenzo.bianconi, linux-iio, Linus Walleij,
Denis Ciocca
On Wed, Dec 9, 2020 at 4:41 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Dec 8, 2020 at 4:38 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
...
> > + err = -EOPNOTSUPP;
>
> Wrong error code. You must not return it to user space (or you should
> fix all call sites to be sure this won't be returned).
Oops, seems the other way around. Sorry for the noise.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-08 14:36 [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread Lorenzo Bianconi
2020-12-09 14:41 ` Andy Shevchenko
@ 2020-12-13 15:04 ` Jonathan Cameron
2020-12-13 19:23 ` Lorenzo Bianconi
1 sibling, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2020-12-13 15:04 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: lorenzo.bianconi, linux-iio, linus.walleij, denis.ciocca
On Tue, 8 Dec 2020 15:36:40 +0100
Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> Return a boolean value in st_sensors_new_samples_available routine in
> order to avoid an infinite loop in st_sensors_irq_thread if
> stat_drdy.addr is not defined or stat_drdy read fails
>
> Fixes: 90efe05562921 ("iio: st_sensors: harden interrupt handling")
> Reported-by: Jonathan Cameron <jic23@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
One trivial inline. Note I'm looking for an Ack form Dennis on this one
as it may result in a some breakage if any devices are using
edge interrupts.
It's possible we would be better falling back to interrupt free support
in that case rather than failing to probe at all.
I think that would be best done by moving the check out to the
various per type drivers so we fail in the same fashion as no irq
provided + a warning.
thanks,
Jonathan
> ---
> Changes since v2:
> - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> status register
>
> Changes since v1:
> - return true if the sensor does not have stat_drdy register
> ---
> .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 0507283bd4c1..2dbd2646e44e 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -23,35 +23,31 @@
> * @sdata: Sensor data.
> *
> * returns:
> - * 0 - no new samples available
> - * 1 - new samples available
> - * negative - error or unknown
> + * false - no new samples available or read error
> + * true - new samples available
> */
> -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> - struct st_sensor_data *sdata)
> +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> + struct st_sensor_data *sdata)
> {
> int ret, status;
>
> /* How would I know if I can't check it? */
> if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> - return -EINVAL;
> + return true;
>
> /* No scan mask, no interrupt */
> if (!indio_dev->active_scan_mask)
> - return 0;
> + return false;
>
> ret = regmap_read(sdata->regmap,
> sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> &status);
> if (ret < 0) {
> dev_err(sdata->dev, "error checking samples available\n");
> - return ret;
> + return false;
> }
>
> - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> - return 1;
> -
> - return 0;
> + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
No need for the !! as you can rely on type conversion to a boolean.
> }
>
> /**
> @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>
> /* Tell the interrupt handler that we're dealing with edges */
> if (irq_trig == IRQF_TRIGGER_FALLING ||
> - irq_trig == IRQF_TRIGGER_RISING)
> + irq_trig == IRQF_TRIGGER_RISING) {
> + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> + dev_err(&indio_dev->dev,
> + "edge IRQ not supported w/o stat register.\n");
> + err = -EOPNOTSUPP;
> + goto iio_trigger_free;
> + }
> sdata->edge_irq = true;
> - else
> + } else {
> /*
> * If we're not using edges (i.e. level interrupts) we
> * just mask off the IRQ, handle one interrupt, then
> @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> * interrupt handler top half again and start over.
> */
> irq_trig |= IRQF_ONESHOT;
> + }
>
> /*
> * If the interrupt pin is Open Drain, by definition this
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-13 15:04 ` Jonathan Cameron
@ 2020-12-13 19:23 ` Lorenzo Bianconi
2020-12-22 10:53 ` Lorenzo Bianconi
0 siblings, 1 reply; 10+ messages in thread
From: Lorenzo Bianconi @ 2020-12-13 19:23 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lorenzo Bianconi, linux-iio, Linus Walleij, Denis CIOCCA
>
> On Tue, 8 Dec 2020 15:36:40 +0100
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > Return a boolean value in st_sensors_new_samples_available routine in
> > order to avoid an infinite loop in st_sensors_irq_thread if
> > stat_drdy.addr is not defined or stat_drdy read fails
> >
> > Fixes: 90efe05562921 ("iio: st_sensors: harden interrupt handling")
> > Reported-by: Jonathan Cameron <jic23@kernel.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>
> One trivial inline. Note I'm looking for an Ack form Dennis on this one
> as it may result in a some breakage if any devices are using
> edge interrupts.
Looking at the current code I am wondering if it is possible since we
would have already triggered the infinite loop in this case. I think
nobody is currently using edge interrupts if the status register is
not available. What do you think?
Regards,
Lorenzo
>
> It's possible we would be better falling back to interrupt free support
> in that case rather than failing to probe at all.
> I think that would be best done by moving the check out to the
> various per type drivers so we fail in the same fashion as no irq
> provided + a warning.
>
> thanks,
>
> Jonathan
>
>
> > ---
> > Changes since v2:
> > - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> > status register
> >
> > Changes since v1:
> > - return true if the sensor does not have stat_drdy register
> > ---
> > .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> > 1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > index 0507283bd4c1..2dbd2646e44e 100644
> > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > @@ -23,35 +23,31 @@
> > * @sdata: Sensor data.
> > *
> > * returns:
> > - * 0 - no new samples available
> > - * 1 - new samples available
> > - * negative - error or unknown
> > + * false - no new samples available or read error
> > + * true - new samples available
> > */
> > -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > - struct st_sensor_data *sdata)
> > +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > + struct st_sensor_data *sdata)
> > {
> > int ret, status;
> >
> > /* How would I know if I can't check it? */
> > if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > - return -EINVAL;
> > + return true;
> >
> > /* No scan mask, no interrupt */
> > if (!indio_dev->active_scan_mask)
> > - return 0;
> > + return false;
> >
> > ret = regmap_read(sdata->regmap,
> > sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> > &status);
> > if (ret < 0) {
> > dev_err(sdata->dev, "error checking samples available\n");
> > - return ret;
> > + return false;
> > }
> >
> > - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> > - return 1;
> > -
> > - return 0;
> > + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
>
> No need for the !! as you can rely on type conversion to a boolean.
>
> > }
> >
> > /**
> > @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> >
> > /* Tell the interrupt handler that we're dealing with edges */
> > if (irq_trig == IRQF_TRIGGER_FALLING ||
> > - irq_trig == IRQF_TRIGGER_RISING)
> > + irq_trig == IRQF_TRIGGER_RISING) {
> > + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> > + dev_err(&indio_dev->dev,
> > + "edge IRQ not supported w/o stat register.\n");
> > + err = -EOPNOTSUPP;
> > + goto iio_trigger_free;
> > + }
> > sdata->edge_irq = true;
> > - else
> > + } else {
> > /*
> > * If we're not using edges (i.e. level interrupts) we
> > * just mask off the IRQ, handle one interrupt, then
> > @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > * interrupt handler top half again and start over.
> > */
> > irq_trig |= IRQF_ONESHOT;
> > + }
> >
> > /*
> > * If the interrupt pin is Open Drain, by definition this
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-13 19:23 ` Lorenzo Bianconi
@ 2020-12-22 10:53 ` Lorenzo Bianconi
2020-12-30 12:08 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Lorenzo Bianconi @ 2020-12-22 10:53 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lorenzo Bianconi, linux-iio, Linus Walleij, Denis CIOCCA
>
> >
> > On Tue, 8 Dec 2020 15:36:40 +0100
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > > Return a boolean value in st_sensors_new_samples_available routine in
> > > order to avoid an infinite loop in st_sensors_irq_thread if
> > > stat_drdy.addr is not defined or stat_drdy read fails
> > >
> > > Fixes: 90efe05562921 ("iio: st_sensors: harden interrupt handling")
> > > Reported-by: Jonathan Cameron <jic23@kernel.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> >
> > One trivial inline. Note I'm looking for an Ack form Dennis on this one
> > as it may result in a some breakage if any devices are using
> > edge interrupts.
>
> Looking at the current code I am wondering if it is possible since we
> would have already triggered the infinite loop in this case. I think
> nobody is currently using edge interrupts if the status register is
> not available. What do you think?
>
Hi Jonathan,
any news about this patch?
Regards,
Lorenzo
> Regards,
> Lorenzo
>
> >
> > It's possible we would be better falling back to interrupt free support
> > in that case rather than failing to probe at all.
> > I think that would be best done by moving the check out to the
> > various per type drivers so we fail in the same fashion as no irq
> > provided + a warning.
>
>
> >
> > thanks,
> >
> > Jonathan
> >
> >
> > > ---
> > > Changes since v2:
> > > - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> > > status register
> > >
> > > Changes since v1:
> > > - return true if the sensor does not have stat_drdy register
> > > ---
> > > .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > index 0507283bd4c1..2dbd2646e44e 100644
> > > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > @@ -23,35 +23,31 @@
> > > * @sdata: Sensor data.
> > > *
> > > * returns:
> > > - * 0 - no new samples available
> > > - * 1 - new samples available
> > > - * negative - error or unknown
> > > + * false - no new samples available or read error
> > > + * true - new samples available
> > > */
> > > -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > - struct st_sensor_data *sdata)
> > > +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > + struct st_sensor_data *sdata)
> > > {
> > > int ret, status;
> > >
> > > /* How would I know if I can't check it? */
> > > if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > > - return -EINVAL;
> > > + return true;
> > >
> > > /* No scan mask, no interrupt */
> > > if (!indio_dev->active_scan_mask)
> > > - return 0;
> > > + return false;
> > >
> > > ret = regmap_read(sdata->regmap,
> > > sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> > > &status);
> > > if (ret < 0) {
> > > dev_err(sdata->dev, "error checking samples available\n");
> > > - return ret;
> > > + return false;
> > > }
> > >
> > > - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> > > - return 1;
> > > -
> > > - return 0;
> > > + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
> >
> > No need for the !! as you can rely on type conversion to a boolean.
> >
> > > }
> > >
> > > /**
> > > @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > >
> > > /* Tell the interrupt handler that we're dealing with edges */
> > > if (irq_trig == IRQF_TRIGGER_FALLING ||
> > > - irq_trig == IRQF_TRIGGER_RISING)
> > > + irq_trig == IRQF_TRIGGER_RISING) {
> > > + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> > > + dev_err(&indio_dev->dev,
> > > + "edge IRQ not supported w/o stat register.\n");
> > > + err = -EOPNOTSUPP;
> > > + goto iio_trigger_free;
> > > + }
> > > sdata->edge_irq = true;
> > > - else
> > > + } else {
> > > /*
> > > * If we're not using edges (i.e. level interrupts) we
> > > * just mask off the IRQ, handle one interrupt, then
> > > @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > * interrupt handler top half again and start over.
> > > */
> > > irq_trig |= IRQF_ONESHOT;
> > > + }
> > >
> > > /*
> > > * If the interrupt pin is Open Drain, by definition this
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-22 10:53 ` Lorenzo Bianconi
@ 2020-12-30 12:08 ` Jonathan Cameron
2021-01-11 8:21 ` Lorenzo Bianconi
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2020-12-30 12:08 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, linux-iio, Linus Walleij, Denis CIOCCA
On Tue, 22 Dec 2020 11:53:57 +0100
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> >
> > >
> > > On Tue, 8 Dec 2020 15:36:40 +0100
> > > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > >
> > > > Return a boolean value in st_sensors_new_samples_available routine in
> > > > order to avoid an infinite loop in st_sensors_irq_thread if
> > > > stat_drdy.addr is not defined or stat_drdy read fails
> > > >
> > > > Fixes: 90efe05562921 ("iio: st_sensors: harden interrupt handling")
> > > > Reported-by: Jonathan Cameron <jic23@kernel.org>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > >
> > > One trivial inline. Note I'm looking for an Ack form Dennis on this one
> > > as it may result in a some breakage if any devices are using
> > > edge interrupts.
> >
> > Looking at the current code I am wondering if it is possible since we
> > would have already triggered the infinite loop in this case. I think
> > nobody is currently using edge interrupts if the status register is
> > not available. What do you think?
> >
>
> Hi Jonathan,
>
> any news about this patch?
>
> Regards,
> Lorenzo
It's a very minor gamble, so I've applied it to the fixes-togreg branch of
iio.git. Fingers crossed no one hits the corner case and has somehow been
getting away with it.
Thanks,
Jonathan
>
> > Regards,
> > Lorenzo
> >
> > >
> > > It's possible we would be better falling back to interrupt free support
> > > in that case rather than failing to probe at all.
> > > I think that would be best done by moving the check out to the
> > > various per type drivers so we fail in the same fashion as no irq
> > > provided + a warning.
> >
> >
> > >
> > > thanks,
> > >
> > > Jonathan
> > >
> > >
> > > > ---
> > > > Changes since v2:
> > > > - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> > > > status register
> > > >
> > > > Changes since v1:
> > > > - return true if the sensor does not have stat_drdy register
> > > > ---
> > > > .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> > > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > index 0507283bd4c1..2dbd2646e44e 100644
> > > > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > @@ -23,35 +23,31 @@
> > > > * @sdata: Sensor data.
> > > > *
> > > > * returns:
> > > > - * 0 - no new samples available
> > > > - * 1 - new samples available
> > > > - * negative - error or unknown
> > > > + * false - no new samples available or read error
> > > > + * true - new samples available
> > > > */
> > > > -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > - struct st_sensor_data *sdata)
> > > > +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > + struct st_sensor_data *sdata)
> > > > {
> > > > int ret, status;
> > > >
> > > > /* How would I know if I can't check it? */
> > > > if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > > > - return -EINVAL;
> > > > + return true;
> > > >
> > > > /* No scan mask, no interrupt */
> > > > if (!indio_dev->active_scan_mask)
> > > > - return 0;
> > > > + return false;
> > > >
> > > > ret = regmap_read(sdata->regmap,
> > > > sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> > > > &status);
> > > > if (ret < 0) {
> > > > dev_err(sdata->dev, "error checking samples available\n");
> > > > - return ret;
> > > > + return false;
> > > > }
> > > >
> > > > - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> > > > - return 1;
> > > > -
> > > > - return 0;
> > > > + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
> > >
> > > No need for the !! as you can rely on type conversion to a boolean.
> > >
> > > > }
> > > >
> > > > /**
> > > > @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > >
> > > > /* Tell the interrupt handler that we're dealing with edges */
> > > > if (irq_trig == IRQF_TRIGGER_FALLING ||
> > > > - irq_trig == IRQF_TRIGGER_RISING)
> > > > + irq_trig == IRQF_TRIGGER_RISING) {
> > > > + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> > > > + dev_err(&indio_dev->dev,
> > > > + "edge IRQ not supported w/o stat register.\n");
> > > > + err = -EOPNOTSUPP;
> > > > + goto iio_trigger_free;
> > > > + }
> > > > sdata->edge_irq = true;
> > > > - else
> > > > + } else {
> > > > /*
> > > > * If we're not using edges (i.e. level interrupts) we
> > > > * just mask off the IRQ, handle one interrupt, then
> > > > @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > * interrupt handler top half again and start over.
> > > > */
> > > > irq_trig |= IRQF_ONESHOT;
> > > > + }
> > > >
> > > > /*
> > > > * If the interrupt pin is Open Drain, by definition this
> > >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2020-12-30 12:08 ` Jonathan Cameron
@ 2021-01-11 8:21 ` Lorenzo Bianconi
2021-01-11 13:10 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Lorenzo Bianconi @ 2021-01-11 8:21 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lorenzo Bianconi, linux-iio, Linus Walleij, Denis CIOCCA
[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]
> On Tue, 22 Dec 2020 11:53:57 +0100
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
>
> > >
[...]
> It's a very minor gamble, so I've applied it to the fixes-togreg branch of
> iio.git. Fingers crossed no one hits the corner case and has somehow been
> getting away with it.
>
> Thanks,
Hi Jonathan,
I am not able to find this commit in your tree. Am I missing something?
Regards,
Lorenzo
>
> Jonathan
>
> >
> > > Regards,
> > > Lorenzo
> > >
> > > >
> > > > It's possible we would be better falling back to interrupt free support
> > > > in that case rather than failing to probe at all.
> > > > I think that would be best done by moving the check out to the
> > > > various per type drivers so we fail in the same fashion as no irq
> > > > provided + a warning.
> > >
> > >
> > > >
> > > > thanks,
> > > >
> > > > Jonathan
> > > >
> > > >
> > > > > ---
> > > > > Changes since v2:
> > > > > - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> > > > > status register
> > > > >
> > > > > Changes since v1:
> > > > > - return true if the sensor does not have stat_drdy register
> > > > > ---
> > > > > .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> > > > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > index 0507283bd4c1..2dbd2646e44e 100644
> > > > > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > @@ -23,35 +23,31 @@
> > > > > * @sdata: Sensor data.
> > > > > *
> > > > > * returns:
> > > > > - * 0 - no new samples available
> > > > > - * 1 - new samples available
> > > > > - * negative - error or unknown
> > > > > + * false - no new samples available or read error
> > > > > + * true - new samples available
> > > > > */
> > > > > -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > > - struct st_sensor_data *sdata)
> > > > > +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > > + struct st_sensor_data *sdata)
> > > > > {
> > > > > int ret, status;
> > > > >
> > > > > /* How would I know if I can't check it? */
> > > > > if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > > > > - return -EINVAL;
> > > > > + return true;
> > > > >
> > > > > /* No scan mask, no interrupt */
> > > > > if (!indio_dev->active_scan_mask)
> > > > > - return 0;
> > > > > + return false;
> > > > >
> > > > > ret = regmap_read(sdata->regmap,
> > > > > sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> > > > > &status);
> > > > > if (ret < 0) {
> > > > > dev_err(sdata->dev, "error checking samples available\n");
> > > > > - return ret;
> > > > > + return false;
> > > > > }
> > > > >
> > > > > - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> > > > > - return 1;
> > > > > -
> > > > > - return 0;
> > > > > + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
> > > >
> > > > No need for the !! as you can rely on type conversion to a boolean.
> > > >
> > > > > }
> > > > >
> > > > > /**
> > > > > @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > >
> > > > > /* Tell the interrupt handler that we're dealing with edges */
> > > > > if (irq_trig == IRQF_TRIGGER_FALLING ||
> > > > > - irq_trig == IRQF_TRIGGER_RISING)
> > > > > + irq_trig == IRQF_TRIGGER_RISING) {
> > > > > + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> > > > > + dev_err(&indio_dev->dev,
> > > > > + "edge IRQ not supported w/o stat register.\n");
> > > > > + err = -EOPNOTSUPP;
> > > > > + goto iio_trigger_free;
> > > > > + }
> > > > > sdata->edge_irq = true;
> > > > > - else
> > > > > + } else {
> > > > > /*
> > > > > * If we're not using edges (i.e. level interrupts) we
> > > > > * just mask off the IRQ, handle one interrupt, then
> > > > > @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > > * interrupt handler top half again and start over.
> > > > > */
> > > > > irq_trig |= IRQF_ONESHOT;
> > > > > + }
> > > > >
> > > > > /*
> > > > > * If the interrupt pin is Open Drain, by definition this
> > > >
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2021-01-11 8:21 ` Lorenzo Bianconi
@ 2021-01-11 13:10 ` Jonathan Cameron
2021-01-11 13:26 ` Lorenzo Bianconi
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2021-01-11 13:10 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Jonathan Cameron, Lorenzo Bianconi, linux-iio, Linus Walleij,
Denis CIOCCA
On Mon, 11 Jan 2021 09:21:34 +0100
Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > On Tue, 22 Dec 2020 11:53:57 +0100
> > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> >
> > > >
>
> [...]
>
> > It's a very minor gamble, so I've applied it to the fixes-togreg branch of
> > iio.git. Fingers crossed no one hits the corner case and has somehow been
> > getting away with it.
> >
> > Thanks,
>
> Hi Jonathan,
>
> I am not able to find this commit in your tree. Am I missing something?
oops. Now pushed out.
J
>
> Regards,
> Lorenzo
>
> >
> > Jonathan
> >
> > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > >
> > > > > It's possible we would be better falling back to interrupt free support
> > > > > in that case rather than failing to probe at all.
> > > > > I think that would be best done by moving the check out to the
> > > > > various per type drivers so we fail in the same fashion as no irq
> > > > > provided + a warning.
> > > >
> > > >
> > > > >
> > > > > thanks,
> > > > >
> > > > > Jonathan
> > > > >
> > > > >
> > > > > > ---
> > > > > > Changes since v2:
> > > > > > - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> > > > > > status register
> > > > > >
> > > > > > Changes since v1:
> > > > > > - return true if the sensor does not have stat_drdy register
> > > > > > ---
> > > > > > .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> > > > > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > > index 0507283bd4c1..2dbd2646e44e 100644
> > > > > > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > > @@ -23,35 +23,31 @@
> > > > > > * @sdata: Sensor data.
> > > > > > *
> > > > > > * returns:
> > > > > > - * 0 - no new samples available
> > > > > > - * 1 - new samples available
> > > > > > - * negative - error or unknown
> > > > > > + * false - no new samples available or read error
> > > > > > + * true - new samples available
> > > > > > */
> > > > > > -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > > > - struct st_sensor_data *sdata)
> > > > > > +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > > > + struct st_sensor_data *sdata)
> > > > > > {
> > > > > > int ret, status;
> > > > > >
> > > > > > /* How would I know if I can't check it? */
> > > > > > if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > > > > > - return -EINVAL;
> > > > > > + return true;
> > > > > >
> > > > > > /* No scan mask, no interrupt */
> > > > > > if (!indio_dev->active_scan_mask)
> > > > > > - return 0;
> > > > > > + return false;
> > > > > >
> > > > > > ret = regmap_read(sdata->regmap,
> > > > > > sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> > > > > > &status);
> > > > > > if (ret < 0) {
> > > > > > dev_err(sdata->dev, "error checking samples available\n");
> > > > > > - return ret;
> > > > > > + return false;
> > > > > > }
> > > > > >
> > > > > > - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> > > > > > - return 1;
> > > > > > -
> > > > > > - return 0;
> > > > > > + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
> > > > >
> > > > > No need for the !! as you can rely on type conversion to a boolean.
> > > > >
> > > > > > }
> > > > > >
> > > > > > /**
> > > > > > @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > > >
> > > > > > /* Tell the interrupt handler that we're dealing with edges */
> > > > > > if (irq_trig == IRQF_TRIGGER_FALLING ||
> > > > > > - irq_trig == IRQF_TRIGGER_RISING)
> > > > > > + irq_trig == IRQF_TRIGGER_RISING) {
> > > > > > + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> > > > > > + dev_err(&indio_dev->dev,
> > > > > > + "edge IRQ not supported w/o stat register.\n");
> > > > > > + err = -EOPNOTSUPP;
> > > > > > + goto iio_trigger_free;
> > > > > > + }
> > > > > > sdata->edge_irq = true;
> > > > > > - else
> > > > > > + } else {
> > > > > > /*
> > > > > > * If we're not using edges (i.e. level interrupts) we
> > > > > > * just mask off the IRQ, handle one interrupt, then
> > > > > > @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > > > * interrupt handler top half again and start over.
> > > > > > */
> > > > > > irq_trig |= IRQF_ONESHOT;
> > > > > > + }
> > > > > >
> > > > > > /*
> > > > > > * If the interrupt pin is Open Drain, by definition this
> > > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread
2021-01-11 13:10 ` Jonathan Cameron
@ 2021-01-11 13:26 ` Lorenzo Bianconi
0 siblings, 0 replies; 10+ messages in thread
From: Lorenzo Bianconi @ 2021-01-11 13:26 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lorenzo Bianconi, Jonathan Cameron, linux-iio, Linus Walleij,
Denis CIOCCA
>
> On Mon, 11 Jan 2021 09:21:34 +0100
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > > On Tue, 22 Dec 2020 11:53:57 +0100
> > > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> > >
> > > > >
> >
> > [...]
> >
> > > It's a very minor gamble, so I've applied it to the fixes-togreg branch of
> > > iio.git. Fingers crossed no one hits the corner case and has somehow been
> > > getting away with it.
> > >
> > > Thanks,
> >
> > Hi Jonathan,
> >
> > I am not able to find this commit in your tree. Am I missing something?
>
> oops. Now pushed out.
ack, thx :)
Regards,
Lorenzo
>
> J
>
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Jonathan
> > >
> > > >
> > > > > Regards,
> > > > > Lorenzo
> > > > >
> > > > > >
> > > > > > It's possible we would be better falling back to interrupt free support
> > > > > > in that case rather than failing to probe at all.
> > > > > > I think that would be best done by moving the check out to the
> > > > > > various per type drivers so we fail in the same fashion as no irq
> > > > > > provided + a warning.
> > > > >
> > > > >
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > Jonathan
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > > Changes since v2:
> > > > > > > - return -EOPNOTSUPP if the drv requests edge IRQ and the sensor does not support
> > > > > > > status register
> > > > > > >
> > > > > > > Changes since v1:
> > > > > > > - return true if the sensor does not have stat_drdy register
> > > > > > > ---
> > > > > > > .../common/st_sensors/st_sensors_trigger.c | 31 ++++++++++---------
> > > > > > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > > > index 0507283bd4c1..2dbd2646e44e 100644
> > > > > > > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > > > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > > > > > @@ -23,35 +23,31 @@
> > > > > > > * @sdata: Sensor data.
> > > > > > > *
> > > > > > > * returns:
> > > > > > > - * 0 - no new samples available
> > > > > > > - * 1 - new samples available
> > > > > > > - * negative - error or unknown
> > > > > > > + * false - no new samples available or read error
> > > > > > > + * true - new samples available
> > > > > > > */
> > > > > > > -static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > > > > - struct st_sensor_data *sdata)
> > > > > > > +static bool st_sensors_new_samples_available(struct iio_dev *indio_dev,
> > > > > > > + struct st_sensor_data *sdata)
> > > > > > > {
> > > > > > > int ret, status;
> > > > > > >
> > > > > > > /* How would I know if I can't check it? */
> > > > > > > if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > > > > > > - return -EINVAL;
> > > > > > > + return true;
> > > > > > >
> > > > > > > /* No scan mask, no interrupt */
> > > > > > > if (!indio_dev->active_scan_mask)
> > > > > > > - return 0;
> > > > > > > + return false;
> > > > > > >
> > > > > > > ret = regmap_read(sdata->regmap,
> > > > > > > sdata->sensor_settings->drdy_irq.stat_drdy.addr,
> > > > > > > &status);
> > > > > > > if (ret < 0) {
> > > > > > > dev_err(sdata->dev, "error checking samples available\n");
> > > > > > > - return ret;
> > > > > > > + return false;
> > > > > > > }
> > > > > > >
> > > > > > > - if (status & sdata->sensor_settings->drdy_irq.stat_drdy.mask)
> > > > > > > - return 1;
> > > > > > > -
> > > > > > > - return 0;
> > > > > > > + return !!(status & sdata->sensor_settings->drdy_irq.stat_drdy.mask);
> > > > > >
> > > > > > No need for the !! as you can rely on type conversion to a boolean.
> > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > /**
> > > > > > > @@ -180,9 +176,15 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > > > >
> > > > > > > /* Tell the interrupt handler that we're dealing with edges */
> > > > > > > if (irq_trig == IRQF_TRIGGER_FALLING ||
> > > > > > > - irq_trig == IRQF_TRIGGER_RISING)
> > > > > > > + irq_trig == IRQF_TRIGGER_RISING) {
> > > > > > > + if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> > > > > > > + dev_err(&indio_dev->dev,
> > > > > > > + "edge IRQ not supported w/o stat register.\n");
> > > > > > > + err = -EOPNOTSUPP;
> > > > > > > + goto iio_trigger_free;
> > > > > > > + }
> > > > > > > sdata->edge_irq = true;
> > > > > > > - else
> > > > > > > + } else {
> > > > > > > /*
> > > > > > > * If we're not using edges (i.e. level interrupts) we
> > > > > > > * just mask off the IRQ, handle one interrupt, then
> > > > > > > @@ -190,6 +192,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> > > > > > > * interrupt handler top half again and start over.
> > > > > > > */
> > > > > > > irq_trig |= IRQF_ONESHOT;
> > > > > > > + }
> > > > > > >
> > > > > > > /*
> > > > > > > * If the interrupt pin is Open Drain, by definition this
> > > > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-01-11 13:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-08 14:36 [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread Lorenzo Bianconi
2020-12-09 14:41 ` Andy Shevchenko
2020-12-09 14:42 ` Andy Shevchenko
2020-12-13 15:04 ` Jonathan Cameron
2020-12-13 19:23 ` Lorenzo Bianconi
2020-12-22 10:53 ` Lorenzo Bianconi
2020-12-30 12:08 ` Jonathan Cameron
2021-01-11 8:21 ` Lorenzo Bianconi
2021-01-11 13:10 ` Jonathan Cameron
2021-01-11 13:26 ` Lorenzo Bianconi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).