2019-07-15 19:12:23

by Matthias Kaehlcke

[permalink] [raw]
Subject: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

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 <[email protected]>
---
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;
default:
return -EINVAL;
}
--
2.22.0.510.g264f2c817a-goog


2019-07-15 19:42:07

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

Hi,

On Mon, Jul 15, 2019 at 12:10 PM Matthias Kaehlcke <[email protected]> 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 <[email protected]>
> ---
> drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

See also <https://lkml.kernel.org/r/[email protected]>

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

2019-07-15 19:51:50

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

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 <[email protected]> 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 <[email protected]>
> > ---
> > drivers/iio/accel/cros_ec_accel_legacy.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
>
> See also <https://lkml.kernel.org/r/[email protected]>
>
> 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.

2019-07-15 19:57:19

by Benson Leung

[permalink] [raw]
Subject: Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

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 <[email protected]>
> ---
> 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
>

--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]


Attachments:
(No filename) (1.60 kB)
signature.asc (235.00 B)
Download all attachments

2019-07-15 20:05:26

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

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 <[email protected]>
> > ---
> > 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.

2019-07-15 23:18:40

by Gwendal Grignou

[permalink] [raw]
Subject: Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.

2019-07-15 23:22:19

by Gwendal Grignou

[permalink] [raw]
Subject: Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read()

Let's use Matthias CL.

On Mon, Jul 15, 2019 at 4:17 PM Gwendal Grignou <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.