On Sep 11 2025, Mario Limonciello wrote: > On 9/11/25 8:49 AM, Benjamin Tissoires wrote: > > Hi Mario, > > > > On Sep 09 2025, Mario Limonciello (AMD) wrote: > > > Dell systems utilize an EC-based touchpad emulation when the ACPI > > > touchpad _DSM is not invoked. This emulation acts as a secondary > > > master on the I2C bus, designed for scenarios where the I2C touchpad > > > driver is absent, such as in BIOS menus. Typically, loading the > > > i2c-hid module triggers the _DSM at initialization, disabling the > > > EC-based emulation. > > > > > > However, if the i2c-hid module is missing from the boot kernel > > > used for hibernation snapshot restoration, the _DSM remains > > > uncalled, resulting in dual masters on the I2C bus and > > > subsequent arbitration errors. This issue arises when i2c-hid > > > resides in the rootfs instead of the kernel or initramfs. > > > > In the downstream Red Hat bug it was mentioned that the kernel > > configuration had an impact though. But as I understand it now that I'm > > re-reading it for the 4th time: > > - on stock fedora kernel (i2c-hid in initramfs): works > > - on stock RHEL kernel (i2c-hid in initramfs): bug but config issue > > - on vanilla kernel without i2c-hid in initramfs: bug fixed by that > > patch > > - on vanilla kernel with i2c-hid in initramfs: works > > The detail you're missing here for #4 is vanilla kernel *with Fedora kernel > config* not with a defconfig. A defconfig kernel I don't expect enables all > the config options needed for this hardware. > > I've reproduced it with my own kernel config as well which is not the Fedora > or RHEL kernel config but rather a narrowed down config of options that I > normally use for testing. > > > > > So that patch is needed no matter the config bug we have there and it > > will also fix that config bug. Am I correct? > > Yes there are certainly circumstances it is needed if i2c-hid isn't present > in the kernel used to restore the hibernation image. > > > > > > > > > To address this, switch from using the SYSTEM_SLEEP_PM_OPS() > > > macro to dedicated callbacks, introducing a specific > > > callback for restoring the S4 image. This callback ensures > > > the _DSM is invoked. > > > > I'm a little bit hesitant to include this patch without proper testing. > > Did you run this through a wider range of laptops than just the > > problematic one? > > I have been including it in my (local) development and testing tree ever > since I made it about a month ago so I've thrown it at a random assortment > of hardware I boot that on. I have also been doing S4 testing explicitly > because of checking that I didn't break S4 with by S4 at S5 series [1]. > > That hardware is all biased to be AMD hardware though. Alright, thanks for the precision. I'll run on my intel based laptop a few tests then and will include this upstream. Cheers, Benjamin > > [1] https://lore.kernel.org/linux-usb/20250909191619.2580169-1-superm1@xxxxxxxxxx/ > > > > Other than that, patch looks good to me, but I would have liked to have > > another opinion on it. > > > > Cheers, > > Benjamin > > > > > > > > Signed-off-by: Mario Limonciello (AMD) <superm1@xxxxxxxxxx> > > > --- > > > drivers/hid/i2c-hid/i2c-hid-acpi.c | 8 ++++++++ > > > drivers/hid/i2c-hid/i2c-hid-core.c | 28 +++++++++++++++++++++++++++- > > > drivers/hid/i2c-hid/i2c-hid.h | 2 ++ > > > 3 files changed, 37 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c > > > index 1b49243adb16a..abd700a101f46 100644 > > > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c > > > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c > > > @@ -76,6 +76,13 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi) > > > return hid_descriptor_address; > > > } > > > +static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops) > > > +{ > > > + struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops); > > > + > > > + i2c_hid_acpi_get_descriptor(ihid_acpi); > > > +} > > > + > > > static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops) > > > { > > > struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops); > > > @@ -96,6 +103,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client) > > > ihid_acpi->adev = ACPI_COMPANION(dev); > > > ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail; > > > + ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence; > > > ret = i2c_hid_acpi_get_descriptor(ihid_acpi); > > > if (ret < 0) > > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > > > index d3912e3f2f13a..3257aa87be898 100644 > > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > > > @@ -961,6 +961,14 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid) > > > ihid->ops->shutdown_tail(ihid->ops); > > > } > > > +static void i2c_hid_core_restore_sequence(struct i2c_hid *ihid) > > > +{ > > > + if (!ihid->ops->restore_sequence) > > > + return; > > > + > > > + ihid->ops->restore_sequence(ihid->ops); > > > +} > > > + > > > static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff) > > > { > > > struct i2c_client *client = ihid->client; > > > @@ -1360,8 +1368,26 @@ static int i2c_hid_core_pm_resume(struct device *dev) > > > return i2c_hid_core_resume(ihid); > > > } > > > +static int i2c_hid_core_pm_restore(struct device *dev) > > > +{ > > > + struct i2c_client *client = to_i2c_client(dev); > > > + struct i2c_hid *ihid = i2c_get_clientdata(client); > > > + > > > + if (ihid->is_panel_follower) > > > + return 0; > > > + > > > + i2c_hid_core_restore_sequence(ihid); > > > + > > > + return i2c_hid_core_resume(ihid); > > > +} > > > + > > > const struct dev_pm_ops i2c_hid_core_pm = { > > > - SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume) > > > + .suspend = pm_sleep_ptr(i2c_hid_core_pm_suspend), > > > + .resume = pm_sleep_ptr(i2c_hid_core_pm_resume), > > > + .freeze = pm_sleep_ptr(i2c_hid_core_pm_suspend), > > > + .thaw = pm_sleep_ptr(i2c_hid_core_pm_resume), > > > + .poweroff = pm_sleep_ptr(i2c_hid_core_pm_suspend), > > > + .restore = pm_sleep_ptr(i2c_hid_core_pm_restore), > > > }; > > > EXPORT_SYMBOL_GPL(i2c_hid_core_pm); > > > diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h > > > index 2c7b66d5caa0f..1724a435c783a 100644 > > > --- a/drivers/hid/i2c-hid/i2c-hid.h > > > +++ b/drivers/hid/i2c-hid/i2c-hid.h > > > @@ -27,11 +27,13 @@ static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product) > > > * @power_up: do sequencing to power up the device. > > > * @power_down: do sequencing to power down the device. > > > * @shutdown_tail: called at the end of shutdown. > > > + * @restore_sequence: hibernation restore sequence. > > > */ > > > struct i2chid_ops { > > > int (*power_up)(struct i2chid_ops *ops); > > > void (*power_down)(struct i2chid_ops *ops); > > > void (*shutdown_tail)(struct i2chid_ops *ops); > > > + void (*restore_sequence)(struct i2chid_ops *ops); > > > }; > > > int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, > > > -- > > > 2.43.0 > > > >
