On Fri, Mar 7, 2025 at 3:40 PM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote: > > > On Thu, Mar 6, 2025 at 4:48 PM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote: > > > > > > > Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions > > > > from falling in an infinite loop in case pattern_len is equal to zero and > > > > the device FIFO is not empty. > > > > > > > > Signed-off-by: Silvano Seva <s.seva@xxxxxxxxx> > > > > --- > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > index 0a7cd8c1aa33..7f343614f8a5 100644 > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > @@ -395,12 +395,17 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) * > > > > ST_LSM6DSX_CHAN_SIZE; > > > > fifo_len = (fifo_len / pattern_len) * pattern_len; > > > > + if (!fifo_len) > > > > + return 0; > > > > > > I do not think this check is necessary since if fifo_len is 0 we will not run > > > the for loop, right? > > > > This check is present in the st_lsm6dsx_read_tagged_fifo() function, i > > added it here for consistency. I agree with you that is not strictly > > necessary. > > > > > > > > > > > > > acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]); > > > > gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]); > > > > if (hw->iio_devs[ST_LSM6DSX_ID_EXT0]) > > > > ext_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_EXT0]); > > > > > > > > + if (!pattern_len) > > > > + pattern_len = ST_LSM6DSX_SAMPLE_SIZE; > > > > > > same here, I do not think pattern_len can be 0 since hw->sip must be greater > > > than 0 in order to enable the FIFO. Moreover, this check should be some lines > > > above since we have already divided fifo_len by pattern_len here. > > > > > > > There is a situation causing the subsequent for loop to never > > terminate, hanging the kernel boot process: given a system which > > doesn't have an hardware reset line allowing the kernel to > > re-initialize the IMU hardware, in case of an hot reboot the driver > > probe() function attempts to flush the FIFO, which may contain some > > data, while the hw->sip is zero. > > The complete execution path is the following: > > - call of st_lsm6dsx_probe(); > > - allocation of the st_lsm6dsx_hw structure via the devm_kzalloc, > > zero-initializing the sip field; > > - call of st_lsm6dsx_init_device(); > > - call of st_lsm6dsx_reset_device(); > > - call of st_lsm6dsx_flush_fifo(); > > - call of st_lsm6dsx_read_fifo/st_lsm6dsx_read_tagged_fifo via the > > fifo_ops function pointer. > > ack, I can see the issue now, thx for pointing this out. I should we should set > pattern_len to ST_LSM6DSX_SAMPLE_SIZE or ST_LSM6DSX_TAGGED_SAMPLE_SIZE if it is > 0. Can you please move the check before updating fifo_len in > st_lsm6dsx_read_fifo()? > > Regards, > Lorenzo > Changes done, I sent the updated patch in a separate reply email: I hope that doing so didn't cause any trouble. Thank you, Silvano.
