>
> >
> > On Tue, 8 Dec 2020 15:36:40 +0100
> > Lorenzo Bianconi <lorenzo@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> >
> > 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
> >
