[Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Bjorn Helgaas]

https://bugzilla.kernel.org/show_bug.cgi?id=203297

Regression, suspected but as yet unconfirmed cause:

  c5eb1190074c ("PCI / PM: Allow runtime PM without callback functions")

backported to 4.20 stable as 39e1be324c2f.

Keijo Vaara <ferdasyn@rocketmail.com> reported:

> After upgrading from 4.20.1 to 4.20.2 Synaptics touchpad TM-3127 on HP
> 250 G5 is recognized in all logs like normal, but PCI runtime power
> management seems to keep it from working shortly after module
> initialization. Issuing "modprobe -r psmouse && modprobe psmouse" fixes
> the issue but the device stops working again if inactive for a second or
> so. This is likely caused by https://lkml.org/lkml/2019/1/11/615.
> 
> Possible workarounds known to work:
> 1) Forcing psmouse module to use imps protocol works, but disables most
> of the touchpad functionality such as two finger scrolling.
> 2) Disabling PCI runtime power management for the device (f.e. using tlp
> RUNTIME_PM_ON_BAT=on fixes the issue)
> 3) Downgrade kernel to 4.20.1
> 
> The bug has been reported by multiple users with different software
> setups, all with HP laptops (see
> https://bbs.archlinux.org/viewtopic.php?id=243677 for more info).
> 
> The suspect commit from kernel log:
> 
> commit 39e1be324c2f9048b013aaa190acf91b3f23b1a8
> Author: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Date:   Tue Oct 23 14:45:52 2018 +0300
> 
>     PCI / PM: Allow runtime PM without callback functions
>     
>     commit c5eb1190074cfb14c5d9cac692f1912eecf1a5e4 upstream.
>     
>     a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
>     nullified the runtime PM suspend/resume callback pointers while keeping the
>     runtime PM enabled.
>     
>     This caused the SMBus PCI device to stay in D0 with
>     /sys/devices/.../power/runtime_status showing "error" when the runtime PM
>     framework attempted to autosuspend the device.  This is due to PCI bus
>     runtime PM, which checks for driver runtime PM callbacks and returns
>     -ENOSYS if they are not set.
>     
>     Since i2c-i801.c doesn't need to do anything device-specific for runtime
>     PM, Jean Delvare proposed this be fixed in the PCI core rather than adding
>     dummy runtime PM callback functions in the PCI drivers.
>     
>     Change pci_pm_runtime_suspend()/pci_pm_runtime_resume() so they allow
>     changing the PCI device power state during runtime PM transitions even if
>     the driver supplies no runtime PM callbacks.
>     
>     This fixes the runtime PM regression on i2c-i801.c.
>     
>     It is not obvious why the code previously required the runtime PM
>     callbacks.  The test has been there since the code was introduced by
>     6cbf82148ff2 ("PCI PM: Run-time callbacks for PCI bus type").
>     
>     On the other hand, a similar change was done to generic runtime PM
>     callbacks in 05aa55dddb9e ("PM / Runtime: Lenient generic runtime pm
>     callbacks").
>     
>     Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
>     Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>     Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Jean Delvare <jdelvare@suse.de>
>     Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>     Cc: stable@vger.kernel.org      # v4.18+
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Jarkko Nikula]

On 4/22/19 4:08 PM, Bjorn Helgaas wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=203297
> 
> Regression, suspected but as yet unconfirmed cause:
> 
>    c5eb1190074c ("PCI / PM: Allow runtime PM without callback functions")
> 
> backported to 4.20 stable as 39e1be324c2f.
> 
With help of Keijo it was confirmed above patch broke the Synaptics 
touchpad. Not bisected but touchpad works again by forcing the i2c-i801 
SMBus controller always on:
"echo on >/sys/bus/pci/devices/0000\:00\:1f.3/power/control"

Above patch is a generalized fix that fixed the runtime PM regression on 
i2c-i801 and re-allow the controller go to runtime suspend when idle. So 
most probably Synaptics touchpad was broken by i2c-i801 runtime PM also 
before but got unnoticed. Which is easy since on many platforms SMBus 
controller doesn't necessarily have the PCI PM capabilities.

I would like to ask help from input subsystem experts what kind of SMBus 
power state dependency Synaptics RMI4 SMBus devices have since it cease 
to work if SMBus controllers idles between transfers and how this is 
best to fix?

Instead of revert I think we'd need to have some method to force SMBus 
controller on whenever the touchpad is active, like when there is a 
userspace listening.

I'm not expert in this area so as quick proof of concept I had a 
following hack which forces the I2C/SMBus adapter, and eventually the 
parent PCI device of it on when the RMI4 SMBus device is probed and let 
the SMBus controller to idle when removed.

According to Keijo it fixes the issue but I like to hear input experts 
for better place to put these.

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index b6ccf39c6a7b..2b11d69be313 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -16,6 +16,7 @@
  #include <linux/lockdep.h>
  #include <linux/module.h>
  #include <linux/pm.h>
+#include <linux/pm_runtime.h>
  #include <linux/rmi.h>
  #include <linux/slab.h>
  #include "rmi_driver.h"
@@ -332,6 +333,9 @@ static int rmi_smb_probe(struct i2c_client *client,

  	dev_info(&client->dev, "registering SMbus-connected sensor\n");

+	/* Force SMBus adapter on while RMI4 device is connected */
+	pm_runtime_get(&client->adapter->dev);
+
  	error = rmi_register_transport_device(&rmi_smb->xport);
  	if (error) {
  		dev_err(&client->dev, "failed to register sensor: %d\n", error);
@@ -346,6 +350,7 @@ static int rmi_smb_remove(struct i2c_client *client)
  	struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);

  	rmi_unregister_transport_device(&rmi_smb->xport);
+	pm_runtime_put(&client->adapter->dev);

  	return 0;
  }

-- 
Jarkko

Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Rafael J. Wysocki]

