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

[Wolfram Sang <wsa@kernel.org>]

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