iio: cros_ec_accel_legacy: Always release lock when returning from _read()
diff mbox series

Message ID 20190715191017.98488-1-mka@chromium.org
State New
Headers show
Series
  • iio: cros_ec_accel_legacy: Always release lock when returning from _read()
Related show

Commit Message

Matthias Kaehlcke July 15, 2019, 7:10 p.m. UTC
Before doing any actual work cros_ec_accel_legacy_read() acquires
a mutex, which is released at the end of the function. However for
'calibbias' channels the function returns directly, without releasing
the lock. The next attempt to acquire the lock blocks forever. Instead
of an explicit return statement use the common return path, which
releases the lock.

Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Doug Anderson July 15, 2019, 7:40 p.m. UTC | #1
Hi,

On Mon, Jul 15, 2019 at 12:10 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Before doing any actual work cros_ec_accel_legacy_read() acquires
> a mutex, which is released at the end of the function. However for
> 'calibbias' channels the function returns directly, without releasing
> the lock. The next attempt to acquire the lock blocks forever. Instead
> of an explicit return statement use the common return path, which
> releases the lock.
>
> Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

See also <https://lkml.kernel.org/r/39403a4c-bf7f-6a98-890c-57397fa66493@collabora.com>

Actually, the "Fixes" tag is wrong here, though.  The problem only
exists because we have <https://crrev.com/c/1632659> in our tree, AKA
("FROMLIST: iio: cros_ec : Extend legacy support to ARM device").
Before that there was no mutex.  For upstream purposes this could
probably be squashed into the original patch.

-Doug
Matthias Kaehlcke July 15, 2019, 7:50 p.m. UTC | #2
On Mon, Jul 15, 2019 at 12:40:42PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jul 15, 2019 at 12:10 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Before doing any actual work cros_ec_accel_legacy_read() acquires
> > a mutex, which is released at the end of the function. However for
> > 'calibbias' channels the function returns directly, without releasing
> > the lock. The next attempt to acquire the lock blocks forever. Instead
> > of an explicit return statement use the common return path, which
> > releases the lock.
> >
> > Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> See also <https://lkml.kernel.org/r/39403a4c-bf7f-6a98-890c-57397fa66493@collabora.com>
> 
> Actually, the "Fixes" tag is wrong here, though.  The problem only
> exists because we have <https://crrev.com/c/1632659> in our tree, AKA
> ("FROMLIST: iio: cros_ec : Extend legacy support to ARM device").
> Before that there was no mutex.  For upstream purposes this could
> probably be squashed into the original patch.

Oops, I didn't realize that upstream doesn't have the mutex. In this
case the entire patch as is with it's commit message doesn't make much
sense.
Benson Leung July 15, 2019, 7:55 p.m. UTC | #3
Hi Matthias,

On Mon, Jul 15, 2019 at 12:10:17PM -0700, Matthias Kaehlcke wrote:
> Before doing any actual work cros_ec_accel_legacy_read() acquires
> a mutex, which is released at the end of the function. However for
> 'calibbias' channels the function returns directly, without releasing
> the lock. The next attempt to acquire the lock blocks forever. Instead
> of an explicit return statement use the common return path, which
> releases the lock.
> 
> Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> index 46bb2e421bb9..27ca4a64dddf 100644
> --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> @@ -206,7 +206,8 @@ static int cros_ec_accel_legacy_read(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		/* Calibration not supported. */
>  		*val = 0;
> -		return IIO_VAL_INT;
> +		ret = IIO_VAL_INT;
> +		break;

The value of ret is not used below this. It seems to be only used in
case IIO_CHAN_INFO_RAW. In fact, with your change,
there's no return value at all and we just reach the end of
cros_ec_accel_legacy_read.

>  	default:
>  		return -EINVAL;
>  	}
> -- 
> 2.22.0.510.g264f2c817a-goog
>
Matthias Kaehlcke July 15, 2019, 8:04 p.m. UTC | #4
Hi Benson,

On Mon, Jul 15, 2019 at 12:55:57PM -0700, Benson Leung wrote:
> Hi Matthias,
> 
> On Mon, Jul 15, 2019 at 12:10:17PM -0700, Matthias Kaehlcke wrote:
> > Before doing any actual work cros_ec_accel_legacy_read() acquires
> > a mutex, which is released at the end of the function. However for
> > 'calibbias' channels the function returns directly, without releasing
> > the lock. The next attempt to acquire the lock blocks forever. Instead
> > of an explicit return statement use the common return path, which
> > releases the lock.
> > 
> > Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> > index 46bb2e421bb9..27ca4a64dddf 100644
> > --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> > @@ -206,7 +206,8 @@ static int cros_ec_accel_legacy_read(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_CALIBBIAS:
> >  		/* Calibration not supported. */
> >  		*val = 0;
> > -		return IIO_VAL_INT;
> > +		ret = IIO_VAL_INT;
> > +		break;
> 
> The value of ret is not used below this. It seems to be only used in
> case IIO_CHAN_INFO_RAW. In fact, with your change,
> there's no return value at all and we just reach the end of
> cros_ec_accel_legacy_read.
> 
> >  	default:
> >  		return -EINVAL;
> >  	}
> 

