| Message ID | 20201005052420.GA3262631@dtor-ws (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | iio: adc: exynos: do not rely on 'users' counter in ISR | expand |
On Sun, Oct 04, 2020 at 10:24:20PM -0700, dmitry.torokhov@gmail.com wrote: > The order in which 'users' counter is decremented vs calling drivers' > close() method is implementation specific, and we should not rely on > it. Let's introduce driver private flag and use it to signal ISR > to exit when device is being closed. > > This has a side-effect of fixing issue of accessing inut->users > outside of input->mutex protection. > > Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/iio/adc/exynos_adc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Acked-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On Sun, Oct 04, 2020 at 10:24:20PM -0700, dmitry.torokhov@gmail.com wrote: > The order in which 'users' counter is decremented vs calling drivers' > close() method is implementation specific, and we should not rely on > it. Let's introduce driver private flag and use it to signal ISR > to exit when device is being closed. > > This has a side-effect of fixing issue of accessing inut->users > outside of input->mutex protection. > > Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/iio/adc/exynos_adc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c > index 22131a677445..7eb2a5df6e98 100644 > --- a/drivers/iio/adc/exynos_adc.c > +++ b/drivers/iio/adc/exynos_adc.c > @@ -135,6 +135,8 @@ struct exynos_adc { > u32 value; > unsigned int version; > > + bool ts_enabled; > + > bool read_ts; > u32 ts_x; > u32 ts_y; > @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id) > bool pressed; > int ret; > > - while (info->input->users) { > + while (info->ts_enabled) { > ret = exynos_read_s3c64xx_ts(dev, &x, &y); > if (ret == -ETIMEDOUT) > break; > @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev) > { > struct exynos_adc *info = input_get_drvdata(dev); > > + info->ts_enabled = true; > + mb(); > enable_irq(info->tsirq); > > return 0; > @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev) > { > struct exynos_adc *info = input_get_drvdata(dev); > > + info->ts_enabled = false; > + mb(); > disable_irq(info->tsirq); This should be WRITE_ONCE paired with READ_ONCE in the ISR. But is the check really needed? I see that this is to break waiting for a touch release event, so I would assume this shouldn't wait forever (unless the hardware is buggy) and breaking the loop will desync touch state (I would guess this would be noticable by next user). Best Regards, Michał Mirosław
Hi Michał, On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote: > On Sun, Oct 04, 2020 at 10:24:20PM -0700, dmitry.torokhov@gmail.com wrote: > > The order in which 'users' counter is decremented vs calling drivers' > > close() method is implementation specific, and we should not rely on > > it. Let's introduce driver private flag and use it to signal ISR > > to exit when device is being closed. > > > > This has a side-effect of fixing issue of accessing inut->users > > outside of input->mutex protection. > > > > Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/iio/adc/exynos_adc.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c > > index 22131a677445..7eb2a5df6e98 100644 > > --- a/drivers/iio/adc/exynos_adc.c > > +++ b/drivers/iio/adc/exynos_adc.c > > @@ -135,6 +135,8 @@ struct exynos_adc { > > u32 value; > > unsigned int version; > > > > + bool ts_enabled; > > + > > bool read_ts; > > u32 ts_x; > > u32 ts_y; > > @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id) > > bool pressed; > > int ret; > > > > - while (info->input->users) { > > + while (info->ts_enabled) { > > ret = exynos_read_s3c64xx_ts(dev, &x, &y); > > if (ret == -ETIMEDOUT) > > break; > > @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev) > > { > > struct exynos_adc *info = input_get_drvdata(dev); > > > > + info->ts_enabled = true; > > + mb(); > > enable_irq(info->tsirq); > > > > return 0; > > @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev) > > { > > struct exynos_adc *info = input_get_drvdata(dev); > > > > + info->ts_enabled = false; > > + mb(); > > disable_irq(info->tsirq); > > This should be WRITE_ONCE paired with READ_ONCE in the ISR. Why? I can see that maybe full memory barrier is too heavy when we set the flag to true, but the only requirement is for the flag to be set before we disable IRQ, so any additional guarantees provided by WRITE_ONCE are not needed. On the read side we want the flag to be noticed eventually, and there is no additional dependencies on the data, so it is unclear what READ_ONCE() will give us here. > > But is the check really needed? I see that this is to break waiting for > a touch release event, so I would assume this shouldn't wait forever > (unless the hardware is buggy) It is not hardware, it is user. Do you want to delay indefinitely close() just because user has a finger on the touchscreen? > and breaking the loop will desync touch > state (I would guess this would be noticable by next user). Upon next open driver will service the interrupt and provide new set of touch coordinates. Userspace is supposed to query current state of device when opening it before starting processing events. Or you are concerned about some other state? In any case, this is current driver behavior and if it needs to be changed it is a topic for a separate patch. Thanks.
On Mon, Oct 05, 2020 at 10:36:36AM -0700, dmitry.torokhov@gmail.com wrote: > Hi Michał, > > On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote: > > On Sun, Oct 04, 2020 at 10:24:20PM -0700, dmitry.torokhov@gmail.com wrote: > > > The order in which 'users' counter is decremented vs calling drivers' > > > close() method is implementation specific, and we should not rely on > > > it. Let's introduce driver private flag and use it to signal ISR > > > to exit when device is being closed. > > > > > > This has a side-effect of fixing issue of accessing inut->users > > > outside of input->mutex protection. > > > > > > Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/iio/adc/exynos_adc.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c > > > index 22131a677445..7eb2a5df6e98 100644 > > > --- a/drivers/iio/adc/exynos_adc.c > > > +++ b/drivers/iio/adc/exynos_adc.c > > > @@ -135,6 +135,8 @@ struct exynos_adc { > > > u32 value; > > > unsigned int version; > > > > > > + bool ts_enabled; > > > + > > > bool read_ts; > > > u32 ts_x; > > > u32 ts_y; > > > @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id) > > > bool pressed; > > > int ret; > > > > > > - while (info->input->users) { > > > + while (info->ts_enabled) { > > > ret = exynos_read_s3c64xx_ts(dev, &x, &y); > > > if (ret == -ETIMEDOUT) > > > break; > > > @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev) > > > { > > > struct exynos_adc *info = input_get_drvdata(dev); > > > > > > + info->ts_enabled = true; > > > + mb(); > > > enable_irq(info->tsirq); > > > > > > return 0; > > > @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev) > > > { > > > struct exynos_adc *info = input_get_drvdata(dev); > > > > > > + info->ts_enabled = false; > > > + mb(); > > > disable_irq(info->tsirq); > > > > This should be WRITE_ONCE paired with READ_ONCE in the ISR. > > Why? I can see that maybe full memory barrier is too heavy when we set > the flag to true, but the only requirement is for the flag to be set > before we disable IRQ, so any additional guarantees provided by > WRITE_ONCE are not needed. On the read side we want the flag to be > noticed eventually, and there is no additional dependencies on the data, > so it is unclear what READ_ONCE() will give us here. Without READ_ONCE you have no guarantee that the compiler won't optimize 'while (flag) ...' to 'if (flag) for(;;) ...'. Maybe the platform has stronger coherency guarantees than Linux memory model assumes, but if not, a CPU running the ISR (without paired memory barrier) might not ever see the store from another CPU (both in current and proposed code). > > But is the check really needed? I see that this is to break waiting for > > a touch release event, so I would assume this shouldn't wait forever > > (unless the hardware is buggy) > > It is not hardware, it is user. Do you want to delay indefinitely > close() just because user has a finger on the touchscreen? Ack. This covers the why of loop breaking. > > and breaking the loop will desync touch > > state (I would guess this would be noticable by next user). > Upon next open driver will service the interrupt and provide new set of > touch coordinates. Userspace is supposed to query current state of > device when opening it before starting processing events. Or you are > concerned about some other state? From the code I would expect that there is a slight window, wher when the user releases the touch between close() and open(), the client that open()s will see a 'pressed' state until the ISR runs again (probably immediately because of pending interrupt). OTOH, maybe the app should be prepared for that anyway? > In any case, this is current driver behavior and if it needs to be > changed it is a topic for a separate patch. Agreed. > > Thanks. > > -- > Dmitry
On Mon, Oct 05, 2020 at 09:00:10PM +0200, Michał Mirosław wrote: > On Mon, Oct 05, 2020 at 10:36:36AM -0700, dmitry.torokhov@gmail.com wrote: > > Hi Michał, > > > > On Mon, Oct 05, 2020 at 01:09:08PM +0200, Michał Mirosław wrote: > > > and breaking the loop will desync touch > > > state (I would guess this would be noticable by next user). > > Upon next open driver will service the interrupt and provide new set of > > touch coordinates. Userspace is supposed to query current state of > > device when opening it before starting processing events. Or you are > > concerned about some other state? > > From the code I would expect that there is a slight window, wher when the > user releases the touch between close() and open(), the client that open()s > will see a 'pressed' state until the ISR runs again (probably immediately > because of pending interrupt). OTOH, maybe the app should be prepared > for that anyway? I think users should be prepared for it. There could be many users, so anyone opening interface device (evdev or similar) can not expect that the hardware device is in a quiesce state; it could just happen that finger was on the surface and will be released as someone is opening the device anyway. Thanks.
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c index 22131a677445..7eb2a5df6e98 100644 --- a/drivers/iio/adc/exynos_adc.c +++ b/drivers/iio/adc/exynos_adc.c @@ -135,6 +135,8 @@ struct exynos_adc { u32 value; unsigned int version; + bool ts_enabled; + bool read_ts; u32 ts_x; u32 ts_y; @@ -633,7 +635,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id) bool pressed; int ret; - while (info->input->users) { + while (info->ts_enabled) { ret = exynos_read_s3c64xx_ts(dev, &x, &y); if (ret == -ETIMEDOUT) break; @@ -712,6 +714,8 @@ static int exynos_adc_ts_open(struct input_dev *dev) { struct exynos_adc *info = input_get_drvdata(dev); + info->ts_enabled = true; + mb(); enable_irq(info->tsirq); return 0; @@ -721,6 +725,8 @@ static void exynos_adc_ts_close(struct input_dev *dev) { struct exynos_adc *info = input_get_drvdata(dev); + info->ts_enabled = false; + mb(); disable_irq(info->tsirq); }
The order in which 'users' counter is decremented vs calling drivers' close() method is implementation specific, and we should not rely on it. Let's introduce driver private flag and use it to signal ISR to exit when device is being closed. This has a side-effect of fixing issue of accessing inut->users outside of input->mutex protection. Reported-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/iio/adc/exynos_adc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)