Messages in this thread | | | Subject | Re: [PATCH v2] Introduce support for Systems Management Driver over WMI for Dell Systems | From | Hans de Goede <> | Date | Mon, 21 Sep 2020 11:38:11 +0200 |
| |
Hi,
On 9/17/20 7:22 AM, Bharathi, Divya wrote:
<snip>
>>>>> + >>>>> +/** >>>>> + * 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.
sysfs_remove_group() just does a kernfs_remove_by_name() for each attribute in the group.
Since the integer_, enumeration_ and string_ attr_group-s all have e.g. a current_value attribute that means that current_value will be removed 3 times and for the 2nd and 3th call kernfs_remove_by_name() will fail with -ENOENT.
Currently neither kernfs_remove_by_name() nor sysfs_remove_group() print an error message for this, but still it is not very clean.
Instead why not do this:
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(&wmi_priv.mutex); enumeration_data[instance_id].attr_name_kobj = attr_name_kobj; /* ^^^^^^^^^^^^^^^^ This line is new ^^^^^^^^^^^^^^^^^^^^^^^^*/ ...
void exit_enum_attributes(void) { int i;
for (i = 0; i < enumeration_instances_count; i++) { if (enumeration_data[instance_id].attr_name_kobj) sysfs_remove_group(enumeration_data[instance_id].attr_name_kobj, &enumeration_attr_group); }
kfree(enumeration_data); }
That makes the teardown mirror the setup much more closely and as such is a cleaner solution IMHO.
Regards,
Hans
| |