[Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Benoit Parrot]

As a wakeup source when the system is in suspend there is little point
trying to access a register across the i2c bus as it is probably still
inactive. We need to prevent the irq handler from being called during
suspend.

Without this modification upon wakeup you would see the following kernel
error:

[ 118.733717] PM: Wakeup source GPIO0
[ 118.751933] edt_ft5x06 1-0038: Unable to fetch data, error: -13

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index c639ebce914c..c885bfe783a4 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1200,8 +1200,10 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
-	if (device_may_wakeup(dev))
+	if (device_may_wakeup(dev)) {
 		enable_irq_wake(client->irq);
+		disable_irq(client->irq);
+	}
 
 	return 0;
 }
@@ -1210,8 +1212,10 @@ static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
-	if (device_may_wakeup(dev))
+	if (device_may_wakeup(dev)) {
 		disable_irq_wake(client->irq);
+		enable_irq(client->irq);
+	}
 
 	return 0;
 }
-- 
2.17.1

Re: [Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Andy Shevchenko]

On Fri, Jun 21, 2019 at 9:53 PM Benoit Parrot <bparrot@ti.com> wrote:
>
> As a wakeup source when the system is in suspend there is little point
> trying to access a register across the i2c bus as it is probably still
> inactive. We need to prevent the irq handler from being called during
> suspend.
>

Hmm... But how OS will know what the event to handle afterwards?
I mean shouldn't we guarantee somehow the delivery of the event to the
input, in this case, subsystem followed by corresponding user space?

> Without this modification upon wakeup you would see the following kernel
> error:
>
> [ 118.733717] PM: Wakeup source GPIO0
> [ 118.751933] edt_ft5x06 1-0038: Unable to fetch data, error: -13
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index c639ebce914c..c885bfe783a4 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1200,8 +1200,10 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>
> -       if (device_may_wakeup(dev))
> +       if (device_may_wakeup(dev)) {
>                 enable_irq_wake(client->irq);
> +               disable_irq(client->irq);
> +       }
>
>         return 0;
>  }
> @@ -1210,8 +1212,10 @@ static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>
> -       if (device_may_wakeup(dev))
> +       if (device_may_wakeup(dev)) {
>                 disable_irq_wake(client->irq);
> +               enable_irq(client->irq);
> +       }
>
>         return 0;
>  }
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Dmitry Torokhov]

On Sat, Jun 22, 2019 at 01:37:10PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 21, 2019 at 9:53 PM Benoit Parrot <bparrot@ti.com> wrote:
> >
> > As a wakeup source when the system is in suspend there is little point
> > trying to access a register across the i2c bus as it is probably still
> > inactive. We need to prevent the irq handler from being called during
> > suspend.
> >
> 
> Hmm... But how OS will know what the event to handle afterwards?
> I mean shouldn't we guarantee somehow the delivery of the event to the
> input, in this case, subsystem followed by corresponding user space?

If we are using level interrupts then it will work OK, however it is
really easy to lose edge here, as replaying disabled edge triggered
interrupts is not really reliable.

Benoit, what kind of interrupt do you use in your system?

Thanks.

-- 
Dmitry

Re: [Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Benoit Parrot]

Andy Shevchenko <andy.shevchenko@gmail.com> wrote on Sat [2019-Jun-22 13:37:10 +0300]:
> On Fri, Jun 21, 2019 at 9:53 PM Benoit Parrot <bparrot@ti.com> wrote:
> >
> > As a wakeup source when the system is in suspend there is little point
> > trying to access a register across the i2c bus as it is probably still
> > inactive. We need to prevent the irq handler from being called during
> > suspend.
> >
> 
> Hmm... But how OS will know what the event to handle afterwards?
> I mean shouldn't we guarantee somehow the delivery of the event to the
> input, in this case, subsystem followed by corresponding user space?

I am not sure I understand, do you mean that you want the input wake up event
being processed by the edt_ft50x6 driver as it happens? How can we do that
if we can't access the device through the bus? Are we trying to capture
specific gesture here (given that the display should be off during
sleep/suspend)?

Anyhow here I am just trying to eliminate the runtime error caused by
trying to access a resource which we know is unavailable at the time.
What method would you suggest we use to achieve this? 

Benoit

