[PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Alex Henrie]

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

Signed-off-by: Alex Henrie <alexh@vpitech.com>
---
 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");
+
 /* 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:
+	 * 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
+			 * 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)
-- 
2.34.1

Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Jean Delvare]

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@vpitech.com>
> ---
>  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

Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Alex Henrie]

On Tue, 18 Jan 2022 13:47:05 +0100
Jean Delvare <jdelvare@suse.de> wrote:

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

I need to access a GPIO chip that is connected via SMBus and this was
the best way I could find to make it work. However, today I stumbled
across another solution (see bottom of email).

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

I looked for a BIOS option to make it behave better, but found none.

> Furthermore, please run ./scripts/checkpatch.pl on your patches and fix
> every reported issue before you post them.

Thanks for the tip. I'm a bit new to kernel development.

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

This is the convention that is used in drivers/hid/hid-apple.c, which
was the last driver I worked on. I didn't realize that it's not
standardized across the kernel.

> > +	 * If BIOS AML code tries to touches the OpRegion we have two options:
> 
> Spelling: touches -> touch

Good catch.

> > +			/*
> > +			 * 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?

I think Hector's patch to share the SMBus is a great idea; my patch was
meant to complement his, not replace it. The problem is that when I
tried Hector's patch, I got the message "BIOS left SMBus locked". So I
didn't want to let the BIOS touch the SMBus at all because once it had
the lock, it seemed to never let go. However, today I tried v2 of
Hector's patch [1] instead of v3 [2], and v2 worked perfectly! My guess
is that despite the text of the error message, it's Linux that's
leaving the SMBus locked, not the BIOS.

The only difference between v2 and v3 is that v2 called
outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv)) at the end of i801_access.
Hector, can you clarify why you removed that call in v3?

-Alex

[1] https://lore.kernel.org/linux-i2c/20210519091707.7248-1-marcan@marcan.st/
[2] https://lore.kernel.org/linux-i2c/20210626054113.246309-1-marcan@marcan.st/

Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Hector Martin]

On 20/01/2022 01.49, Alex Henrie wrote:
> I think Hector's patch to share the SMBus is a great idea; my patch was
> meant to complement his, not replace it. The problem is that when I
> tried Hector's patch, I got the message "BIOS left SMBus locked". So I
> didn't want to let the BIOS touch the SMBus at all because once it had
> the lock, it seemed to never let go. However, today I tried v2 of
> Hector's patch [1] instead of v3 [2], and v2 worked perfectly! My guess
> is that despite the text of the error message, it's Linux that's
> leaving the SMBus locked, not the BIOS.
> 
> The only difference between v2 and v3 is that v2 called
> outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv)) at the end of i801_access.
> Hector, can you clarify why you removed that call in v3?

Well that solves the mystery.

That line was split off and submitted separately in another patch, as it
fixes the immediate breakage that I was trying to address with my patch,
namely that *linux* left the SMBus locked and that broke BIOS accesses.
 The full patch implements proper sharing logic; that line alone is
enough to unbork the backlight on iMacs (among other things) which was
the immediate regression I had encountered. The rest of the patch breaks
horribly without that line, as you'd expect, since it's trying to work
around a broken BIOS when Linux itself is broken.

That patch was posted in June 2021 and CC'd stable [1], and was merged
into the stable 5.10 and 5.12 trees as well as released in 5.13. So, you
must be running an old kernel :-).

[1]
https://lore.kernel.org/linux-i2c/cefbeb76-5f7f-036b-fa0e-1e339d261c35@gmail.com/

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Alex Henrie]

On Thu, 20 Jan 2022 02:32:02 +0900
Hector Martin <marcan@marcan.st> wrote:

> On 20/01/2022 01.49, Alex Henrie wrote:
> > I think Hector's patch to share the SMBus is a great idea; my patch was
> > meant to complement his, not replace it. The problem is that when I
> > tried Hector's patch, I got the message "BIOS left SMBus locked". So I
> > didn't want to let the BIOS touch the SMBus at all because once it had
> > the lock, it seemed to never let go. However, today I tried v2 of
> > Hector's patch [1] instead of v3 [2], and v2 worked perfectly! My guess
> > is that despite the text of the error message, it's Linux that's
> > leaving the SMBus locked, not the BIOS.
> > 
> > The only difference between v2 and v3 is that v2 called
> > outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv)) at the end of i801_access.
> > Hector, can you clarify why you removed that call in v3?
> 
> Well that solves the mystery.
> 
> That line was split off and submitted separately in another patch, as it
> fixes the immediate breakage that I was trying to address with my patch,
> namely that *linux* left the SMBus locked and that broke BIOS accesses.
>  The full patch implements proper sharing logic; that line alone is
> enough to unbork the backlight on iMacs (among other things) which was
> the immediate regression I had encountered. The rest of the patch breaks
> horribly without that line, as you'd expect, since it's trying to work
> around a broken BIOS when Linux itself is broken.
> 
> That patch was posted in June 2021 and CC'd stable [1], and was merged
> into the stable 5.10 and 5.12 trees as well as released in 5.13. So, you
> must be running an old kernel :-).
> 
> [1]
> https://lore.kernel.org/linux-i2c/cefbeb76-5f7f-036b-fa0e-1e339d261c35@gmail.com/

Excellent! Thank you so much Hector. Indeed, this system is currently
using a custom-built 5.4 kernel.

Jean, please feel free to disregard my patch and to commit Hector's
with "Tested-by: Alex Henrie <alexh@vpitech.com>".

-Alex

Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Wolfram Sang]

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


> Jean, please feel free to disregard my patch and to commit Hector's
> with "Tested-by: Alex Henrie <alexh@vpitech.com>".

Please respnd to the mail directly with the Tested-by tag. We have
tooling which collects tags for us per patch. Makes a maintainers life a
lot easier.


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

Re: [External] Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Alex Henrie]

On Wed, 19 Jan 2022 19:43:00 +0100
Wolfram Sang <wsa@kernel.org> wrote:

> 
> > Jean, please feel free to disregard my patch and to commit Hector's
> > with "Tested-by: Alex Henrie <alexh@vpitech.com>".
> 
> Please respnd to the mail directly with the Tested-by tag. We have
> tooling which collects tags for us per patch. Makes a maintainers life a
> lot easier.

I wasn't subscribed to the mailing list when that patch was sent, and
without my own copy of the patch email I don't have an easy way to
reply with the correct In-Reply-To header. Is a reply with a correct
Subject header enough for the automated tools?

-Alex

Re: [External] Re: [PATCH] i2c: i801: Add module parameter to inhibit BIOS access

[Wolfram Sang]

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


> I wasn't subscribed to the mailing list when that patch was sent, and
> without my own copy of the patch email I don't have an easy way to
> reply with the correct In-Reply-To header. Is a reply with a correct
> Subject header enough for the automated tools?

I see. I'll bounce the message to you.


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