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

[Mark Pearson <markpearson@lenovo.com>]

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