> 
> > Without this modification upon wakeup you would see the following kernel
> > error:
> >
> > [ 118.733717] PM: Wakeup source GPIO0
> > [ 118.751933] edt_ft5x06 1-0038: Unable to fetch data, error: -13
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  drivers/input/touchscreen/edt-ft5x06.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index c639ebce914c..c885bfe783a4 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -1200,8 +1200,10 @@ static int __maybe_unused edt_ft5x06_ts_suspend(struct device *dev)
> >  {
> >         struct i2c_client *client = to_i2c_client(dev);
> >
> > -       if (device_may_wakeup(dev))
> > +       if (device_may_wakeup(dev)) {
> >                 enable_irq_wake(client->irq);
> > +               disable_irq(client->irq);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -1210,8 +1212,10 @@ static int __maybe_unused edt_ft5x06_ts_resume(struct device *dev)
> >  {
> >         struct i2c_client *client = to_i2c_client(dev);
> >
> > -       if (device_may_wakeup(dev))
> > +       if (device_may_wakeup(dev)) {
> >                 disable_irq_wake(client->irq);
> > +               enable_irq(client->irq);
> > +       }
> >
> >         return 0;
> >  }
> > --
> > 2.17.1
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Benoit Parrot]

Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote on Sat [2019-Jun-22 22:59:40 -0700]:
> On Sat, Jun 22, 2019 at 01:37:10PM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 21, 2019 at 9:53 PM Benoit Parrot <bparrot@ti.com> wrote:
> > >
> > > As a wakeup source when the system is in suspend there is little point
> > > trying to access a register across the i2c bus as it is probably still
> > > inactive. We need to prevent the irq handler from being called during
> > > suspend.
> > >
> > 
> > Hmm... But how OS will know what the event to handle afterwards?
> > I mean shouldn't we guarantee somehow the delivery of the event to the
> > input, in this case, subsystem followed by corresponding user space?
> 
> If we are using level interrupts then it will work OK, however it is
> really easy to lose edge here, as replaying disabled edge triggered
> interrupts is not really reliable.
> 
> Benoit, what kind of interrupt do you use in your system?

Dmitry,

On our systems we currently used edge trigger. One example is available in
mainline: arch/arm/boot/dts/am437x-sk-evm.dts
632:              interrupts = <31 IRQ_TYPE_EDGE_FALLING>;


Benoit

> 
> Thanks.
> 
> -- 
> Dmitry

Re: [Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Dmitry Torokhov]

On Mon, Jun 24, 2019 at 07:24:57AM -0500, Benoit Parrot wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote on Sat [2019-Jun-22 22:59:40 -0700]:
> > On Sat, Jun 22, 2019 at 01:37:10PM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 21, 2019 at 9:53 PM Benoit Parrot <bparrot@ti.com> wrote:
> > > >
> > > > As a wakeup source when the system is in suspend there is little point
> > > > trying to access a register across the i2c bus as it is probably still
> > > > inactive. We need to prevent the irq handler from being called during
> > > > suspend.
> > > >
> > > 
> > > Hmm... But how OS will know what the event to handle afterwards?
> > > I mean shouldn't we guarantee somehow the delivery of the event to the
> > > input, in this case, subsystem followed by corresponding user space?
> > 
> > If we are using level interrupts then it will work OK, however it is
> > really easy to lose edge here, as replaying disabled edge triggered
> > interrupts is not really reliable.
> > 
> > Benoit, what kind of interrupt do you use in your system?
> 
> Dmitry,
> 
> On our systems we currently used edge trigger. One example is available in
> mainline: arch/arm/boot/dts/am437x-sk-evm.dts
> 632:              interrupts = <31 IRQ_TYPE_EDGE_FALLING>;

Does your device still work if you switch to level-triggered interrupt?

Regarding your patch I am uncomfortable with disabling interrupts if
interrupt is edge-triggered, as replaying edge interrupts after enabling
is not very reliable. So we should either only disable interrupt if it
is level-triggered, or make sure we read and process data from the
device after re-enabling interrupt to rearm it. We'll need to make sure
suspend does not race with interrupt handler than and also make sure we
handle case when device does not actually has data to report.

Thanks.

-- 
Dmitry

Re: [Patch 1/1] Input: edt-ft5x06 - disable irq handling during suspend

[Benoit Parrot]

Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote on Mon [2019-Jul-01 00:32:33 -0700]:
> On Mon, Jun 24, 2019 at 07:24:57AM -0500, Benoit Parrot wrote:
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote on Sat [2019-Jun-22 22:59:40 -0700]:
> > > On Sat, Jun 22, 2019 at 01:37:10PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Jun 21, 2019 at 9:53 PM Benoit Parrot <bparrot@ti.com> wrote:
> > > > >
> > > > > As a wakeup source when the system is in suspend there is little point
> > > > > trying to access a register across the i2c bus as it is probably still
> > > > > inactive. We need to prevent the irq handler from being called during
> > > > > suspend.
> > > > >
> > > > 
> > > > Hmm... But how OS will know what the event to handle afterwards?
> > > > I mean shouldn't we guarantee somehow the delivery of the event to the
> > > > input, in this case, subsystem followed by corresponding user space?
> > > 
> > > If we are using level interrupts then it will work OK, however it is
> > > really easy to lose edge here, as replaying disabled edge triggered
> > > interrupts is not really reliable.
> > > 
> > > Benoit, what kind of interrupt do you use in your system?
> > 
> > Dmitry,
> > 
> > On our systems we currently used edge trigger. One example is available in
> > mainline: arch/arm/boot/dts/am437x-sk-evm.dts
> > 632:              interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> 
> Does your device still work if you switch to level-triggered interrupt?

That would depend on the device. But for instance on am437x, in order for
GPIO IRQ to be detected as a wake up event they need to be edge triggered.

> 
> Regarding your patch I am uncomfortable with disabling interrupts if
> interrupt is edge-triggered, as replaying edge interrupts after enabling
> is not very reliable. So we should either only disable interrupt if it
> is level-triggered, or make sure we read and process data from the
> device after re-enabling interrupt to rearm it. We'll need to make sure
> suspend does not race with interrupt handler than and also make sure we
> handle case when device does not actually has data to report.

I am still not sure who would consume these events. Upon waking up from
suspend it would take a while for user-space to be ready to consume these
events, and by that time there may have been quite a few of them.

We are currently missing those events anyways, no?
I mean the i2c read operation during suspend is failing anyways, which
means that particular event is already missed.

Regards,
Benoit

> 
> Thanks.
> 
> -- 
> Dmitry