On Mon, Oct 05, 2020 at 10:36:36AM -0700, dmitry.torokhov@xxxxxxxxx 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@xxxxxxxxx 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@xxxxxxxxxxxxx>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > ---
> > > 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
