| Message ID | 20190909112846.55280-6-sean@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/6] iio: imu: st_lsm6dsx: move interrupt thread to core | expand |
> When events and buffered reads is enabled simultaneously, and the first > event accours the interrupt pin stays high. > > This can be reverted when we find a solution to allow events and > buffered reads simultaneously. > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > Changes since v4: > * Use fifo configuration mutex to prevent a race in hw->enable_event check. > > Changes since v5: > * Updated do not return without unlocking mutexes > * Lock mutex before unlock > * Runtime tested ;-) > [...] > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -1340,8 +1340,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, > if (type != IIO_EV_TYPE_THRESH) > return -EINVAL; > > - if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) > - return -EBUSY; > + mutex_lock(&hw->conf_lock); > + > + if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) { > + err = -EBUSY; > + goto out; > + } This patch is still broken!!! Returning in case of error you need to relase the lock. > > /* do not enable events if they are already enabled */ > if (state && hw->enable_event) > @@ -1357,7 +1361,10 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, > > hw->enable_event = state; > > - return 0; > +out: > + mutex_unlock(&hw->conf_lock); > + > + return err; > } > > int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val) > -- > 2.23.0 >
On 09/09/2019 13.55, Lorenzo Bianconi wrote: >> When events and buffered reads is enabled simultaneously, and the first >> event accours the interrupt pin stays high. >> >> This can be reverted when we find a solution to allow events and >> buffered reads simultaneously. >> >> Signed-off-by: Sean Nyekjaer <sean@geanix.com> >> --- >> Changes since v4: >> * Use fifo configuration mutex to prevent a race in hw->enable_event check. >> >> Changes since v5: >> * Updated do not return without unlocking mutexes >> * Lock mutex before unlock >> * Runtime tested ;-) >> > > [...] > >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> @@ -1340,8 +1340,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, >> if (type != IIO_EV_TYPE_THRESH) >> return -EINVAL; >> >> - if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) >> - return -EBUSY; >> + mutex_lock(&hw->conf_lock); >> + >> + if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) { >> + err = -EBUSY; >> + goto out; >> + } > > This patch is still broken!!! Returning in case of error you need to relase the > lock. > Hmm, please read below this: - return 0; +out: + mutex_unlock(&hw->conf_lock); + + return err; >> >> /* do not enable events if they are already enabled */ >> if (state && hw->enable_event) >> @@ -1357,7 +1361,10 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, >> >> hw->enable_event = state; >> >> - return 0; >> +out: >> + mutex_unlock(&hw->conf_lock); >> + >> + return err; >> } >> >> int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val) >> -- >> 2.23.0 >>
On 09/09/2019 13.59, Sean Nyekjaer wrote: > > > On 09/09/2019 13.55, Lorenzo Bianconi wrote: >>> When events and buffered reads is enabled simultaneously, and the first >>> event accours the interrupt pin stays high. >>> >>> This can be reverted when we find a solution to allow events and >>> buffered reads simultaneously. >>> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com> >>> --- >>> Changes since v4: >>> * Use fifo configuration mutex to prevent a race in >>> hw->enable_event check. >>> >>> Changes since v5: >>> * Updated do not return without unlocking mutexes >>> * Lock mutex before unlock >>> * Runtime tested ;-) >>> >> >> [...] >> >>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >>> @@ -1340,8 +1340,12 @@ static int >>> st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, >>> if (type != IIO_EV_TYPE_THRESH) >>> return -EINVAL; >>> - if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) >>> - return -EBUSY; >>> + mutex_lock(&hw->conf_lock); >>> + >>> + if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) { >>> + err = -EBUSY; >>> + goto out; >>> + } >> >> This patch is still broken!!! Returning in case of error you need to >> relase the >> lock. >> > > Hmm, > > please read below this: > > - return 0; > +out: > + mutex_unlock(&hw->conf_lock); > + > + return err; > > I see what you mean... I will update this function with: goto's :-) /Sean >>> /* do not enable events if they are already enabled */ >>> if (state && hw->enable_event) >>> @@ -1357,7 +1361,10 @@ static int >>> st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, >>> hw->enable_event = state; >>> - return 0; >>> +out: >>> + mutex_unlock(&hw->conf_lock); >>> + >>> + return err; >>> } >>> int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int >>> val) >>> -- >>> 2.23.0 >>>
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c index ef579650fd52..b87a1872bc60 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c @@ -603,6 +603,11 @@ int st_lsm6dsx_update_fifo(struct st_lsm6dsx_sensor *sensor, bool enable) mutex_lock(&hw->conf_lock); + if (hw->enable_event) { + err = -EBUSY; + goto out; + } + if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) { err = st_lsm6dsx_flush_fifo(hw); if (err < 0) diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index 00ba14d15c13..2616036ce953 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -1340,8 +1340,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, if (type != IIO_EV_TYPE_THRESH) return -EINVAL; - if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) - return -EBUSY; + mutex_lock(&hw->conf_lock); + + if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) { + err = -EBUSY; + goto out; + } /* do not enable events if they are already enabled */ if (state && hw->enable_event) @@ -1357,7 +1361,10 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, hw->enable_event = state; - return 0; +out: + mutex_unlock(&hw->conf_lock); + + return err; } int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
When events and buffered reads is enabled simultaneously, and the first event accours the interrupt pin stays high. This can be reverted when we find a solution to allow events and buffered reads simultaneously. Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- Changes since v4: * Use fifo configuration mutex to prevent a race in hw->enable_event check. Changes since v5: * Updated do not return without unlocking mutexes * Lock mutex before unlock * Runtime tested ;-) drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 5 +++++ drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 13 ++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-)