Hi Alex,
On Tue, 11 Jan 2022 16:31:51 -0700, Alex Henrie wrote:
> This parameter can only be set before the module is loaded (e.g. by
> passing i2c_i801.block_acpi=1 on the kernel command line).
Before I consider applying this, you'll have to provide a rationale of
why this is needed. Preventing the BIOS from accessing the SMBus is
pretty dangerous, and I can't really think of a situation where you
would want to do that.
Plus, if there's really a reason for doing that, I'd rather have it
implemented as an option in the BIOS itself, than in the kernel driver.
Furthermore, please run ./scripts/checkpatch.pl on your patches and fix
every reported issue before you post them.
> Signed-off-by: Alex Henrie <alexh@xxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-i801.c | 63 +++++++++++++++++++++++++++--------
> 1 file changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 41446f9cc52d..615fd1185b61 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -288,6 +288,11 @@ struct i801_priv {
> * ACPI AML use. Protected by acpi_lock.
> */
> bool acpi_reserved;
> + /*
> + * If set to true ACPI AML tried to use SMBus but block_acpi was
> + * set. Protected by acpi_lock.
> + */
> + bool acpi_blocked;
> struct mutex acpi_lock;
> };
>
> @@ -320,6 +325,11 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
> "\t\t 0x10 don't use interrupts\n"
> "\t\t 0x20 disable SMBus Host Notify ");
>
> +static bool block_acpi;
> +module_param(block_acpi, bool, S_IRUGO);
> +MODULE_PARM_DESC(block_acpi, "Prevent ACPI AML from accessing SMBus. "
> + "[0] = allow ACPI access, 1 = deny ACPI access");
I've not seen the square brackets convention for marking the default
value used anywhere else in the kernel. For consistency, please instead
add "(default)" after the default setting.
> +
> /* Make sure the SMBus host is ready to start transmitting.
> Return 0 if it is, -EBUSY if it is not. */
> static int i801_check_pre(struct i801_priv *priv)
> @@ -1616,23 +1626,48 @@ 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.
> + * If BIOS AML code tries to touches the OpRegion we have two options:
Spelling: touches -> touch
> + * Warn and inhibit any further access from the driver, or warn and
> + * inhibit all access from the BIOS.
> */
> mutex_lock(&priv->acpi_lock);
>
> - if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
> - priv->acpi_reserved = true;
> -
> - dev_warn(&pdev->dev, "BIOS is accessing SMBus registers\n");
> - dev_warn(&pdev->dev, "Driver SMBus register access inhibited\n");
> -
> - /*
> - * BIOS is accessing the host controller so prevent it from
> - * suspending automatically from now on.
> - */
> - pm_runtime_get_sync(&pdev->dev);
> + if (i801_acpi_is_smbus_ioport(priv, address)) {
> + if (block_acpi) {
> + /*
> + * Refuse to allow the BIOS to use SMBus. SMBus does
> + * have a lock bit in the status register that in theory
> + * can be used to safely share the SMBus between the
> + * BIOS and the kernel, but some badly behaved BIOS
> + * implementations don't use it. In that case, the only
It's not really fair to blame the BIOS, considering that the driver
doesn't use it (yet) either. A patch was proposed months ago actually,
reviewing it is still on my to-do list. Could that be an alternative to
your patch?
> + * way to ensure continued safe access from the driver
> + * is to cripple the BIOS.
> + */
> + if (!priv->acpi_blocked) {
> + dev_warn(&pdev->dev,
> + "BIOS tried to access SMBus registers\n");
> + dev_warn(&pdev->dev,
> + "BIOS SMBus register access inhibited\n");
> + priv->acpi_blocked = true;
> + }
> + mutex_unlock(&priv->acpi_lock);
> + return -EPERM;
> + }
> + if (!priv->acpi_reserved) {
> + /* This device is now owned by the system firmware. */
> + priv->acpi_reserved = true;
> +
> + dev_warn(&pdev->dev,
> + "BIOS is accessing SMBus registers\n");
> + dev_warn(&pdev->dev,
> + "Driver SMBus register access inhibited\n");
> +
> + /*
> + * BIOS is accessing the host controller so prevent it
> + * from suspending automatically from now on.
> + */
> + pm_runtime_get_sync(&pdev->dev);
> + }
> }
>
> if ((function & ACPI_IO_MASK) == ACPI_READ)
--
Jean Delvare
SUSE L3 Support