On Fri, Apr 26, 2019 at 2:14 PM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> On 4/22/19 4:08 PM, Bjorn Helgaas wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=203297
> >
> > Regression, suspected but as yet unconfirmed cause:
> >
> >    c5eb1190074c ("PCI / PM: Allow runtime PM without callback functions")
> >
> > backported to 4.20 stable as 39e1be324c2f.
> >
> With help of Keijo it was confirmed above patch broke the Synaptics
> touchpad. Not bisected but touchpad works again by forcing the i2c-i801
> SMBus controller always on:
> "echo on >/sys/bus/pci/devices/0000\:00\:1f.3/power/control"
>
> Above patch is a generalized fix that fixed the runtime PM regression on
> i2c-i801 and re-allow the controller go to runtime suspend when idle. So
> most probably Synaptics touchpad was broken by i2c-i801 runtime PM also
> before but got unnoticed. Which is easy since on many platforms SMBus
> controller doesn't necessarily have the PCI PM capabilities.
>
> I would like to ask help from input subsystem experts what kind of SMBus
> power state dependency Synaptics RMI4 SMBus devices have since it cease
> to work if SMBus controllers idles between transfers and how this is
> best to fix?
>
> Instead of revert I think we'd need to have some method to force SMBus
> controller on whenever the touchpad is active, like when there is a
> userspace listening.
>
> I'm not expert in this area so as quick proof of concept I had a
> following hack which forces the I2C/SMBus adapter, and eventually the
> parent PCI device of it on when the RMI4 SMBus device is probed and let
> the SMBus controller to idle when removed.
>
> According to Keijo it fixes the issue but I like to hear input experts
> for better place to put these.
>
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index b6ccf39c6a7b..2b11d69be313 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -16,6 +16,7 @@
>   #include <linux/lockdep.h>
>   #include <linux/module.h>
>   #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/rmi.h>
>   #include <linux/slab.h>
>   #include "rmi_driver.h"
> @@ -332,6 +333,9 @@ static int rmi_smb_probe(struct i2c_client *client,
>
>         dev_info(&client->dev, "registering SMbus-connected sensor\n");
>
> +       /* Force SMBus adapter on while RMI4 device is connected */
> +       pm_runtime_get(&client->adapter->dev);

That should be pm_runtime_get_sync() IMO.

Otherwise, the rmi_register_transport_device() may be called before
completing the PM transition.

> +
>         error = rmi_register_transport_device(&rmi_smb->xport);
>         if (error) {
>                 dev_err(&client->dev, "failed to register sensor: %d\n", error);
> @@ -346,6 +350,7 @@ static int rmi_smb_remove(struct i2c_client *client)
>         struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
>
>         rmi_unregister_transport_device(&rmi_smb->xport);
> +       pm_runtime_put(&client->adapter->dev);
>
>         return 0;
>   }
>
> --
> Jarkko

Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Benjamin Tissoires]