I messed up. I was over-confident that a FROMLIST patch in our 4.19
kernel + this patch applying on upstream means that upstream uses the
same code. I should have double-checked that the upstream context is
actually the same.

Sorry for the noise.
Gwendal Grignou July 15, 2019, 11:17 p.m. UTC | #5
Sorry for the original mistake. I upload a patch at
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1702884.

On Mon, Jul 15, 2019 at 1:04 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Benson,
>
> On Mon, Jul 15, 2019 at 12:55:57PM -0700, Benson Leung wrote:
> > Hi Matthias,
> >
> > On Mon, Jul 15, 2019 at 12:10:17PM -0700, Matthias Kaehlcke wrote:
> > > Before doing any actual work cros_ec_accel_legacy_read() acquires
> > > a mutex, which is released at the end of the function. However for
> > > 'calibbias' channels the function returns directly, without releasing
> > > the lock. The next attempt to acquire the lock blocks forever. Instead
> > > of an explicit return statement use the common return path, which
> > > releases the lock.
> > >
> > > Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >  drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> > > index 46bb2e421bb9..27ca4a64dddf 100644
> > > --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> > > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> > > @@ -206,7 +206,8 @@ static int cros_ec_accel_legacy_read(struct iio_dev *indio_dev,
> > >     case IIO_CHAN_INFO_CALIBBIAS:
> > >             /* Calibration not supported. */
> > >             *val = 0;
> > > -           return IIO_VAL_INT;
> > > +           ret = IIO_VAL_INT;
> > > +           break;
> >
> > The value of ret is not used below this. It seems to be only used in
> > case IIO_CHAN_INFO_RAW. In fact, with your change,
> > there's no return value at all and we just reach the end of
> > cros_ec_accel_legacy_read.
> >
> > >     default:
> > >             return -EINVAL;
> > >     }
> >
>
> I messed up. I was over-confident that a FROMLIST patch in our 4.19
> kernel + this patch applying on upstream means that upstream uses the
> same code. I should have double-checked that the upstream context is
> actually the same.
>
> Sorry for the noise.
Gwendal Grignou July 15, 2019, 11:19 p.m. UTC | #6
Let's use Matthias CL.

On Mon, Jul 15, 2019 at 4:17 PM Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Sorry for the original mistake. I upload a patch at
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1702884.
>
> On Mon, Jul 15, 2019 at 1:04 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Benson,
> >
> > On Mon, Jul 15, 2019 at 12:55:57PM -0700, Benson Leung wrote:
> > > Hi Matthias,
> > >
> > > On Mon, Jul 15, 2019 at 12:10:17PM -0700, Matthias Kaehlcke wrote:
> > > > Before doing any actual work cros_ec_accel_legacy_read() acquires
> > > > a mutex, which is released at the end of the function. However for
> > > > 'calibbias' channels the function returns directly, without releasing
> > > > the lock. The next attempt to acquire the lock blocks forever. Instead
> > > > of an explicit return statement use the common return path, which
> > > > releases the lock.
> > > >
> > > > Fixes: 11b86c7004ef1 ("platform/chrome: Add cros_ec_accel_legacy driver")
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > >  drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> > > > index 46bb2e421bb9..27ca4a64dddf 100644
> > > > --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> > > > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> > > > @@ -206,7 +206,8 @@ static int cros_ec_accel_legacy_read(struct iio_dev *indio_dev,
> > > >     case IIO_CHAN_INFO_CALIBBIAS:
> > > >             /* Calibration not supported. */
> > > >             *val = 0;
> > > > -           return IIO_VAL_INT;
> > > > +           ret = IIO_VAL_INT;
> > > > +           break;
> > >
> > > The value of ret is not used below this. It seems to be only used in
> > > case IIO_CHAN_INFO_RAW. In fact, with your change,
> > > there's no return value at all and we just reach the end of
> > > cros_ec_accel_legacy_read.
> > >
> > > >     default:
> > > >             return -EINVAL;
> > > >     }
> > >
> >
> > I messed up. I was over-confident that a FROMLIST patch in our 4.19
> > kernel + this patch applying on upstream means that upstream uses the
> > same code. I should have double-checked that the upstream context is
> > actually the same.
> >
> > Sorry for the noise.

Patch
diff mbox series

diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
index 46bb2e421bb9..27ca4a64dddf 100644
--- a/drivers/iio/accel/cros_ec_accel_legacy.c
+++ b/drivers/iio/accel/cros_ec_accel_legacy.c
@@ -206,7 +206,8 @@  static int cros_ec_accel_legacy_read(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_CALIBBIAS:
 		/* Calibration not supported. */
 		*val = 0;
-		return IIO_VAL_INT;
+		ret = IIO_VAL_INT;
+		break;
 	default:
 		return -EINVAL;
 	}