From 8d70d99ae2c7b361f7555bf9683ca311c9699c94 Mon Sep 17 00:00:00 2001 From: Simone Date: Fri, 31 Jul 2026 11:25:27 +0200 Subject: [PATCH] i2c-imc-skylake: hand the engine back quiesced at shutdown The engine is shared with the firmware, and until now the driver never returned it: at shutdown the BIOS finds whatever command state the last transfer left behind. ws1-mint shows an intermittent poweroff hang isolated to machine_power_off() -> ACPI _S5 with the engine in session use; a BIOS SMI handler wedging on a mid-session engine at the S5 trap is the working hypothesis. Handing the engine back the way the firmware left it is correct hygiene regardless of that outcome. - probe snapshots both pristine CMD words before the driver's first transfer, and fails if they cannot be read: an unreadable snapshot would turn the handback into writing garbage - new .shutdown callback (shutdown and kexec): mark both adapters suspended, drain the possible in-flight transfer under the lock, wait for the engine to go idle, restore the pristine words with GO masked. Best effort: it never blocks the power-off - resume refreshes the snapshot after the firmware-polling re-check passes, since the firmware demonstrably drives the engine across suspend; an unreadable refresh leaves the adapters suspended - the unbind comment now distinguishes unbind (state left as-is, next user is another instance of this driver) from shutdown (next user is the firmware) Version 1.0.4. --- VERSION | 2 +- i2c-imc-skylake.c | 91 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/VERSION b/VERSION index 6d7de6e..ee90284 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.0.2 +1.0.4 diff --git a/i2c-imc-skylake.c b/i2c-imc-skylake.c index 917dc2b..7500c00 100644 --- a/i2c-imc-skylake.c +++ b/i2c-imc-skylake.c @@ -166,6 +166,7 @@ struct imc_smbus { /* needed: both channels share one engine */ struct i2c_adapter adap[2]; /* one per hardware channel */ int io_err; /* sticky config-access error, under lock */ + u32 boot_cmd[ARRAY_SIZE(imc_chans)]; /* pristine CMD words, see probe */ }; struct imc_xfer_state { @@ -664,6 +665,28 @@ static int imc_resume(struct device *dev) return 0; } + /* + * The firmware demonstrably uses the engine across suspend (see + * above), so the boot-time snapshot may no longer be the state it + * expects to find at shutdown. Take a fresh one. If it cannot be + * read the adapters stay suspended, exactly as after a failed + * re-check: an unreadable snapshot would make the shutdown handback + * write garbage. + */ + mutex_lock(&s->lock); + s->io_err = 0; + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) + s->boot_cmd[i] = imc_reg(s, imc_chans[i].cmd); + if (s->io_err) { + mutex_unlock(&s->lock); + dev_err(s->dev, + "cannot refresh the engine snapshot; adapters stay disabled\n"); + return 0; + } + dev_dbg(s->dev, "snapshot refreshed on resume: ch0 0x%08x ch1 0x%08x\n", + s->boot_cmd[0], s->boot_cmd[1]); + mutex_unlock(&s->lock); + for (i = 0; i < ARRAY_SIZE(s->adap); i++) i2c_mark_adapter_resumed(&s->adap[i]); @@ -672,6 +695,44 @@ static int imc_resume(struct device *dev) static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume); +/* + * Hand the engine back to the firmware. After this point the next master is + * the BIOS - its S5/SMI path and the next boot - and it should find the + * command registers as it left them, not holding this driver's last transfer. + * Best effort by design: at shutdown there is nobody left to report an error + * to, so a failure only shortens the sequence, it never blocks the power-off. + * The same callback covers kexec, where the "next boot" starts without a + * firmware pass to reset the engine. + */ +static void imc_pci_shutdown(struct pci_dev *pdev) +{ + struct imc_smbus *s = pci_get_drvdata(pdev); + int i; + + /* probe never completed: the engine was not touched, leave it alone */ + if (!s) + return; + + /* refuse new transfers, then drain the one possibly in flight */ + for (i = 0; i < ARRAY_SIZE(s->adap); i++) + i2c_mark_adapter_suspended(&s->adap[i]); + + mutex_lock(&s->lock); + s->io_err = 0; + imc_wait_engine_idle(s); + + /* + * GO masked for the same reason as in imc_restore_xfer(): put the + * boot-time address, register and TSOD state back without re-arming + * a transaction. + */ + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) + imc_set(s, imc_chans[i].cmd, s->boot_cmd[i] & ~GO_BIT); + mutex_unlock(&s->lock); + + dev_dbg(&pdev->dev, "engine quiesced for shutdown\n"); +} + static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct imc_smbus *s; @@ -703,6 +764,18 @@ static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (ret) return ret; + /* + * Pristine command words: the state the firmware left in the engine at + * boot, captured before this driver's first transfer. They are handed + * back in imc_pci_shutdown(), so an unreadable word would turn that + * handback into writing garbage; fail the probe instead. + */ + for (i = 0; i < ARRAY_SIZE(imc_chans); i++) + s->boot_cmd[i] = imc_reg(s, imc_chans[i].cmd); + if (s->io_err) + return dev_err_probe(&pdev->dev, s->io_err, + "cannot snapshot the engine command words\n"); + /* * Lifetime safety: the I2C core guarantees that smbus_xfer callbacks * are not invoked after i2c_del_adapter() returns. Since we use @@ -762,15 +835,25 @@ MODULE_DEVICE_TABLE(pci, imc_pci_ids); /* * All resources (mutex, i2c adapters) are devm-managed and are released * automatically after probe returns or during device unbinding, so no .remove - * callback is needed. The register state is deliberately left as-is on - * unbind: the engine carries no driver-private latched state that needs - * teardown, and the SMBus controls we drive are side-band registers the rest - * of the kernel does not touch. + * callback is needed. What happens to the register state depends on who + * comes next: + * + * - On unbind with the OS running, it is deliberately left as-is. The + * engine carries no driver-private latched state that needs teardown, the + * SMBus controls we drive are side-band registers the rest of the kernel + * does not touch, and the next user is another instance of this driver, + * which snapshots whatever it finds. + * + * - On shutdown and kexec the next user is the firmware, which shares the + * engine with this driver (see the header comment) and did not sign up + * for finding it in mid-session state. imc_pci_shutdown() hands it back + * quiesced, with the command words as the firmware left them. */ static struct pci_driver imc_driver = { .name = "i2c-imc-skylake", .id_table = imc_pci_ids, .probe = imc_pci_probe, + .shutdown = imc_pci_shutdown, .driver.pm = pm_sleep_ptr(&imc_pm_ops), }; module_pci_driver(imc_driver);