On Fri, Apr 26, 2019 at 2:14 PM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> On 4/22/19 4:08 PM, Bjorn Helgaas wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=203297
> >
> > Regression, suspected but as yet unconfirmed cause:
> >
> >    c5eb1190074c ("PCI / PM: Allow runtime PM without callback functions")
> >
> > backported to 4.20 stable as 39e1be324c2f.
> >
> With help of Keijo it was confirmed above patch broke the Synaptics
> touchpad. Not bisected but touchpad works again by forcing the i2c-i801
> SMBus controller always on:
> "echo on >/sys/bus/pci/devices/0000\:00\:1f.3/power/control"
>
> Above patch is a generalized fix that fixed the runtime PM regression on
> i2c-i801 and re-allow the controller go to runtime suspend when idle. So
> most probably Synaptics touchpad was broken by i2c-i801 runtime PM also
> before but got unnoticed. Which is easy since on many platforms SMBus
> controller doesn't necessarily have the PCI PM capabilities.
>
> I would like to ask help from input subsystem experts what kind of SMBus
> power state dependency Synaptics RMI4 SMBus devices have since it cease
> to work if SMBus controllers idles between transfers and how this is
> best to fix?

Hmm, I am not sure there is such an existing architecture you could
use in a simple patch.

rmi-driver.c does indeed create an input device we could use to toggle
on/off the PM state, but those callbacks are not wired to the
transport driver (rmi_smbus.c), so it would required a little bit of
extra work. And then, there are other RMI4 functions (firmware
upgrade) that would not be happy if PM is in suspend while there is no
open input node.

