Re: [PATCH v3] i2c: i801: Safely share SMBus with BIOS/ACPI

On Sat, Jun 26, 2021 at 02:41:13PM +0900, Hector Martin wrote:

The i801 controller provides a locking mechanism that the OS is supposed to use to safely share the SMBus with ACPI AML or other firmware.

Previously, Linux attempted to get out of the way of ACPI AML entirely, but left the bus locked if it used it before the first AML access. This causes AML implementations that *do* attempt to safely share the bus to time out if Linux uses it first; notably, this regressed ACPI video backlight controls on 2015 iMacs after 01590f361e started instantiating SPD EEPROMs on boot.

Commit 065b6211a8 fixed the immediate problem of leaving the bus locked, but we can do better. The controller does have a proper locking mechanism, so let's use it as intended. Since we can't rely on the BIOS doing this properly, we implement the following logic:

• If ACPI AML uses the bus at all, we make a note and disable power management. The latter matches already existing behavior.
• When we want to use the bus, we attempt to lock it first. If the locking attempt times out, *and* ACPI hasn't tried to use the bus at all yet, we cautiously go ahead and assume the BIOS forgot to unlock the bus after boot. This preserves existing behavior.
• We always unlock the bus after a transfer.
• If ACPI AML tries to use the bus (except trying to lock it) while we're in the middle of a transfer, or after we've determined locking is broken, we know we cannot safely share the bus and give up.

Upon first usage of SMBus by ACPI AML, if nothing has gone horribly wrong so far, users will see:

i801_smbus 0000:00:1f.4: SMBus controller is shared with ACPI AML. This seems safe so far.

If locking the SMBus times out, users will see:

i801_smbus 0000:00:1f.4: BIOS left SMBus locked

And if ACPI AML tries to use the bus concurrently with Linux, or it previously used the bus and we failed to subsequently lock it as above, the driver will give up and users will get:

i801_smbus 0000:00:1f.4: BIOS uses SMBus unsafely i801_smbus 0000:00:1f.4: Driver SMBus register access inhibited

This fixes the regression introduced by 01590f361e, and further allows safely sharing the SMBus on 2015 iMacs. Tested by running `i2cdump` in a loop while changing backlight levels via the ACPI video device.

Fixes: 01590f361e ("i2c: i801: Instantiate SPD EEPROMs automatically") Cc: stable@vger.kernel.org Signed-off-by: Hector Martin marcan@marcan.st

Jean, Heiner, what do we do with this topic?

---

drivers/i2c/busses/i2c-i801.c | 96 ++++++++++++++++++++++++++++------- 1 file changed, 79 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 04a1e38f2a6f..03be6310d6d7 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -287,11 +287,18 @@ struct i801_priv { #endif struct platform_device *tco_pdev;

• /* BIOS left the controller marked busy. */
• bool inuse_stuck; /*

• * If set to true the host controller registers are reserved for
  

• * ACPI AML use. Protected by acpi_lock.
  

• * If set to true, ACPI AML uses the host controller registers.
  

• * Protected by acpi_lock.
  

  */

• bool acpi_reserved;

• bool acpi_usage;
• /*

• * If set to true, ACPI AML uses the host controller registers in an
  

• * unsafe way. Protected by acpi_lock.
  

• */
  

• bool acpi_unsafe; struct mutex acpi_lock;

}; @@ -854,10 +861,37 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, int hwpec; int block = 0; int ret = 0, xact = 0;

• int timeout = 0; struct i801_priv *priv = i2c_get_adapdata(adap);

• /*

• * The controller provides a bit that implements a mutex mechanism
  

• * between users of the bus. First, try to lock the hardware mutex.
  

• * If this doesn't work, we give up trying to do this, but then
  

• * bail if ACPI uses SMBus at all.
  

• */
  

• if (!priv->inuse_stuck) {

• while (inb_p(SMBHSTSTS(priv)) & SMBHSTSTS_INUSE_STS) {
  

• 	if (++timeout >= MAX_RETRIES) {
  

• 		dev_warn(&priv->pci_dev->dev,
  

• 			 "BIOS left SMBus locked\n");
  

• 		priv->inuse_stuck = true;
  

• 		break;
  

• 	}
  

• 	usleep_range(250, 500);
  

• }
  

• }
• mutex_lock(&priv->acpi_lock);

