RE: [PATCH v5] Introduce support for Systems Management Driver over WMI for Dell Systems
> > Hmm, checkpatch is saying: > > WARNING: Missing Signed-off-by: line by nominal patch author 'Divya > Bharathi ' > > I assume the dell address is the one you want to use ? > > If so try setting the following in ~/.gitconfig: > > [user] > email = divya.bhara...@dell.com > > And then do: > > git commit --amend --reset-author > > To change the author which git is using for the patch. > > > --- > > Thanks, I used --reset-author while creating patch-v6. And I had added my dell address in gitconfig for previous patches as well. Also, local checkpatch.pl script did not throw me this warning. Hope, next patch will not hit this warning. Regards, Divya
RE: [PATCH v5] Introduce support for Systems Management Driver over WMI for Dell Systems
Sorry for another mail on same patch. I missed to add one response on
previous comments
> > +/**
> > + * init_bios_attributes() - Initialize all attributes for a type
> > + * @attr_type: The attribute type to initialize
> > + * @guid: The WMI GUID associated with this type to initialize
> > + *
> > + * Initialiaze all 4 types of attributes enumeration, integer, string and
> password object.
> > + * Populates each attrbute typ's respective properties under sysfs files
> > + **/
> > +static int init_bios_attributes(int attr_type, const char *guid)
> > +{
> > + struct kobject *attr_name_kobj; //individual attribute names
> > + union acpi_object *obj = NULL;
> > + union acpi_object *elements;
> > + struct kset *tmp_set;
> > +
> > + /* instance_id needs to be reset for each type GUID
> > +* also, instance IDs are unique within GUID but not across
> > +*/
> > + int instance_id = 0;
> > + int retval = 0;
> > +
> > + retval = alloc_attributes_data(attr_type);
> > + if (retval)
> > + return retval;
> > + /* need to use specific instance_id and guid combination to get right
> data */
> > + obj = get_wmiobj_pointer(instance_id, guid);
> > + if (!obj)
> > + return -ENODEV;
> > + elements = obj->package.elements;
> > +
> > + mutex_lock(&wmi_priv.mutex);
> > + while (elements) {
> > + /* sanity checking */
> > + if (strlen(elements[ATTR_NAME].string.pointer) == 0) {
> > + pr_debug("empty attribute found\n");
> > + goto nextobj;
> > + }
> > + if (attr_type == PO)
> > + tmp_set = wmi_priv.authentication_dir_kset;
> > + else
> > + tmp_set = wmi_priv.main_dir_kset;
> > +
> > + if (kset_find_obj(tmp_set,
> elements[ATTR_NAME].string.pointer)) {
> > + pr_debug("duplicate attribute name found - %s\n",
> > + elements[ATTR_NAME].string.pointer);
> > + goto nextobj;
> > + }
> > +
> > + /* build attribute */
> > + attr_name_kobj = kzalloc(sizeof(*attr_name_kobj),
> GFP_KERNEL);
> > + if (!attr_name_kobj)
> > + goto err_attr_init;
> > +
> > + attr_name_kobj->kset = tmp_set;
> > +
> > + retval = kobject_init_and_add(attr_name_kobj,
> &attr_name_ktype, NULL, "%s",
> > +
> elements[ATTR_NAME].string.pointer);
> > + if (retval) {
> > + kobject_put(attr_name_kobj);
> > + goto err_attr_init;
> > + }
> > +
> > + /* enumerate all of this attribute */
> > + switch (attr_type) {
> > + case ENUM:
> > + retval = populate_enum_data(elements, instance_id,
> attr_name_kobj);
> > + break;
> > + case INT:
> > + retval = populate_int_data(elements, instance_id,
> attr_name_kobj);
> > + break;
> > + case STR:
> > + retval = populate_str_data(elements, instance_id,
> attr_name_kobj);
> > + break;
> > + case PO:
> > + retval = populate_po_data(elements, instance_id,
> attr_name_kobj);
> > + break;
> > + default:
> > + break;
> > + }
>
> The show functions don't take the mutex and can be called as soon as
> kobject_init_and_add() is called, so it would be better to first populate the
> data for the current instance_id and only then call kobject_init_and_add()
>
Populate functions called here for each type of attribute uses
attribute_koject which helps in attribute group cleanup.
Regards,
Divya
RE: [PATCH] MAINTAINERS: rectify DELL WMI SYSMAN DRIVERS section
>
> Commit e8a60aa7404b ("platform/x86: Introduce support for Systems
> Management Driver over WMI for Dell Systems") added a new section DELL
> WMI SYSMAN DRIVERS in MAINTAINERS, but slipped in a typo.
>
> Hence, ./scripts/get_maintainer.pl --self-test=patterns complains:
>
> warning: no file matchesF:drivers/platform/x86/dell-wmi-syman/*
>
> Point the file entry to the right location and add an entry for its
> Documentation while at it.
>
> Signed-off-by: Lukas Bulwahn
Acked-by: Divya Bharathi
RE: [PATCH v2] Introduce support for Systems Management Driver over WMI for Dell Systems
> >
> > >
> > > > +
> > > > +/* kept variable names same as in sysfs file name for sysfs_show
> > > > +macro
> > > definition */
> > > > +struct enumeration_data {
> > > > + char display_name_language_code[MAX_BUFF];
> > > > + char attribute_name[MAX_BUFF];
> > > > + char display_name[MAX_BUFF];
> > > > + char default_value[MAX_BUFF];
> > > > + char current_value[MAX_BUFF];
> > > > + char modifier[MAX_BUFF];
> > > > + int value_modifier_count;
> > > > + char value_modifier[MAX_BUFF];
> > > > + int possible_value_count;
> > > > + char possible_value[MAX_BUFF];
> > > > + char type[MAX_BUFF];
> > > > +};
> > > > +
> > > > +static struct enumeration_data *enumeration_data; static int
> > > > +enumeration_instances_count;
> > >
> > > Please store these 2 in the global wmi_priv data.
> > >
> > > Also there is a lot of overlap between structs like struct
> > > enumeration_data, struct integer_data, etc.
> > >
> > > I think it would be good to make a single struct attr_data, which
> > > contains fields for all the supported types.
> > >
> > > I also see a lot of overlapping code between:
> > >
> > > drivers/platform/x86/dell-wmi-enum-attributes.c
> > > drivers/platform/x86/dell-wmi-int-attributes.c
> > > drivers/platform/x86/dell-wmi-string-attributes.c
> > >
> > > I think that folding the data structures together will help with also
> > > unifying that code into a single dell-wmi-std-attributes.c file.
> > >
>
> Yes, it does seem like lot of code is overlapping but they differ by
> properties that are little unnoticeable.
>
> If we make single file adding switch cases we may end up in many
> switch cases and if conditions. Because, here only attribute_name,
> display_lang_code, display_lang and modifier are same. Apart from
> these other properties are different either by name or data type.
>
> Also, one advantage here is if any new type is added in future it will
> be easier to create new sysfs_attr_group according to new type's
> properties
>
> We will certainly try and minimize some identical looking code
> wherever possible and add inline comments/document the
> differences more clearly in v3 which is incoming shortly.
>
> > > > +get_instance_id(enumeration);
> > > > +
> > > > +static ssize_t current_value_show(struct kobject *kobj, struct
> > > kobj_attribute *attr, char *buf)
> > > > +{
> > > > + int instance_id;
> > > > +
> > > > + if (!capable(CAP_SYS_ADMIN))
> > > > + return -EPERM;
> > > > + instance_id = get_enumeration_instance_id(kobj);
> > >
> > > If you unify the integer, string and enum code then this just becomes:
> > > get_std_instance_id(kobj)
> > >
>
> For each type of attribute GUIDs are different and for each type
> instance IDs start from zero. So if we populate them in single data
> structure then instance IDs may overlap.
>
> > > > + if (instance_id >= 0) {
> > > > + union acpi_object *obj;
> > > > +
> > > > + obj = get_wmiobj_pointer(instance_id,
> > > DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
> > > > + if (!obj)
> > > > + return -AE_ERROR;
> > >
> > > > +
> > > > strncpy_attr(enumeration_data[instance_id].current_value,
> > > > +
> > > > obj->package.elements[CURRENT_VAL].string.pointer);
> > > So these 2 lines seem to be the only thing which is different for
> > > current_value_show, between enums, ints and strings, so you can put a
> > > simple switch-case on the type here.
> > >
> >
> > This is a good point, it will cause a lot of changes as a result and a lot
> > less
> > code.
> >
> > > > + kfree(obj);
> > > > + return sprintf(buf, "%s\n",
> > > enumeration_data[instance_id].current_value);
> > > > + }
> > > > + return -EIO;
> > > > +} > +
> > > > +/**
> > > > + * validate_enumeration_input() - Validate input of current_value
> > > > +against
> > > possible values
> > > > + * @instance_id: The instance on which input is validated
> > > > + * @buf: Input value
> > > > + **/
> > > > +int validate_enumeration_input(int instance_id, const char *buf) {
> > > > + char *options, *tmp, *p;
> > > > + int ret = -EINVAL;
> > > > +
> > > > + options = tmp =
> > > > +kstrdup((enumeration_data[instance_id].possible_value),
> > > GFP_KERNEL);
> > > > + if (!options)
> > > > + return -ENOMEM;
> > > > +
> > > > + while ((p = strsep(&options, ";")) != NULL) {
> > > > + if (!*p)
> > > > + continue;
> > > > + if (!strncasecmp(p, buf, strlen(p))) {
> > > > + ret = 0;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + kfree(tmp);
> > > > + return ret;
> > > > +}
> > >
> > > For the validate functions you can keep all 3 in the new
> > > dell-wmi-std-attributes.c file and then
RE: [PATCH v2] Introduce support for Systems Management Driver over WMI for Dell Systems
> >
> > On 9/4/20 4:28 PM, Divya Bharathi wrote:
> > > diff --git a/drivers/platform/x86/dell-wmi-biosattr-interface.c
> > b/drivers/platform/x86/dell-wmi-biosattr-interface.c
> > > new file mode 100644
> > > index ..dda38d448fec
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/dell-wmi-biosattr-interface.c
> > > @@ -0,0 +1,198 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Functions corresponding to SET methods under BIOS attributes
> > > +interface
> > GUID for use
> > > + * with dell-wmi-sysman
> > > + *
> > > + * Copyright (c) 2020 Dell Inc.
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include "dell-wmi-sysman-attributes.h"
> > > +
> > > +#define SETDEFAULTVALUES_METHOD_ID
> 0x02
> > > +#define SETBIOSDEFAULTS_METHOD_ID
> 0x03
> > > +#define SETATTRIBUTE_METHOD_ID
> 0x04
> > > +
> > > +static DEFINE_MUTEX(call_mutex);
> >
> > By doing this you still declare one lock per file, so code from 2
> > files could be accessing the wmi_priv struct and be making WMI calls at the
> same time.
> >
> > Please add a mutex to the dell_wmi_sysman_priv struct, like this:
> >
> > In dell-wmi-sysman-attributes.h:
> >
> > struct dell_wmi_sysman_priv {
> > struct mutex mutex;
> > char current_admin_password[MAX_BUFF];
> > char current_system_password[MAX_BUFF];
> > struct wmi_device *password_attr_wdev;
> > struct wmi_device *bios_attr_wdev;
> > bool pending_changes;
> > };
> >
> > And in dell-wmi-sysman-attributes.c:
> >
> > struct dell_wmi_sysman_priv wmi_priv = {
> > .mutex = __MUTEX_INITIALIZER(wmi_priv.mutex),
> > };
> >
> > And then whenever you need the mutex call:
> >
> > mutex_lock(&wmi_priv.mutex);
> > ...
> > mutex_unlock(&wmi_priv.mutex);
> >
> > And use this in all files.
> >
> > > +extern struct dell_wmi_sysman_priv wmi_priv;
> >
> > Please move this to dell-wmi-sysman-attributes.h .
> >
>
> I guess locking it down to a single mutex will actually make it more
> determinant if you tried to do two things at once. We'll rework that again
> for
> v3.
>
> >
> > > +
> > > +static int call_biosattributes_interface(struct wmi_device *wdev,
> > > +char
> > *in_args, size_t size,
> > > + int method_id)
> > > +{
> > > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> > > + struct acpi_buffer input;
> > > + union acpi_object *obj;
> > > + acpi_status status;
> > > + int ret = -EIO;
> > > +
> > > + input.length = (acpi_size) size;
> > > + input.pointer = in_args;
> > > + status = wmidev_evaluate_method(wdev, 0, method_id, &input,
> &output);
> > > + if (ACPI_FAILURE(status))
> > > + return -EIO;
> > > + obj = (union acpi_object *)output.pointer;
> > > + if (obj->type == ACPI_TYPE_INTEGER)
> > > + ret = obj->integer.value;
> > > +
> > > + kfree(output.pointer);
> > > + kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
> >
> > Why are you generating an uevent here? I think this needs a comment
> > (or it should be removed).
>
> It's there so that userspace can know that a setting has been changed and
> doesn't need to poll the attribute indicating a reboot is pending.
>
> We can certainly add a comment to this effect.
>
> >
> > > + return map_wmi_error(ret);
> > > +}
> > > +
> > > +/**
> > > + * set_attribute() - Update an attribute value
> > > + * @a_name: The attribute name
> > > + * @a_value: The attribute value
> > > + *
> > > + * Sets an attribute to new value
> > > + **/
> > > +int set_attribute(const char *a_name, const char *a_value) {
> > > + size_t security_area_size, string_area_size, buffer_size;
> > > + char *attribute_name, *attribute_value;
> > > + u8 *name_len, *value_len;
> > > + char *buffer;
> > > + int ret;
> > > +
> > > + /* build/calculate buffer */
> > > + security_area_size = calculate_security_buffer();
> > > + string_area_size = (strlen(a_name) + strlen(a_value))*2;
> > > + buffer_size = security_area_size + string_area_size + sizeof(u16) * 2;
> > > + buffer = kzalloc(buffer_size, GFP_KERNEL);
> > > + if (!buffer)
> > > + return -ENOMEM;
> > > +
> > > + /* build security area */
> > > + if (strlen(wmi_priv.current_admin_password) > 0)
> > > + populate_security_buffer(buffer,
> > > +wmi_priv.current_admin_password);
> > > +
> > > + /* build variables to set */
> > > + name_len = buffer + security_area_size;
> > > + attribute_name = name_len + sizeof(u16);
> > > + *name_len = utf8s_to_utf16s(a_name, strlen(a_name),
> UTF16_HOST_ENDIAN,
> > > + (wchar_t *) attribute_name, MAX_BUFF) *
> 2;
> > > + if (*name_len < 0) {
> > > + ret = -EINVAL;
> > > + pr_err("UTF16 conversion failed");
> > > + goto out_set_attribute;
> > > + }
> > > +
> > > + value_len = (u8 *) attribute_name + *name_len;
> > > + attribute_value = value_len + sizeof(u16);
> > > + *value_len = utf8s_to_utf16s(a_value, strlen(a_value),
> > UTF16_HOST_ENDIAN,
> > > +