So I think this "hack" (with Mika's comments addressed) should go in
until someone starts propagating the PM states correctly.

Cheers,
Benjamin

>
> Instead of revert I think we'd need to have some method to force SMBus
> controller on whenever the touchpad is active, like when there is a
> userspace listening.
>
> I'm not expert in this area so as quick proof of concept I had a
> following hack which forces the I2C/SMBus adapter, and eventually the
> parent PCI device of it on when the RMI4 SMBus device is probed and let
> the SMBus controller to idle when removed.
>
> According to Keijo it fixes the issue but I like to hear input experts
> for better place to put these.
>
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index b6ccf39c6a7b..2b11d69be313 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -16,6 +16,7 @@
>   #include <linux/lockdep.h>
>   #include <linux/module.h>
>   #include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/rmi.h>
>   #include <linux/slab.h>
>   #include "rmi_driver.h"
> @@ -332,6 +333,9 @@ static int rmi_smb_probe(struct i2c_client *client,
>
>         dev_info(&client->dev, "registering SMbus-connected sensor\n");
>
> +       /* Force SMBus adapter on while RMI4 device is connected */
> +       pm_runtime_get(&client->adapter->dev);
> +
>         error = rmi_register_transport_device(&rmi_smb->xport);
>         if (error) {
>                 dev_err(&client->dev, "failed to register sensor: %d\n", error);
> @@ -346,6 +350,7 @@ static int rmi_smb_remove(struct i2c_client *client)
>         struct rmi_smb_xport *rmi_smb = i2c_get_clientdata(client);
>
>         rmi_unregister_transport_device(&rmi_smb->xport);
> +       pm_runtime_put(&client->adapter->dev);
>
>         return 0;
>   }
>
> --
> Jarkko

Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Jarkko Nikula]

[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]

On 4/29/19 10:45 AM, Benjamin Tissoires wrote:
>> I would like to ask help from input subsystem experts what kind of SMBus
>> power state dependency Synaptics RMI4 SMBus devices have since it cease
>> to work if SMBus controllers idles between transfers and how this is
>> best to fix?
> 
> Hmm, I am not sure there is such an existing architecture you could
> use in a simple patch.
> 
> rmi-driver.c does indeed create an input device we could use to toggle
> on/off the PM state, but those callbacks are not wired to the
> transport driver (rmi_smbus.c), so it would required a little bit of
> extra work. And then, there are other RMI4 functions (firmware
> upgrade) that would not be happy if PM is in suspend while there is no
> open input node.
> 
I see.

I got another thought about this. I noticed these input drivers need 
SMBus Host Notify, maybe that explain the PM dependency? If that's the 
only dependency then we could prevent the controller suspend if there is 
a client needing host notify mechanism. IMHO that's less hack than the 
patch to rmi_smbus.c.

Keijo: care to test does this idea would fix the issue (without the 
previous patch)? I also attached the diff.

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 38af18645133..d54eafad7727 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -327,6 +327,8 @@ static int i2c_device_probe(struct device *dev)

  		if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
  			dev_dbg(dev, "Using Host Notify IRQ\n");
+			/* Adapter should not suspend for Host Notify */
+			pm_runtime_get_sync(&client->adapter->dev);
  			irq = i2c_smbus_host_notify_to_irq(client);
  		} else if (dev->of_node) {
  			irq = of_irq_get_byname(dev->of_node, "irq");
@@ -431,6 +433,8 @@ static int i2c_device_remove(struct device *dev)
  	device_init_wakeup(&client->dev, false);

  	client->irq = client->init_irq;
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
+		pm_runtime_put(&client->adapter->dev);

  	return status;
  }

> So I think this "hack" (with Mika's comments addressed) should go in
> until someone starts propagating the PM states correctly.
> 
I guess you mean the Rafael's pm_runtime_get_sync() comment?

-- 
Jarkko

[-- Attachment #2: i2c-core-host-notify-pm.diff --]
[-- Type: text/x-patch, Size: 830 bytes --]

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 38af18645133..d54eafad7727 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -327,6 +327,8 @@ static int i2c_device_probe(struct device *dev)
 
 		if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
 			dev_dbg(dev, "Using Host Notify IRQ\n");
+			/* Adapter should not suspend for Host Notify */
+			pm_runtime_get_sync(&client->adapter->dev);
 			irq = i2c_smbus_host_notify_to_irq(client);
 		} else if (dev->of_node) {
 			irq = of_irq_get_byname(dev->of_node, "irq");
@@ -431,6 +433,8 @@ static int i2c_device_remove(struct device *dev)
 	device_init_wakeup(&client->dev, false);
 
 	client->irq = client->init_irq;
+	if (client->flags & I2C_CLIENT_HOST_NOTIFY)
+		pm_runtime_put(&client->adapter->dev);
 
 	return status;
 }

Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Benjamin Tissoires]

