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

From: Hans de Goede
Date: Mon Sep 21 2020 - 05:38:18 EST


On 9/17/20 7:22 AM, Bharathi, Divya wrote:


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



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;

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);


That makes the teardown mirror the setup much more closely and as such is
a cleaner solution IMHO.