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

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

[Hector Martin]

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>
---
 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

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

[Wolfram Sang]

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

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jean Delvare]

On Mon, 29 Nov 2021 10:00:55 +0100, Wolfram Sang wrote:
> 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?

I like the idea, I need to give it a try and review the code.

-- 
Jean Delvare
SUSE L3 Support

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

[Alex Henrie]

Tested-by: Alex Henrie <alexh@vpitech.com>

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

[Ettore Chimenti]

Tested on a SECO SBC-B68 and a UDOO X86.
The BIOS AML code queries the Embedded Controller over SMBus, 
respecting the hardware semaphore implementation.

I get this line on kernel log and everything works as expected.
[    7.270172] i801_smbus 0000:00:1f.3: SMBus controller is shared with ACPI AML. This seems safe so far.

Tested with continous use of i2c-tools (i2cdump) with temperature reads
in thermal_zone (that triggers AML code).

Tested-by: Ettore Chimenti <ek5.chimenti@gmail.com>