Re: [PATCH v7] Introduce support for Systems Management Driver over WMI for Dell Systems
Em Thu, 29 Oct 2020 09:01:14 +0100
Mauro Carvalho Chehab escreveu:
> Em Tue, 27 Oct 2020 19:19:44 +0530
> Divya Bharathi escreveu:
>
> > The Dell WMI Systems Management Driver provides a sysfs
> > interface for systems management to enable BIOS configuration
> > capability on certain Dell Systems.
> >
> > This driver allows user to configure Dell systems with a
> > uniform common interface. To facilitate this, the patch
> > introduces a generic way for driver to be able to create
> > configurable BIOS Attributes available in Setup (F2) screen.
> >
> > Cc: Hans de Goede
> > Cc: Andy Shevchenko
> > Cc: mark gross
> >
> > Co-developed-by: Mario Limonciello
> > Signed-off-by: Mario Limonciello
> > Co-developed-by: Prasanth KSR
> > Signed-off-by: Prasanth KSR
> > Signed-off-by: Divya Bharathi
> > ---
>
>
> > +What: /sys/class/firmware-attributes/*/authentication/
> > +Date: February 2021
> > +KernelVersion: 5.11
> > +Contact: Divya Bharathi ,
> > + Mario Limonciello ,
> > + Prasanth KSR
> > +
> > + Devices support various authentication mechanisms which can be
> > exposed
> > + as a separate configuration object.
> > +
> > + For example a "BIOS Admin" password and "System" Password can
> > be set,
> > + reset or cleared using these attributes.
> > + - An "Admin" password is used for preventing modification to
> > the BIOS
> > + settings.
> > + - A "System" password is required to boot a machine.
> > +
>
> This is adding a new warning:
>
> $ ./scripts/get_abi.pl validate
> Warning: file
> Documentation/ABI/testing/sysfs-class-firmware-attributes#172:
> What '/sys/class/firmware-attributes/*/authentication/' doesn't
> have a description
>
> Because you forgot to add a Description: tag.
>
> Feel free to either add the enclosed tag to the tree which added this into
> linux-next, or to fold id with the original patch.
>
> Thanks,
> Mauro
>
> ABI: docs: sysfs-class-firmware-attributes: add a missing tag
>
> The Description: tag is missing, causing this warning with
> scripts/get_abi.pl:
>
> Warning: file
> Documentation/ABI/testing/sysfs-class-firmware-attributes#172:
>What '/sys/class/firmware-attributes/*/authentication/'
> doesn't have a description
>
> Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management
> Driver over WMI for Dell Systems")
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> index 04a15c72e883..ea1837f1f3c2 100644
> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -113,7 +113,7 @@ KernelVersion:5.11
> Contact: Divya Bharathi ,
> Mario Limonciello ,
> Prasanth KSR
> -
> +Description:
> Devices support various authentication mechanisms which can be
> exposed
> as a separate configuration object.
>
>
There are a few other warnings produced by it, when generating the
ABI output, so, I sent a new patch covering all warnings.
If you want to test the ABI file generation, the patchset is at:
https://git.linuxtv.org/mchehab/experimental.git/log/?h=abi_patches_v7
You can easily build just the sysfs firmware attribute class ABI with
something like this:
$ mkdir -p fodir && cp
Documentation/ABI/testing/sysfs-class-firmware-attributes fodir/ &&
./scripts/get_abi.pl rest -dir fodir/ --rst-source >Documentation/foo/abi.rst
&& make SPHINXDIRS=foo htmldocs
You can also see it at:
http://www.infradead.org/~mchehab/kernel_docs/foo/abi.html
Thanks,
Mauro
Re: [PATCH v7] Introduce support for Systems Management Driver over WMI for Dell Systems
Em Tue, 27 Oct 2020 19:19:44 +0530
Divya Bharathi escreveu:
> The Dell WMI Systems Management Driver provides a sysfs
> interface for systems management to enable BIOS configuration
> capability on certain Dell Systems.
>
> This driver allows user to configure Dell systems with a
> uniform common interface. To facilitate this, the patch
> introduces a generic way for driver to be able to create
> configurable BIOS Attributes available in Setup (F2) screen.
>
> Cc: Hans de Goede
> Cc: Andy Shevchenko
> Cc: mark gross
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Co-developed-by: Prasanth KSR
> Signed-off-by: Prasanth KSR
> Signed-off-by: Divya Bharathi
> ---
> +What:/sys/class/firmware-attributes/*/authentication/
> +Date:February 2021
> +KernelVersion: 5.11
> +Contact: Divya Bharathi ,
> + Mario Limonciello ,
> + Prasanth KSR
> +
> + Devices support various authentication mechanisms which can be
> exposed
> + as a separate configuration object.
> +
> + For example a "BIOS Admin" password and "System" Password can
> be set,
> + reset or cleared using these attributes.
> + - An "Admin" password is used for preventing modification to
> the BIOS
> + settings.
> + - A "System" password is required to boot a machine.
> +
This is adding a new warning:
$ ./scripts/get_abi.pl validate
Warning: file
Documentation/ABI/testing/sysfs-class-firmware-attributes#172:
What '/sys/class/firmware-attributes/*/authentication/' doesn't
have a description
Because you forgot to add a Description: tag.
Feel free to either add the enclosed tag to the tree which added this into
linux-next, or to fold id with the original patch.
Thanks,
Mauro
ABI: docs: sysfs-class-firmware-attributes: add a missing tag
The Description: tag is missing, causing this warning with
scripts/get_abi.pl:
Warning: file
Documentation/ABI/testing/sysfs-class-firmware-attributes#172:
What '/sys/class/firmware-attributes/*/authentication/'
doesn't have a description
Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems Management
Driver over WMI for Dell Systems")
Signed-off-by: Mauro Carvalho Chehab
diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes
b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 04a15c72e883..ea1837f1f3c2 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -113,7 +113,7 @@ KernelVersion: 5.11
Contact: Divya Bharathi ,
Mario Limonciello ,
Prasanth KSR
-
+Description:
Devices support various authentication mechanisms which can be
exposed
as a separate configuration object.
Re: [PATCH v7] Introduce support for Systems Management Driver over WMI for Dell Systems
Hi,
On 10/27/20 2:49 PM, Divya Bharathi wrote:
> The Dell WMI Systems Management Driver provides a sysfs
> interface for systems management to enable BIOS configuration
> capability on certain Dell Systems.
>
> This driver allows user to configure Dell systems with a
> uniform common interface. To facilitate this, the patch
> introduces a generic way for driver to be able to create
> configurable BIOS Attributes available in Setup (F2) screen.
>
> Cc: Hans de Goede
> Cc: Andy Shevchenko
> Cc: mark gross
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Co-developed-by: Prasanth KSR
> Signed-off-by: Prasanth KSR
> Signed-off-by: Divya Bharathi
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note I've made 2 small changes while applying this:
1. I've added a missing "This attribute is mandatory." to the docs for
the /sys/class/firmware-attributes/*/authentication/*/is_enabled
sysfs file.
2. You did a direct return after the ACPI object type check, in
the various current_value_show functions:
if (obj->package.elements[CURRENT_VAL].type != ACPI_TYPE_STRING)
return -EINVAL;
This leaks the object returned by get_wmiobj_pointer() I've added the
missing kfree() call like this:
--- a/drivers/platform/x86/dell-wmi-sysman/enum-attributes.c
+++ b/drivers/platform/x86/dell-wmi-sysman/enum-attributes.c
@@ -23,8 +23,10 @@ static ssize_t current_value_show(struct kobject *kobj,
struct kobj_attribute *a
obj = get_wmiobj_pointer(instance_id,
DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
if (!obj)
return -EIO;
- if (obj->package.elements[CURRENT_VAL].type != ACPI_TYPE_STRING)
+ if (obj->package.elements[CURRENT_VAL].type != ACPI_TYPE_STRING) {
+ kfree(obj);
return -EINVAL;
+ }
ret = snprintf(buf, PAGE_SIZE, "%s\n",
obj->package.elements[CURRENT_VAL].string.pointer);
kfree(obj);
return ret;
And I did the same for all the other cases.
Once I've run some tests on my review-hans branch the patches there will
be added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> ---
>
> Changes from v6 to v7:
> - Set current_value to rw only for root user
> - Correct spacing in sysfs documentation
> - Make some more attributes mandatory
> - Remove cap_sys_admin check
> - add extra check for -ENOMEM for attribute_property_store
> - Add dereferencing check for data types read
> - Adjust passobj bounds check by 1
> - Add extra check for -ENOMEM in new_password_store
> - Clear current password properly
> - Simplify reset_bios_store
> - Adjust helper functions per Hans' suggestions
> - Correct integer handling abusing ACPI core
>
> Changes from v5 to v6:
> - Fixed Mutex bug in set_bios_defaults
> - Adjust wrapping for enum-attributes.c description
> - Adjust wrapping for int-attributes.c comparison
> - Make current_password mandatory for mechanism == password
> - Move strlen of admin check into populate_security_password
> - Use correct device for dev_err calls (bios_attr_wdev)
> - Remove type from structs and add type-string directly in show functions
> - Remove unneeded macro definition (make function static)
> - Make calls to get_*_instance_id error handling match kernel style
> - Adjust -AE_ERROR to -EIO
> - Adjust validate_integer_input to propagate errors
> - Make current_password_store return -EIO for an unknown type
> - Don't store current_value, read dynamically every time
> - Move kobject_uevents to class_dev
> - Update all kernel doc comments with standard termination '*/'
> - Correct dereferences in alloc_*_data()
> - Make value_modifier and value_modifier_count local
> - Remove is_enabled from po
> - Fix current_password_store const char* handling
> - Propagate error value from get_po_instance_id() in is_enabled_show()
> - Adjust validate_str_input
> - Cleanups to reset_bios_store to use sysfs_match_string
> - Adjust set_bios_defaults
> - Rename create_reset_bios
> - strlcpy_attr handling
> - Check number of kset_unregister calls in error path
> - Make teardown order match error order
> - Pass password into calculate_security_buffer
> - Correct handling of const char * values in macros
> - Add new function for populating string buffers
> - Switch strncasecmp to strcasecmp
>+ This wasn't strictly necessary (the firmware rejected invalid strings)
>+ However, by doing it in the kernel we can send a more sensbile error
> -EINVAL
>
> Changes from v4 to v5:
> - Correct Kconfig description to not use "WMI" twice.
> - s/is_authentication_set/is_enabled/
> - Specify that all attributes are optional and take UTF8
> - Change authentication type to "role" and "mechanism"
> - Explain wha