• if (priv->acpi_reserved) {

• if (priv->acpi_usage && priv->inuse_stuck && !priv->acpi_unsafe) {

• priv->acpi_unsafe = true;
  

• dev_warn(&priv->pci_dev->dev, "BIOS uses SMBus unsafely\n");
  

• dev_warn(&priv->pci_dev->dev,
  

• 	 "Driver SMBus register access inhibited\n");
  

• }
• if (priv->acpi_unsafe) { mutex_unlock(&priv->acpi_lock); return -EBUSY; }

@@ -1639,6 +1673,16 @@ static bool i801_acpi_is_smbus_ioport(const struct i801_priv *priv, address <= pci_resource_end(priv->pci_dev, SMBBAR); } +static acpi_status +i801_acpi_do_access(u32 function, acpi_physical_address address,

• 		u32 bits, u64 *value)
  

+{

• if ((function & ACPI_IO_MASK) == ACPI_READ)

• return acpi_os_read_port(address, (u32 *)value, bits);
  

• else

• return acpi_os_write_port(address, (u32)*value, bits);
  

+}

static acpi_status i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, u64 *value, void *handler_context, void *region_context) @@ -1648,17 +1692,38 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, acpi_status status; /*

• * Once BIOS AML code touches the OpRegion we warn and inhibit any
  

• * further access from the driver itself. This device is now owned
  

• * by the system firmware.
  

• * Non-i801 accesses pass through.
  

  */

• mutex_lock(&priv->acpi_lock);

• if (!i801_acpi_is_smbus_ioport(priv, address))

• return i801_acpi_do_access(function, address, bits, value);
  

• if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {

• priv->acpi_reserved = true;
  

• if (!mutex_trylock(&priv->acpi_lock)) {

• mutex_lock(&priv->acpi_lock);
  

• /*
  

•  * This better be a read of the status register to acquire
  

•  * the lock...
  

•  */
  

• if (!priv->acpi_unsafe &&
  

• 	!(address == SMBHSTSTS(priv) &&
  

• 	 (function & ACPI_IO_MASK) == ACPI_READ)) {
  

• 	/*
  

• 	 * Uh-oh, ACPI AML is trying to do something with the
  

• 	 * controller without locking it properly.
  

• 	 */
  

• 	priv->acpi_unsafe = true;
  

• 	dev_warn(&pdev->dev, "BIOS uses SMBus unsafely\n");
  

• 	dev_warn(&pdev->dev,
  

• 		 "Driver SMBus register access inhibited\n");
  

• }
  

• }

• dev_warn(&pdev->dev, "BIOS is accessing SMBus registers\n");
  

• dev_warn(&pdev->dev, "Driver SMBus register access inhibited\n");
  

• if (!priv->acpi_usage) {

• priv->acpi_usage = true;
  

• if (!priv->acpi_unsafe)
  

• 	dev_info(&pdev->dev,
  

• 		 "SMBus controller is shared with ACPI AML. This seems safe so far.\n");
  

/* * BIOS is accessing the host controller so prevent it from @@ -1667,10 +1732,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, pm_runtime_get_sync(&pdev->dev); }

• if ((function & ACPI_IO_MASK) == ACPI_READ)

• status = acpi_os_read_port(address, (u32 *)value, bits);
  

• else

• status = acpi_os_write_port(address, (u32)*value, bits);
  

• status = i801_acpi_do_access(function, address, bits, value);

mutex_unlock(&priv->acpi_lock); @@ -1706,7 +1768,7 @@ static void i801_acpi_remove(struct i801_priv *priv) ACPI_ADR_SPACE_SYSTEM_IO, i801_acpi_io_handler); mutex_lock(&priv->acpi_lock);

• if (priv->acpi_reserved)

• if (priv->acpi_usage) pm_runtime_put(&priv->pci_dev->dev); mutex_unlock(&priv->acpi_lock);

}

2.32.0