| Message ID | 20190621185124.28966-1-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] Input: edt-ft5x06 - disable irq handling during suspend | expand |
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 >
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.
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
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
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 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
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; }
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(-)