| Message ID | 20220111233151.8484-1-alexh@vpitech.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | i2c: i801: Add module parameter to inhibit BIOS access | expand |
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)
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/
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/
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
> 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.
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
> 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.
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)
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(-)