Re: [PATCH v3] iio: common: st_sensors: fix possible infinite loop in st_sensors_irq_thread

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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    
> > > > >    
> > >   
> >   
>