| Message ID | 20190715191017.98488-1-mka@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: cros_ec_accel_legacy: Always release lock when returning from _read() | expand |
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
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.
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 >
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.
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.
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.
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; }
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(-)