Re: [PATCH v2] Introduce support for Systems Management Driver over WMI for Dell Systems

From: Hans de Goede
Date: Mon Sep 21 2020 - 05:19:07 EST


On 9/15/20 6:28 PM, Bharathi, Divya wrote:


+/* kept variable names same as in sysfs file name for sysfs_show
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

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:


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

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.

+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:

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.

Ah, I missed that, because of the switch-case in init_bios_attributes()
I assumed it was only called once and all attributes were enumerated
in a single loop.

I see that init_bios_attributes() gets called once for each of
ENUM, INT, STR and PO now. My mistake, sorry.

So you are right. Since the instance-ids overlap then my idea will not
work and we need to keep separate foo_data arrays per type.

It might still be worth it to unify enum_data, string_data and
integer_data into a single shared struct so that some of the
sysfs getter functions can be shared. I will leave that up to you.