Re: [External]Re: [External]Re: [PATCH] platform/x86: think-lmi: add debug_cmd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans

On 2021-07-29 1:34 p.m., Hans de Goede wrote:
Hi Mark,

On 7/19/21 2:46 PM, Mark Pearson wrote:
Thanks for the review Hans

On 2021-07-17 9:59 a.m., Hans de Goede wrote:
Hi Mark,

On 7/16/21 1:08 AM, Mark Pearson wrote:
Many Lenovo BIOS's support the ability to send a debug command
which is useful for debugging and testing unreleased or early
features. Adding support for this feature.

Also moved the pending_reboot node under attributes dir where
it should correctly live. Got that wrong in my last commit and
realised as I was updating the documentation for debug_cmd

Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>

Thank you for the patch, I'm not entirely sure if we want this in
its current form though, isn't this new debug_cmd file not
something which would be better under debugfs ?  Or maybe have it
only show up if a module parameter is passed to enable it ?

Note that both have implications wrt secureboot. debugfs is only writeable when secureboot is disabled; and ATM module options
are not protected by secureboot, but at least in Fedora we would
actually like to change that in the future.

Anyways, lets discuss this a bit further and then merge
something for this later.

I wasn't sure about debugfs but did consider it. It seemed to be
adding a whole extra layer. I'm happy to do that if that's what is
recommended but not having it available with secureboot could be
annoying for users.

I agree that adding debugfs support just for a single file seems a
bit overkill.

This feature is mostly used internally (which is fine) but we have
had occasions where we've used it with customers; and with the WMI
interface it's usually the enterprise customers who do have secure
boot enabled. I'd like to avoid that limitation if possible.

So given that using debugfs seems a bit overkill and it has issues
with selinux I think that it might be best to hide this file behind a
module option (and mention that in the docs, or maybe not document it
at all?).

At least for now kernel commandline options an be set without
breaking secureboot and even if secureboot ever starts verifying the
cmdline then we can always use a /etc/modprobe.d/*.conf file to
specify the option instead of passing it through the kernel cmdline.

Would using a module option to enable the debug_cmd file work for you
?


That sounds very sensible to me - totally happy to do that.
I'll get that implemented when I'm back from PTO and send as an update.

Thanks!
Mark




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux