* [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume @ 2021-12-22 22:06 Hans de Goede 2022-01-04 0:29 ` Dmitry Torokhov 0 siblings, 1 reply; 4+ messages in thread From: Hans de Goede @ 2021-12-22 22:06 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Hans de Goede, Jingle Wu, linux-input Before these changes elan_suspend() would only call elan_disable_power() when device_may_wakeup() returns false; whereas elan_resume() would unconditionally call elan_enable_power(), leading to an enable count imbalance when device_may_wakeup() returns true. This triggers the "WARN_ON(regulator->enable_count)" in regulator_put() when the elan_i2c driver gets unbound, this happens e.g. with the hot-plugable dock with Elan I2C touchpad for the Asus TF103C 2-in-1. Fix this by making the elan_enable_power() call also be conditional on device_may_wakeup() returning false. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/input/mouse/elan_i2c_core.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c index 47af62c12267..cdb36d35ffa4 100644 --- a/drivers/input/mouse/elan_i2c_core.c +++ b/drivers/input/mouse/elan_i2c_core.c @@ -1412,17 +1412,17 @@ static int __maybe_unused elan_resume(struct device *dev) struct elan_tp_data *data = i2c_get_clientdata(client); int error; - if (device_may_wakeup(dev) && data->irq_wake) { + if (!device_may_wakeup(dev)) { + error = elan_enable_power(data); + if (error) { + dev_err(dev, "power up when resuming failed: %d\n", error); + goto err; + } + } else if (data->irq_wake) { disable_irq_wake(client->irq); data->irq_wake = false; } - error = elan_enable_power(data); - if (error) { - dev_err(dev, "power up when resuming failed: %d\n", error); - goto err; - } - error = elan_initialize(data, data->quirks & ETP_QUIRK_QUICK_WAKEUP); if (error) dev_err(dev, "initialize when resuming failed: %d\n", error); -- 2.33.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume 2021-12-22 22:06 [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume Hans de Goede @ 2022-01-04 0:29 ` Dmitry Torokhov 2022-01-06 6:50 ` Jingle.Wu 0 siblings, 1 reply; 4+ messages in thread From: Dmitry Torokhov @ 2022-01-04 0:29 UTC (permalink / raw) To: Hans de Goede; +Cc: Jingle Wu, linux-input Hi Hans, On Wed, Dec 22, 2021 at 11:06:41PM +0100, Hans de Goede wrote: > Before these changes elan_suspend() would only call elan_disable_power() > when device_may_wakeup() returns false; whereas elan_resume() would > unconditionally call elan_enable_power(), leading to an enable count > imbalance when device_may_wakeup() returns true. > > This triggers the "WARN_ON(regulator->enable_count)" in regulator_put() > when the elan_i2c driver gets unbound, this happens e.g. with the > hot-plugable dock with Elan I2C touchpad for the Asus TF103C 2-in-1. > > Fix this by making the elan_enable_power() call also be conditional > on device_may_wakeup() returning false. elan_enable_power() not only tries to toggle the regulator, but also tries to issue command to the controller to power/wake it up. I need confirmation from Jingle Wu that skipping this is OK for all Elan touchpad controllers, or if we need to add call to either power control or sleep control method to make sure the controller it fully resumed. Jingle, what can you tell us here? > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/input/mouse/elan_i2c_core.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c > index 47af62c12267..cdb36d35ffa4 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -1412,17 +1412,17 @@ static int __maybe_unused elan_resume(struct device *dev) > struct elan_tp_data *data = i2c_get_clientdata(client); > int error; > > - if (device_may_wakeup(dev) && data->irq_wake) { > + if (!device_may_wakeup(dev)) { > + error = elan_enable_power(data); > + if (error) { > + dev_err(dev, "power up when resuming failed: %d\n", error); > + goto err; > + } > + } else if (data->irq_wake) { > disable_irq_wake(client->irq); > data->irq_wake = false; > } > > - error = elan_enable_power(data); > - if (error) { > - dev_err(dev, "power up when resuming failed: %d\n", error); > - goto err; > - } > - > error = elan_initialize(data, data->quirks & ETP_QUIRK_QUICK_WAKEUP); > if (error) > dev_err(dev, "initialize when resuming failed: %d\n", error); > -- > 2.33.1 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume 2022-01-04 0:29 ` Dmitry Torokhov @ 2022-01-06 6:50 ` Jingle.Wu 2022-01-06 9:22 ` Hans de Goede 0 siblings, 1 reply; 4+ messages in thread From: Jingle.Wu @ 2022-01-06 6:50 UTC (permalink / raw) To: 'Dmitry Torokhov', 'Hans de Goede'; +Cc: linux-input Hi Hans/Dmitry: It is not OK to skip elan_enable_power() for all Elan touchpad controllers. I suggest to deal with this touchpad module ID as "quirk" case to support this special case. Thanks Jingle -----Original Message----- From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] Sent: Tuesday, January 4, 2022 8:30 AM To: Hans de Goede <hdegoede@redhat.com> Cc: Jingle Wu <jingle.wu@emc.com.tw>; linux-input@vger.kernel.org Subject: Re: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume Hi Hans, On Wed, Dec 22, 2021 at 11:06:41PM +0100, Hans de Goede wrote: > Before these changes elan_suspend() would only call > elan_disable_power() when device_may_wakeup() returns false; whereas > elan_resume() would unconditionally call elan_enable_power(), leading > to an enable count imbalance when device_may_wakeup() returns true. > > This triggers the "WARN_ON(regulator->enable_count)" in > regulator_put() when the elan_i2c driver gets unbound, this happens > e.g. with the hot-plugable dock with Elan I2C touchpad for the Asus TF103C 2-in-1. > > Fix this by making the elan_enable_power() call also be conditional on > device_may_wakeup() returning false. elan_enable_power() not only tries to toggle the regulator, but also tries to issue command to the controller to power/wake it up. I need confirmation from Jingle Wu that skipping this is OK for all Elan touchpad controllers, or if we need to add call to either power control or sleep control method to make sure the controller it fully resumed. Jingle, what can you tell us here? > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/input/mouse/elan_i2c_core.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/input/mouse/elan_i2c_core.c > b/drivers/input/mouse/elan_i2c_core.c > index 47af62c12267..cdb36d35ffa4 100644 > --- a/drivers/input/mouse/elan_i2c_core.c > +++ b/drivers/input/mouse/elan_i2c_core.c > @@ -1412,17 +1412,17 @@ static int __maybe_unused elan_resume(struct device *dev) > struct elan_tp_data *data = i2c_get_clientdata(client); > int error; > > - if (device_may_wakeup(dev) && data->irq_wake) { > + if (!device_may_wakeup(dev)) { > + error = elan_enable_power(data); > + if (error) { > + dev_err(dev, "power up when resuming failed: %d\n", error); > + goto err; > + } > + } else if (data->irq_wake) { > disable_irq_wake(client->irq); > data->irq_wake = false; > } > > - error = elan_enable_power(data); > - if (error) { > - dev_err(dev, "power up when resuming failed: %d\n", error); > - goto err; > - } > - > error = elan_initialize(data, data->quirks & ETP_QUIRK_QUICK_WAKEUP); > if (error) > dev_err(dev, "initialize when resuming failed: %d\n", error); > -- > 2.33.1 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume 2022-01-06 6:50 ` Jingle.Wu @ 2022-01-06 9:22 ` Hans de Goede 0 siblings, 0 replies; 4+ messages in thread From: Hans de Goede @ 2022-01-06 9:22 UTC (permalink / raw) To: Jingle.Wu, 'Dmitry Torokhov'; +Cc: linux-input Hi, On 1/6/22 07:50, Jingle.Wu wrote: > Hi Hans/Dmitry: > > It is not OK to skip elan_enable_power() for all Elan touchpad controllers. > > I suggest to deal with this touchpad module ID as "quirk" case to support > this special case. There is nothing special / quirky about this, regulator enable imbalances simply must not happen / must be fixed. I'll prepare a new version of the patch which adds a parameter to elan_enable_power() to skip the regulator enable if the regulator was not disabled on suspend. This will allow still always calling elan_enable_power() on resume, while also fixing the regulator imbalance. Note that this will also help save power, the regulator imbalance also means that the regulator will no longer get disabled _ever_ after the first suspend/resume cycle where device_may_wakeup() returns true. Regards, Hans > -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > Sent: Tuesday, January 4, 2022 8:30 AM > To: Hans de Goede <hdegoede@redhat.com> > Cc: Jingle Wu <jingle.wu@emc.com.tw>; linux-input@vger.kernel.org > Subject: Re: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance > after suspend/resume > > Hi Hans, > > On Wed, Dec 22, 2021 at 11:06:41PM +0100, Hans de Goede wrote: >> Before these changes elan_suspend() would only call >> elan_disable_power() when device_may_wakeup() returns false; whereas >> elan_resume() would unconditionally call elan_enable_power(), leading >> to an enable count imbalance when device_may_wakeup() returns true. >> >> This triggers the "WARN_ON(regulator->enable_count)" in >> regulator_put() when the elan_i2c driver gets unbound, this happens >> e.g. with the hot-plugable dock with Elan I2C touchpad for the Asus TF103C > 2-in-1. >> >> Fix this by making the elan_enable_power() call also be conditional on >> device_may_wakeup() returning false. > > elan_enable_power() not only tries to toggle the regulator, but also tries > to issue command to the controller to power/wake it up. I need confirmation > from Jingle Wu that skipping this is OK for all Elan touchpad controllers, > or if we need to add call to either power control or sleep control method to > make sure the controller it fully resumed. > > Jingle, what can you tell us here? > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/input/mouse/elan_i2c_core.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/input/mouse/elan_i2c_core.c >> b/drivers/input/mouse/elan_i2c_core.c >> index 47af62c12267..cdb36d35ffa4 100644 >> --- a/drivers/input/mouse/elan_i2c_core.c >> +++ b/drivers/input/mouse/elan_i2c_core.c >> @@ -1412,17 +1412,17 @@ static int __maybe_unused elan_resume(struct > device *dev) >> struct elan_tp_data *data = i2c_get_clientdata(client); >> int error; >> >> - if (device_may_wakeup(dev) && data->irq_wake) { >> + if (!device_may_wakeup(dev)) { >> + error = elan_enable_power(data); >> + if (error) { >> + dev_err(dev, "power up when resuming failed: %d\n", > error); >> + goto err; >> + } >> + } else if (data->irq_wake) { >> disable_irq_wake(client->irq); >> data->irq_wake = false; >> } >> >> - error = elan_enable_power(data); >> - if (error) { >> - dev_err(dev, "power up when resuming failed: %d\n", error); >> - goto err; >> - } >> - >> error = elan_initialize(data, data->quirks & > ETP_QUIRK_QUICK_WAKEUP); >> if (error) >> dev_err(dev, "initialize when resuming failed: %d\n", > error); >> -- >> 2.33.1 >> > > Thanks. > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-06 9:22 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-22 22:06 [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume Hans de Goede 2022-01-04 0:29 ` Dmitry Torokhov 2022-01-06 6:50 ` Jingle.Wu 2022-01-06 9:22 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).