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.
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.
The pending_reboot move is good to have. You really should have
submitted this as a separate patch. Hint as soon as you write
"Also ...", or e.g. "This commit does the following 3 things:
1... 2... 3..." or some such in the commit message that is a hint
that you should split the patch.
I've split out the pending_reboot changes (and also updated the
remove path which you left unchanged) now. While looking into
the remove path, I also found a couple of other probe-failure
related issues so I'll be sending out a patch-set with a > few small fixes (including the pending_reboot move) soon.
Yeah - apologies. It was such a tiny change that I was being lazy.
Thanks you for splitting them out but if you're busy let me know and
I'll happily clean that myself. I had missed the remove part completely.
Thanks
Mark
