On Sun, Mar 15, 2020 at 09:46:04AM +0000, Jonathan Cameron wrote: > On Tue, 10 Mar 2020 00:01:28 +0530 > Rohit Sarkar <rohitsarkar5398@xxxxxxxxx> wrote: > > > Use local lock instead of indio_dev's mlock. > > The mlock was being used to protect local driver state thus using the > > local lock is a better option here. > > > > Signed-off-by: Rohit Sarkar <rohitsarkar5398@xxxxxxxxx> > > Matt. Definitely need your input on this. > > > --- > > drivers/iio/health/max30100.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c > > index 84010501762d..8ddc4649547d 100644 > > --- a/drivers/iio/health/max30100.c > > +++ b/drivers/iio/health/max30100.c > > @@ -388,7 +388,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev, > > * Temperature reading can only be acquired while engine > > * is running > > */ > > - mutex_lock(&indio_dev->mlock); > > + mutex_lock(&data->lock); > > Hmm.. It's another complex one. What is actually being protected here is > the buffer state, but not to take it exclusively like claim_direct does. So just to check if I understand correctly, let's say we did not use any lock in this case. If an execution thread reached the iio_buffer_enabled() check and found the buffer to be enabled, but the buffer got disabled simultaneously, the temperature readings that we get will be corrupted. Does this make sense? > Here we need the inverse, we want to ensure we are 'not' in the direct > mode because this hardware requires the buffer to be running to read the > temperature. > > That is the sort of interface that is going to get userspace very > confused. Agreed > Matt, normally what I'd suggest here is that the temperature read should: > > 1) Claim direct mode, if it fails then do the dance you have here > (with more comments to explain why you are taking an internal lock) > 2) Start up capture as if we were in buffered mode > 3) Grab that temp > 4) stop capture to return to non buffered mode. > 5) Release direct mode. > > I guess we decided it wasn't worth the hassle. > > So Rohit. This one probably needs a comment rather than any change. The code already mentions that the "Temperature can only be acquired while engine is running.", should I add something like "mlock is acquired to protect the buffer state..." to the same comment. > We 'could' add a 'hold_buffered_mode' function that takes the mlock, > verifies we are in buffered mode and continues to hold the lock > until the 'release_buffered_mode'. However, I'm not sure any other > drivers do this particular dance, so clear commenting in the driver > might be enough. Should we ever change how mlock is used in the > core, we'd have to fix this driver up as well. Understood. > Hmm. This is really hammering home that perhaps all the remaining > mlock cases are 'hard'. A nice sideffect of me investigating these is that I am getting some good insight into how iio works. I will see if I can investigate a couple more cases > Thanks, > > Jonathan > > > Thanks, Rohit > > if (!iio_buffer_enabled(indio_dev)) > > ret = -EAGAIN; > > @@ -399,7 +399,7 @@ static int max30100_read_raw(struct iio_dev *indio_dev, > > > > } > > > > - mutex_unlock(&indio_dev->mlock); > > + mutex_unlock(&data->lock); > > break; > > case IIO_CHAN_INFO_SCALE: > > *val = 1; /* 0.0625 */ >
