Re: [PATCH] iio: adc: exynos: do not rely on 'users' counter in ISR

["Michał Mirosław" <mirq-linux@rere.qmqm.pl>]

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