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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

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

<snip>

>>>> +/* 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.

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.

Regards,

Hans