* [PATCH] iio: adc: mcp3422: fix locking scope
@ 2020-08-01 13:55 Angelo Compagnucci
2020-08-01 16:46 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Angelo Compagnucci @ 2020-08-01 13:55 UTC (permalink / raw)
To: linux-iio; +Cc: Angelo Compagnucci, Angelo Compagnucci
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(-)
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,
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] iio: adc: mcp3422: fix locking scope
2020-08-01 13:55 [PATCH] iio: adc: mcp3422: fix locking scope Angelo Compagnucci
@ 2020-08-01 16:46 ` Jonathan Cameron
2020-08-14 6:24 ` Angelo Compagnucci
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2020-08-01 16:46 UTC (permalink / raw)
To: Angelo Compagnucci; +Cc: linux-iio, Angelo Compagnucci
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,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] iio: adc: mcp3422: fix locking scope
2020-08-01 16:46 ` Jonathan Cameron
@ 2020-08-14 6:24 ` Angelo Compagnucci
2020-08-16 9:14 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Angelo Compagnucci @ 2020-08-14 6:24 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Angelo Compagnucci
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,
>
--
Profile: http://it.linkedin.com/in/compagnucciangelo
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] iio: adc: mcp3422: fix locking scope
2020-08-14 6:24 ` Angelo Compagnucci
@ 2020-08-16 9:14 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2020-08-16 9:14 UTC (permalink / raw)
To: Angelo Compagnucci; +Cc: linux-iio, Angelo Compagnucci
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,
> >
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-16 9:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-01 13:55 [PATCH] iio: adc: mcp3422: fix locking scope Angelo Compagnucci
2020-08-01 16:46 ` Jonathan Cameron
2020-08-14 6:24 ` Angelo Compagnucci
2020-08-16 9:14 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).