On Mon, Apr 29, 2019 at 10:38 AM Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
>
> On 4/29/19 10:45 AM, Benjamin Tissoires wrote:
> >> I would like to ask help from input subsystem experts what kind of SMBus
> >> power state dependency Synaptics RMI4 SMBus devices have since it cease
> >> to work if SMBus controllers idles between transfers and how this is
> >> best to fix?
> >
> > Hmm, I am not sure there is such an existing architecture you could
> > use in a simple patch.
> >
> > rmi-driver.c does indeed create an input device we could use to toggle
> > on/off the PM state, but those callbacks are not wired to the
> > transport driver (rmi_smbus.c), so it would required a little bit of
> > extra work. And then, there are other RMI4 functions (firmware
> > upgrade) that would not be happy if PM is in suspend while there is no
> > open input node.
> >
> I see.
>
> I got another thought about this. I noticed these input drivers need
> SMBus Host Notify, maybe that explain the PM dependency? If that's the
> only dependency then we could prevent the controller suspend if there is
> a client needing host notify mechanism. IMHO that's less hack than the
> patch to rmi_smbus.c.

So currently, AFAIK, only Synaptics (rmi4) and Elantech are using
SMBus Host Notify.
So this patch would prevent the same bugs for those 2 vendors, which is good.

It took me some time to understand why this would be less than a hack.
And indeed, given that Host Notify relies on the I2C connection to be
ready for the IRQ, we can not put the controller in suspend like we do
for others where the IRQ controller is still ready.

So yes, that could work from me. Not sure what Wolfram and Jean would
say though.

>
> Keijo: care to test does this idea would fix the issue (without the
> previous patch)? I also attached the diff.
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 38af18645133..d54eafad7727 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -327,6 +327,8 @@ static int i2c_device_probe(struct device *dev)
>
>                 if (client->flags & I2C_CLIENT_HOST_NOTIFY) {
>                         dev_dbg(dev, "Using Host Notify IRQ\n");
> +                       /* Adapter should not suspend for Host Notify */
> +                       pm_runtime_get_sync(&client->adapter->dev);
>                         irq = i2c_smbus_host_notify_to_irq(client);
>                 } else if (dev->of_node) {
>                         irq = of_irq_get_byname(dev->of_node, "irq");
> @@ -431,6 +433,8 @@ static int i2c_device_remove(struct device *dev)
>         device_init_wakeup(&client->dev, false);
>
>         client->irq = client->init_irq;
> +       if (client->flags & I2C_CLIENT_HOST_NOTIFY)
> +               pm_runtime_put(&client->adapter->dev);
>
>         return status;
>   }
>
> > So I think this "hack" (with Mika's comments addressed) should go in
> > until someone starts propagating the PM states correctly.
> >
> I guess you mean the Rafael's pm_runtime_get_sync() comment?

Oops, yes, that one, sorry :)

Cheers,
Benjamin

Re: [Bug 203297] Synaptics touchpad TM-3127 functionality broken by PCI runtime power management patch on 4.20.2

[Jean Delvare]

On Mon, 2019-04-29 at 10:53 +0200, Benjamin Tissoires wrote:
> On Mon, Apr 29, 2019 at 10:38 AM Jarkko Nikula
> <jarkko.nikula@linux.intel.com> wrote:
> > I got another thought about this. I noticed these input drivers need
> > SMBus Host Notify, maybe that explain the PM dependency? If that's the
> > only dependency then we could prevent the controller suspend if there is
> > a client needing host notify mechanism. IMHO that's less hack than the
> > patch to rmi_smbus.c.
> 
> So currently, AFAIK, only Synaptics (rmi4) and Elantech are using
> SMBus Host Notify.
> So this patch would prevent the same bugs for those 2 vendors, which is good.
> 
> It took me some time to understand why this would be less than a hack.
> And indeed, given that Host Notify relies on the I2C connection to be
> ready for the IRQ, we can not put the controller in suspend like we do
> for others where the IRQ controller is still ready.
> 
> So yes, that could work from me. Not sure what Wolfram and Jean would
> say though.

I would say OK with me, this looks like the cleanest solution to me, so
if testing is positive, let's go with it.

-- 
Jean Delvare
SUSE L3 Support