On Tue, 18 Jan 2022 13:47:05 +0100 Jean Delvare <jdelvare@xxxxxxx> 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@xxxxxxxxx/ [2] https://lore.kernel.org/linux-i2c/20210626054113.246309-1-marcan@xxxxxxxxx/
