* [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() @ 2019-07-15 19:10 Matthias Kaehlcke 2019-07-15 19:40 ` Doug Anderson 2019-07-15 19:55 ` Benson Leung 0 siblings, 2 replies; 7+ messages in thread From: Matthias Kaehlcke @ 2019-07-15 19:10 UTC (permalink / raw) To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck Cc: linux-iio, linux-kernel, Gwendal Grignou, Douglas Anderson, Matthias Kaehlcke 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; default: return -EINVAL; } -- 2.22.0.510.g264f2c817a-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() 2019-07-15 19:10 [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() Matthias Kaehlcke @ 2019-07-15 19:40 ` Doug Anderson 2019-07-15 19:50 ` Matthias Kaehlcke 2019-07-15 19:55 ` Benson Leung 1 sibling, 1 reply; 7+ messages in thread From: Doug Anderson @ 2019-07-15 19:40 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, linux-iio, LKML, Gwendal Grignou 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() 2019-07-15 19:40 ` Doug Anderson @ 2019-07-15 19:50 ` Matthias Kaehlcke 0 siblings, 0 replies; 7+ messages in thread From: Matthias Kaehlcke @ 2019-07-15 19:50 UTC (permalink / raw) To: Doug Anderson Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, linux-iio, LKML, Gwendal Grignou 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() 2019-07-15 19:10 [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() Matthias Kaehlcke 2019-07-15 19:40 ` Doug Anderson @ 2019-07-15 19:55 ` Benson Leung 2019-07-15 20:04 ` Matthias Kaehlcke 1 sibling, 1 reply; 7+ messages in thread From: Benson Leung @ 2019-07-15 19:55 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, linux-iio, linux-kernel, Gwendal Grignou, Douglas Anderson [-- Attachment #1: Type: text/plain, Size: 1634 bytes --] 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 > -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() 2019-07-15 19:55 ` Benson Leung @ 2019-07-15 20:04 ` Matthias Kaehlcke 2019-07-15 23:17 ` Gwendal Grignou 0 siblings, 1 reply; 7+ messages in thread From: Matthias Kaehlcke @ 2019-07-15 20:04 UTC (permalink / raw) To: Benson Leung Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, linux-iio, linux-kernel, Gwendal Grignou, Douglas Anderson 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() 2019-07-15 20:04 ` Matthias Kaehlcke @ 2019-07-15 23:17 ` Gwendal Grignou 2019-07-15 23:19 ` Gwendal Grignou 0 siblings, 1 reply; 7+ messages in thread From: Gwendal Grignou @ 2019-07-15 23:17 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Benson Leung, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, linux-iio, linux-kernel, Douglas Anderson 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() 2019-07-15 23:17 ` Gwendal Grignou @ 2019-07-15 23:19 ` Gwendal Grignou 0 siblings, 0 replies; 7+ messages in thread From: Gwendal Grignou @ 2019-07-15 23:19 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Benson Leung, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, linux-iio, linux-kernel, Douglas Anderson 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-15 23:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-15 19:10 [PATCH] iio: cros_ec_accel_legacy: Always release lock when returning from _read() Matthias Kaehlcke 2019-07-15 19:40 ` Doug Anderson 2019-07-15 19:50 ` Matthias Kaehlcke 2019-07-15 19:55 ` Benson Leung 2019-07-15 20:04 ` Matthias Kaehlcke 2019-07-15 23:17 ` Gwendal Grignou 2019-07-15 23:19 ` Gwendal Grignou
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).