Message-ID: <CY4PR19MB1254A26A233052B71ACC5ACA853E0@CY4PR19MB1254.namprd19.prod.outlook.com>
Date:   Thu, 17 Sep 2020 05:22:59 +0000
From:   "Bharathi, Divya" <Divya.Bharathi@...l.com>
To:     "Limonciello, Mario" <Mario.Limonciello@...l.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Divya Bharathi <divya27392@...il.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>
Subject: RE: [PATCH v2] Introduce support for Systems Management Driver over
 WMI for Dell Systems


<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.
> 
> > > > +	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 call the right one through a
> > > switch-case based on the type.
> > >
> > > > +
> > > > +attribute_s_property_show(display_name_language_code,
> > enumeration);
> > > > +static struct kobj_attribute displ_langcode =
> > > > +		__ATTR_RO(display_name_language_code);
> > > > +
> > > > +attribute_s_property_show(display_name, enumeration); struct
> > > > +kobj_attribute displ_name =
> > > > +		__ATTR_RO(display_name);
> > > > +
> > > > +attribute_s_property_show(default_value, enumeration); struct
> > > > +kobj_attribute default_val =
> > > > +		__ATTR_RO(default_value);
> > > > +
> > > > +attribute_property_store(current_value, enumeration); struct
> > > > +kobj_attribute current_val =
> > > > +		__ATTR_RW(current_value);
> > > > +
> > > > +attribute_s_property_show(modifier, enumeration); struct
> > > > +kobj_attribute modifier =
> > > > +		__ATTR_RO(modifier);
> > > > +
> > > > +attribute_s_property_show(value_modifier, enumeration); struct
> > > > +kobj_attribute value_modfr =
> > > > +		__ATTR_RO(value_modifier);
> > > > +
> > > > +attribute_s_property_show(possible_value, enumeration); struct
> > > > +kobj_attribute poss_val =
> > > > +		__ATTR_RO(possible_value);
> > > > +
> > > > +attribute_s_property_show(type, enumeration); struct kobj_attribute
> > > > +type =
> > > > +		__ATTR_RO(type);
> > > > +
> > > > +static struct attribute *enumeration_attrs[] = {
> > > > +	&displ_langcode.attr,
> > > > +	&displ_name.attr,
> > > > +	&default_val.attr,
> > > > +	&current_val.attr,
> > > > +	&modifier.attr,
> > > > +	&value_modfr.attr,
> > > > +	&poss_val.attr,
> > > > +	&type.attr,
> > > > +	NULL,
> > > > +};
> > > > +
> > > > +static const struct attribute_group enumeration_attr_group = {
> > > > +	.attrs = enumeration_attrs,
> > > > +};
> > > > +
> > > > +int alloc_enum_data(void)
> > > > +{
> > > > +	int ret = 0;
> > > > +
> > > > +	enumeration_instances_count =
> > > get_instance_count(DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
> > > > +	enumeration_data = kzalloc((sizeof(struct enumeration_data) *
> > > enumeration_instances_count),
> > > > +					GFP_KERNEL);
> > > > +	if (!enumeration_data)
> > > > +		ret = -ENOMEM;
> > > > +	return ret;
> > > > +}
> > > > +
> > >
> > > The rest of the file is mostly identical between strings, ints and enums.
> > >
> > > > +/**
> > > > + * populate_enum_data() - Populate all properties of an instance
> > > > +under
> > > enumeration attribute
> > > > + * @enumeration_obj: ACPI object with enumeration data
> > > > + * @instance_id: The instance to enumerate
> > > > + * @attr_name_kobj: The parent kernel object  **/ int
> > > > +populate_enum_data(union acpi_object *enumeration_obj, int
> > instance_id,
> > > > +			struct kobject *attr_name_kobj)
> > > > +{
> > > > +	int retval = sysfs_create_group(attr_name_kobj,
> > > &enumeration_attr_group);
> > > > +	int i, next_obj;
> > > > +
> > > > +	if (retval)
> > > > +		goto out;
> > > > +
> > > > +	mutex_lock(&enum_mutex);
> > > > +	strncpy_attr(enumeration_data[instance_id].attribute_name,
> > > > +		enumeration_obj[ATTR_NAME].string.pointer);
> > > > +
> > 	strncpy_attr(enumeration_data[instance_id].display_name_language
> > _code,
> > > > +		enumeration_obj[DISPL_NAME_LANG_CODE].string.pointer);
> > > > +	strncpy_attr(enumeration_data[instance_id].display_name,
> > > > +		enumeration_obj[DISPLAY_NAME].string.pointer);
> > > > +	strncpy_attr(enumeration_data[instance_id].default_value,
> > > > +		enumeration_obj[DEFAULT_VAL].string.pointer);
> > > > +	strncpy_attr(enumeration_data[instance_id].current_value,
> > > > +		enumeration_obj[CURRENT_VAL].string.pointer);
> > > > +	strncpy_attr(enumeration_data[instance_id].modifier,
> > > > +		enumeration_obj[MODIFIER].string.pointer);
> > > > +
> > > > +	next_obj = MODIFIER + 1;
> > > > +
> > > > +	enumeration_data[instance_id].value_modifier_count =
> > > > +		(uintptr_t)enumeration_obj[next_obj].string.pointer;
> > > > +
> > > > +	for (i = 0; i <
> > > > +enumeration_data[instance_id].value_modifier_count; i++)
> > > {
> > > > +		strcat(enumeration_data[instance_id].value_modifier,
> > > > +			enumeration_obj[++next_obj].string.pointer);
> > > > +		strcat(enumeration_data[instance_id].value_modifier, ";");
> > > > +	}
> > > > +
> > > > +	enumeration_data[instance_id].possible_value_count =
> > > > +		(uintptr_t) enumeration_obj[++next_obj].string.pointer;
> > > > +
> > > > +	for (i = 0; i <
> > > > +enumeration_data[instance_id].possible_value_count; i++)
> > > {
> > > > +		strcat(enumeration_data[instance_id].possible_value,
> > > > +			enumeration_obj[++next_obj].string.pointer);
> > > > +		strcat(enumeration_data[instance_id].possible_value, ";");
> > > > +	}
> > > > +	strncpy_attr(enumeration_data[instance_id].type, "enumeration");
> > > > +	mutex_unlock(&enum_mutex);
> > > > +
> > > > +out:
> > > > +	return retval;
> > > > +}
> > >
> > >
> > > I guess you may also need a switch-case based on the type in the
> > > populare function.
> > >
> > > > +
> > > > +/**
> > > > + * exit_enum_attributes() - Clear all attribute data
> > > > + * @kset: The kset to free
> > > > + *
> > > > + * Clears all data allocated for this group of attributes  **/ void
> > > > +exit_enum_attributes(struct kset *kset) {
> > > > +	struct kobject *pos, *next;
> > > > +
> > > > +	mutex_lock(&kset_mutex);
> > > > +	list_for_each_entry_safe(pos, next, &kset->list, entry) {
> > > > +		sysfs_remove_group(pos, &enumeration_attr_group);
> > > > +	}
> > > > +	mutex_unlock(&kset_mutex);
> > > > +	mutex_lock(&enum_mutex);
> > > > +	kfree(enumeration_data);
> > > > +	mutex_unlock(&enum_mutex);
> > > > +}
> > >
> > > Since there is now only 1 kset for the main dir, you are now calling
> > > sysfs_remove_group 4 times (for all the different times) on each entry
> > > in the attributes dir. I guess this may fail silently, but it still is
> > > not good. So this needs to be fixed.
> > >
> > > The remarks to this file also apply to the:
> > >
> > > drivers/platform/x86/dell-wmi-int-attributes.c
> > > drivers/platform/x86/dell-wmi-string-attributes.c
> > >
> > > files.
> > >

Since we maintained 4 different attribute groups under 1 kset, each time
respective attribute group will be removed. And once all groups are
removed, kset is deleted.

Regards,
Divya

Powered by blists - more mailing lists