| Message ID | 20200801135511.342869-1-angelo@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: mcp3422: fix locking scope | expand |
On Sat, 1 Aug 2020 15:55:11 +0200 Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote: > Locking should be held for the entire reading sequence involving setting > the channel, waiting for the channel switch and reading from the > channel. > If not, reading from a channel can result mixing with the reading from > another channel. > > Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> Looks like we should be backporting this. Fixes tag please. Jonathan > --- > drivers/iio/adc/mcp3422.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c > index d86c0b5d80a3..02a60fb164cd 100644 > --- a/drivers/iio/adc/mcp3422.c > +++ b/drivers/iio/adc/mcp3422.c > @@ -96,16 +96,12 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) > { > int ret; > > - mutex_lock(&adc->lock); > - > ret = i2c_master_send(adc->i2c, &newconfig, 1); > if (ret > 0) { > adc->config = newconfig; > ret = 0; > } > > - mutex_unlock(&adc->lock); > - > return ret; > } > > @@ -138,6 +134,8 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > u8 config; > u8 req_channel = channel->channel; > > + mutex_lock(&adc->lock); > + > if (req_channel != MCP3422_CHANNEL(adc->config)) { > config = adc->config; > config &= ~MCP3422_CHANNEL_MASK; > @@ -150,7 +148,11 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]); > } > > - return mcp3422_read(adc, value, &config); > + ret = mcp3422_read(adc, value, &config); > + > + mutex_unlock(&adc->lock); > + > + return ret; > } > > static int mcp3422_read_raw(struct iio_dev *iio,
Il giorno sab 1 ago 2020 alle ore 18:46 Jonathan Cameron <jic23@jic23.retrosnub.co.uk> ha scritto: > > On Sat, 1 Aug 2020 15:55:11 +0200 > Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote: > > > Locking should be held for the entire reading sequence involving setting > > the channel, waiting for the channel switch and reading from the > > channel. > > If not, reading from a channel can result mixing with the reading from > > another channel. > > > > Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> > > Looks like we should be backporting this. Fixes tag please. I don't know what it fixes, there is no signalled bug for this, I simply discovered it doing some testing. > > Jonathan > > > --- > > drivers/iio/adc/mcp3422.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c > > index d86c0b5d80a3..02a60fb164cd 100644 > > --- a/drivers/iio/adc/mcp3422.c > > +++ b/drivers/iio/adc/mcp3422.c > > @@ -96,16 +96,12 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) > > { > > int ret; > > > > - mutex_lock(&adc->lock); > > - > > ret = i2c_master_send(adc->i2c, &newconfig, 1); > > if (ret > 0) { > > adc->config = newconfig; > > ret = 0; > > } > > > > - mutex_unlock(&adc->lock); > > - > > return ret; > > } > > > > @@ -138,6 +134,8 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > > u8 config; > > u8 req_channel = channel->channel; > > > > + mutex_lock(&adc->lock); > > + > > if (req_channel != MCP3422_CHANNEL(adc->config)) { > > config = adc->config; > > config &= ~MCP3422_CHANNEL_MASK; > > @@ -150,7 +148,11 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > > msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]); > > } > > > > - return mcp3422_read(adc, value, &config); > > + ret = mcp3422_read(adc, value, &config); > > + > > + mutex_unlock(&adc->lock); > > + > > + return ret; > > } > > > > static int mcp3422_read_raw(struct iio_dev *iio, >
On Fri, 14 Aug 2020 08:24:46 +0200 Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote: > Il giorno sab 1 ago 2020 alle ore 18:46 Jonathan Cameron > <jic23@jic23.retrosnub.co.uk> ha scritto: > > > > On Sat, 1 Aug 2020 15:55:11 +0200 > > Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote: > > > > > Locking should be held for the entire reading sequence involving setting > > > the channel, waiting for the channel switch and reading from the > > > channel. > > > If not, reading from a channel can result mixing with the reading from > > > another channel. > > > > > > Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> > > > > Looks like we should be backporting this. Fixes tag please. > > I don't know what it fixes, there is no signalled bug for this, I > simply discovered it doing some testing. In this case I'm just looking for which patch originally introduced the bug. It might be the original driver introduction of course in which case give me a tag for that (look at SubmittingPatches.rst for info). That info is needed to let us figure out which stable kernels we should backport this to. Thanks Jonathan > > > > > Jonathan > > > > > --- > > > drivers/iio/adc/mcp3422.c | 12 +++++++----- > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c > > > index d86c0b5d80a3..02a60fb164cd 100644 > > > --- a/drivers/iio/adc/mcp3422.c > > > +++ b/drivers/iio/adc/mcp3422.c > > > @@ -96,16 +96,12 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) > > > { > > > int ret; > > > > > > - mutex_lock(&adc->lock); > > > - > > > ret = i2c_master_send(adc->i2c, &newconfig, 1); > > > if (ret > 0) { > > > adc->config = newconfig; > > > ret = 0; > > > } > > > > > > - mutex_unlock(&adc->lock); > > > - > > > return ret; > > > } > > > > > > @@ -138,6 +134,8 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > > > u8 config; > > > u8 req_channel = channel->channel; > > > > > > + mutex_lock(&adc->lock); > > > + > > > if (req_channel != MCP3422_CHANNEL(adc->config)) { > > > config = adc->config; > > > config &= ~MCP3422_CHANNEL_MASK; > > > @@ -150,7 +148,11 @@ static int mcp3422_read_channel(struct mcp3422 *adc, > > > msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]); > > > } > > > > > > - return mcp3422_read(adc, value, &config); > > > + ret = mcp3422_read(adc, value, &config); > > > + > > > + mutex_unlock(&adc->lock); > > > + > > > + return ret; > > > } > > > > > > static int mcp3422_read_raw(struct iio_dev *iio, > > > >
diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c index d86c0b5d80a3..02a60fb164cd 100644 --- a/drivers/iio/adc/mcp3422.c +++ b/drivers/iio/adc/mcp3422.c @@ -96,16 +96,12 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig) { int ret; - mutex_lock(&adc->lock); - ret = i2c_master_send(adc->i2c, &newconfig, 1); if (ret > 0) { adc->config = newconfig; ret = 0; } - mutex_unlock(&adc->lock); - return ret; } @@ -138,6 +134,8 @@ static int mcp3422_read_channel(struct mcp3422 *adc, u8 config; u8 req_channel = channel->channel; + mutex_lock(&adc->lock); + if (req_channel != MCP3422_CHANNEL(adc->config)) { config = adc->config; config &= ~MCP3422_CHANNEL_MASK; @@ -150,7 +148,11 @@ static int mcp3422_read_channel(struct mcp3422 *adc, msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]); } - return mcp3422_read(adc, value, &config); + ret = mcp3422_read(adc, value, &config); + + mutex_unlock(&adc->lock); + + return ret; } static int mcp3422_read_raw(struct iio_dev *iio,
Locking should be held for the entire reading sequence involving setting the channel, waiting for the channel switch and reading from the channel. If not, reading from a channel can result mixing with the reading from another channel. Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com> --- drivers/iio/adc/mcp3422.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)