| Message ID | 20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Fix spurious wakes on ACPI platforms | expand |
Hi, On 12/21/21 00:43, Raul E Rangel wrote: > The ACPI subsystem is responsible for managing the power and wake > sources for an ACPI device. By explicitly calling > device_set_wakeup_capable, we are circumvent the ACPI subsystem and > setting wake capabilities on the device when it doesn't support it. > > Take the following example: > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have > `_PRW` so that means the device can't wake the system. > * The IRQ line is active level low for this device and is pulled up by the > power resource defined in `_PR0`/`_PR3`. > > Since the i2c-hid driver has set the device as wake capable, the wake > pin gets enabled on suspend. The IRQ pin should only have a enable_irq_wake() called on it if something has actually requested the i2c-HID device to be a wakeup source, the removed code claims the device is wakeup *capable*, but is also explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup. And i2c-hid suspend does: if (device_may_wakeup(&client->dev)) { wake_status = enable_irq_wake(client->irq); And device_may_wakeup() checks the wakeup *enabled* setting AFAIK. I've added Rafael to the Cc since he knows all this a lot better then me. I have the feeling that your userspace is perhaps poking the wakeup settings in sysfs, triggering this issue. > As part of suspend, ACPI will power down > the device since it's not a wake source. When the device is powered > down, the IRQ line will drop, and it will trigger a wake event. To me that sounds like the device is not wakeup *capable* at all, so its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all. Note I'm not certain about this at all, but at a first look this feels like it is not the right fix for your problem. Regards, Hans > > See the following debug log: > [ 42.335804] PM: Suspending system (s2idle) > [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable > [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off > [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold > [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd > [ 42.535293] PM: Wakeup unrelated to ACPI SCI > [ 42.535294] PM: resume from suspend-to-idle > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > --- > > drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c > index a6f0257a26de..fc311a19a19d 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c > @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client) > > acpi_device_fix_up_power(adev); > > - if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { > - device_set_wakeup_capable(dev, true); > - device_set_wakeup_enable(dev, false); > - } > - > return i2c_hid_core_probe(client, &ihid_acpi->ops, > hid_descriptor_address); > } >
On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 12/21/21 00:43, Raul E Rangel wrote: > > The ACPI subsystem is responsible for managing the power and wake > > sources for an ACPI device. By explicitly calling > > device_set_wakeup_capable, we are circumvent the ACPI subsystem and > > setting wake capabilities on the device when it doesn't support it. > > > > Take the following example: > > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have > > `_PRW` so that means the device can't wake the system. > > * The IRQ line is active level low for this device and is pulled up by the > > power resource defined in `_PR0`/`_PR3`. > > > > Since the i2c-hid driver has set the device as wake capable, the wake > > pin gets enabled on suspend. > > The IRQ pin should only have a enable_irq_wake() called on it if > something has actually requested the i2c-HID device to be a wakeup source, > the removed code claims the device is wakeup *capable*, but is also > explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup. > > And i2c-hid suspend does: > > if (device_may_wakeup(&client->dev)) { > wake_status = enable_irq_wake(client->irq); > > And device_may_wakeup() checks the wakeup *enabled* setting AFAIK. > > I've added Rafael to the Cc since he knows all this a lot better then me. > > I have the feeling that your userspace is perhaps poking the > wakeup settings in sysfs, triggering this issue. You are correct, I added some printks in and it is userspace enabling the wake: [ 3.280464] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: disabled [ 3.280502] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: enabled [ 3.280537] i2c_hid_acpi i2c-GDIX0000:00: device_wakeup_enable: start [ 3.280541] CPU: 0 PID: 1248 Comm: powerd Not tainted 5.10.83 #151 c334d4c4185a84ded39aafcb495de6870a8e5161 [ 3.280545] Hardware name: Google Guybrush/Guybrush, BIOS Google_Guybrush.4.15-624-g9d80a9c6aa40 12/21/2021 [ 3.280548] Call Trace: [ 3.280554] dump_stack+0x9c/0xe7 [ 3.280560] device_wakeup_enable+0x136/0x172 [ 3.280564] wakeup_store+0xbc/0xc4 [ 3.280572] kernfs_fop_write_iter+0x10b/0x18a [ 3.280576] vfs_write+0x383/0x405 [ 3.280579] ksys_write+0x74/0xd4 [ 3.280583] do_syscall_64+0x43/0x55 [ 3.280587] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > As part of suspend, ACPI will power down > > the device since it's not a wake source. When the device is powered > > down, the IRQ line will drop, and it will trigger a wake event. > > To me that sounds like the device is not wakeup *capable* at all, so > its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all. The ACPI_FADT_LOW_POWER_S0 flag is a system level flag. The system is wake capable and supports S0ix. The touchscreen device does not support waking the system because it doesn't provide a `_PRW`. > > Note I'm not certain about this at all, but at a first look this feels > like it is not the right fix for your problem. We can't have the `i2c-hid-acpi` driver calling `device_set_wakeup_capable`. This will make the kernel expose the wakeup sysfs entry to userspace regardless if the device supports wakeup or not. This would require userspace to know that enabling this wake source will cause suspend problems and to avoid it. > > Regards, > > Hans Thanks for the review! Raul > > > > > > See the following debug log: > > [ 42.335804] PM: Suspending system (s2idle) > > [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable > > [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off > > [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold > > [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd > > [ 42.535293] PM: Wakeup unrelated to ACPI SCI > > [ 42.535294] PM: resume from suspend-to-idle > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > --- > > > > drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c > > index a6f0257a26de..fc311a19a19d 100644 > > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c > > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c > > @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client) > > > > acpi_device_fix_up_power(adev); > > > > - if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { > > - device_set_wakeup_capable(dev, true); > > - device_set_wakeup_enable(dev, false); > > - } > > - > > return i2c_hid_core_probe(client, &ihid_acpi->ops, > > hid_descriptor_address); > > } > > >
On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 12/21/21 00:43, Raul E Rangel wrote: > > The ACPI subsystem is responsible for managing the power and wake > > sources for an ACPI device. By explicitly calling > > device_set_wakeup_capable, we are circumvent the ACPI subsystem and > > setting wake capabilities on the device when it doesn't support it. > > > > Take the following example: > > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have > > `_PRW` so that means the device can't wake the system. > > * The IRQ line is active level low for this device and is pulled up by the > > power resource defined in `_PR0`/`_PR3`. > > > > Since the i2c-hid driver has set the device as wake capable, the wake > > pin gets enabled on suspend. > > The IRQ pin should only have a enable_irq_wake() called on it if > something has actually requested the i2c-HID device to be a wakeup source, > the removed code claims the device is wakeup *capable*, but is also > explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup. > I thought of something else: > And i2c-hid suspend does: > > if (device_may_wakeup(&client->dev)) { > wake_status = enable_irq_wake(client->irq); > I think we also need to guard the enable_irq_wake call with `!ACPI_COMPANION(dev)`. ACPI will handle enabling the correct GPIO or GPE defined in `_PRW`. We might also be able to remove manually calling {enable,disable}_irq_wake by switching over to `dev_pm_set_wake_irq`. I did this for the elan_i2c driver in the 2nd patch: https://patchwork.kernel.org/project/linux-input/patch/20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid/ > And device_may_wakeup() checks the wakeup *enabled* setting AFAIK. > > I've added Rafael to the Cc since he knows all this a lot better then me. > > I have the feeling that your userspace is perhaps poking the > wakeup settings in sysfs, triggering this issue. > > > As part of suspend, ACPI will power down > > the device since it's not a wake source. When the device is powered > > down, the IRQ line will drop, and it will trigger a wake event. > > To me that sounds like the device is not wakeup *capable* at all, so > its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all. > > Note I'm not certain about this at all, but at a first look this feels > like it is not the right fix for your problem. > > Regards, > > Hans > > > > > > See the following debug log: > > [ 42.335804] PM: Suspending system (s2idle) > > [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable > > [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off > > [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold > > [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd > > [ 42.535293] PM: Wakeup unrelated to ACPI SCI > > [ 42.535294] PM: resume from suspend-to-idle > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org> > > --- > > > > drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c > > index a6f0257a26de..fc311a19a19d 100644 > > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c > > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c > > @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client) > > > > acpi_device_fix_up_power(adev); > > > > - if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { > > - device_set_wakeup_capable(dev, true); > > - device_set_wakeup_enable(dev, false); > > - } > > - > > return i2c_hid_core_probe(client, &ihid_acpi->ops, > > hid_descriptor_address); > > } > > >
Hi, On 12/22/21 00:40, Raul Rangel wrote: > On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 12/21/21 00:43, Raul E Rangel wrote: >>> The ACPI subsystem is responsible for managing the power and wake >>> sources for an ACPI device. By explicitly calling >>> device_set_wakeup_capable, we are circumvent the ACPI subsystem and >>> setting wake capabilities on the device when it doesn't support it. >>> >>> Take the following example: >>> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have >>> `_PRW` so that means the device can't wake the system. >>> * The IRQ line is active level low for this device and is pulled up by the >>> power resource defined in `_PR0`/`_PR3`. >>> >>> Since the i2c-hid driver has set the device as wake capable, the wake >>> pin gets enabled on suspend. >> >> The IRQ pin should only have a enable_irq_wake() called on it if >> something has actually requested the i2c-HID device to be a wakeup source, >> the removed code claims the device is wakeup *capable*, but is also >> explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup. >> >> And i2c-hid suspend does: >> >> if (device_may_wakeup(&client->dev)) { >> wake_status = enable_irq_wake(client->irq); >> >> And device_may_wakeup() checks the wakeup *enabled* setting AFAIK. >> >> I've added Rafael to the Cc since he knows all this a lot better then me. >> >> I have the feeling that your userspace is perhaps poking the >> wakeup settings in sysfs, triggering this issue. > > You are correct, I added some printks in and it is userspace enabling the wake: > > [ 3.280464] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: disabled > [ 3.280502] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: enabled > [ 3.280537] i2c_hid_acpi i2c-GDIX0000:00: device_wakeup_enable: start > [ 3.280541] CPU: 0 PID: 1248 Comm: powerd Not tainted 5.10.83 > #151 c334d4c4185a84ded39aafcb495de6870a8e5161 > [ 3.280545] Hardware name: Google Guybrush/Guybrush, BIOS > Google_Guybrush.4.15-624-g9d80a9c6aa40 12/21/2021 > [ 3.280548] Call Trace: > [ 3.280554] dump_stack+0x9c/0xe7 > [ 3.280560] device_wakeup_enable+0x136/0x172 > [ 3.280564] wakeup_store+0xbc/0xc4 > [ 3.280572] kernfs_fop_write_iter+0x10b/0x18a > [ 3.280576] vfs_write+0x383/0x405 > [ 3.280579] ksys_write+0x74/0xd4 > [ 3.280583] do_syscall_64+0x43/0x55 > [ 3.280587] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > >> >>> As part of suspend, ACPI will power down >>> the device since it's not a wake source. When the device is powered >>> down, the IRQ line will drop, and it will trigger a wake event. >> >> To me that sounds like the device is not wakeup *capable* at all, so >> its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all. > > The ACPI_FADT_LOW_POWER_S0 flag is a system level flag. The > system is wake capable and supports S0ix. The touchscreen device > does not support waking the system because it doesn't provide > a `_PRW`. I've checked a whole bunch of DSDTs from standard x86 ACPI laptops and it seems the _PRW is only used when devices use a GPE for wakeup; not when they are using a standard GPIO-irq as wakeup source. To be specific _PRW seems to only be present on PCI devices and on the ACPI pwr-button and ACPI lid switch devices. I2C attached touchpads and touchscreens seem to never have _PRW set (at least in the collection of DSDTs which I have). Note that what you are doing here is in essence completely reverting commit 203c38fbe833 ("HID: i2c-hid: Enable wakeup capability from Suspend-to-Idle") (note when I reviewed that I deliberately asked for the wakeup to be disabled by default to avoid regressions). Going ahead with this change would in essence cause regressions, breaking wakeup by touchpad on many many laptops, so NACK. >> Note I'm not certain about this at all, but at a first look this feels >> like it is not the right fix for your problem. > > We can't have the `i2c-hid-acpi` driver calling `device_set_wakeup_capable`. > This will make the kernel expose the wakeup sysfs entry to userspace > regardless if the device supports wakeup or not. This would require userspace > to know that enabling this wake source will cause suspend problems and to > avoid it. I'm in 2 minds about this. To me this feels like the story where a person goes to the doctor and says "doctor if I do this-thing it hurts". To which the doctor replies: "Well then don't do this-thing". IOW if ChromeOS would not write the wakeup sysfs attribute then there would be no problem If I've understood the problem correctly then the issue is with a touchscreen right? Unlike the I2C-HID driver which does not know the difference between a touchpad, touchscreen or say a sensor-hub. Userspace can easily query the associated input evdev node and NOT enable wakeup on touchscreens (which seems to be a weird thing to do regardless of if it works properly or not). Blindly enabling wakeup on all devices seems like it is a recipe for hitting all sort of interesting bugs; and it seems best to just avoid that in the first place. As I said I'm in 2 minds about this, OTOH this does feel like a hardware bug why does the interrupt trigger when the touchscreen is put into S3 ? To me this feels as if the interrupt line is missing a pull-up/-down or has the wrong pull-up/-down setting requiring it to be actively driven to the not-irq state and sending an irq signal as soon as the line is not actively driven. Could this be a problem with the configuration of the built-in pull-up/-downs of the GPIO pin used for the IRQ (which can be controlled by the firmware) ? you are right that the kernel should not export a non working wakeup sysfs attribute, so if this cannot be fixed in firmware, then the best option is probably to add a quirk for this model to the existing quirk list in the I2C-HID code and to not do the device_set_wakeup_capable() call on this specific touchscreen based on a new quirk. > I thought of something else: > >> And i2c-hid suspend does: >> >> if (device_may_wakeup(&client->dev)) { >> wake_status = enable_irq_wake(client->irq); >> > > I think we also need to guard the enable_irq_wake call with > `!ACPI_COMPANION(dev)`. > ACPI will handle enabling the correct GPIO or GPE defined in `_PRW`. > Calling enable_irq_wake() is necessary for wakeup to work on devices which use a normal GPIO IRQ rather then a GPE for wakeup. The only case where the enable_irq_wake() should probably not be done is when their is an ACPI_COMPANION and that ACPI companion has a valid _PRW, which as mentioned above most (all?) I2C-HID ACPI device nodes do _not_ have. Regards, Hans >>> >>> See the following debug log: >>> [ 42.335804] PM: Suspending system (s2idle) >>> [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable >>> [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off >>> [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold >>> [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd >>> [ 42.535293] PM: Wakeup unrelated to ACPI SCI >>> [ 42.535294] PM: resume from suspend-to-idle >>> >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org> >>> --- >>> >>> drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ----- >>> 1 file changed, 5 deletions(-) >>> >>> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c >>> index a6f0257a26de..fc311a19a19d 100644 >>> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c >>> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c >>> @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client) >>> >>> acpi_device_fix_up_power(adev); >>> >>> - if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { >>> - device_set_wakeup_capable(dev, true); >>> - device_set_wakeup_enable(dev, false); >>> - } >>> - >>> return i2c_hid_core_probe(client, &ihid_acpi->ops, >>> hid_descriptor_address); >>> } >>> >> >
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c index a6f0257a26de..fc311a19a19d 100644 --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client) acpi_device_fix_up_power(adev); - if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { - device_set_wakeup_capable(dev, true); - device_set_wakeup_enable(dev, false); - } - return i2c_hid_core_probe(client, &ihid_acpi->ops, hid_descriptor_address); }
The ACPI subsystem is responsible for managing the power and wake sources for an ACPI device. By explicitly calling device_set_wakeup_capable, we are circumvent the ACPI subsystem and setting wake capabilities on the device when it doesn't support it. Take the following example: * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have `_PRW` so that means the device can't wake the system. * The IRQ line is active level low for this device and is pulled up by the power resource defined in `_PR0`/`_PR3`. Since the i2c-hid driver has set the device as wake capable, the wake pin gets enabled on suspend. As part of suspend, ACPI will power down the device since it's not a wake source. When the device is powered down, the IRQ line will drop, and it will trigger a wake event. See the following debug log: [ 42.335804] PM: Suspending system (s2idle) [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd [ 42.535293] PM: Wakeup unrelated to ACPI SCI [ 42.535294] PM: resume from suspend-to-idle Signed-off-by: Raul E Rangel <rrangel@chromium.org> --- drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ----- 1 file changed, 5 deletions(-)