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

[Hans de Goede]

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