[PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir

[PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir

[Hans de Goede]

From: Mark Pearson <markpearson@lenovo.com>

Move the pending_reboot node under attributes dir where it should live, as
documented in: Documentation/ABI/testing/sysfs-class-firmware-attributes.

Also move the create / remove code to be together with the other code
populating / cleaning the attributes sysfs dir. In the removal path this
is necessary so that the remove is done before the
kset_unregister(tlmi_priv.attribute_kset) call.

Signed-off-by: Mark Pearson <markpearson@lenovo.com>
Co-developed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/think-lmi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 64dcec53a7a0..989a8221dcd8 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -672,6 +672,7 @@ static void tlmi_release_attr(void)
 			kobject_put(&tlmi_priv.setting[i]->kobj);
 		}
 	}
+	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
 	kset_unregister(tlmi_priv.attribute_kset);
 
 	/* Authentication structures */
@@ -680,7 +681,6 @@ static void tlmi_release_attr(void)
 	sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group);
 	kobject_put(&tlmi_priv.pwd_power->kobj);
 	kset_unregister(tlmi_priv.authentication_kset);
-	sysfs_remove_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
 }
 
 static int tlmi_sysfs_init(void)
@@ -733,6 +733,10 @@ static int tlmi_sysfs_init(void)
 			goto fail_create_attr;
 	}
 
+	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
+	if (ret)
+		goto fail_create_attr;
+
 	/* Create authentication entries */
 	tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
 								&tlmi_priv.class_dev->kobj);
@@ -760,11 +764,6 @@ static int tlmi_sysfs_init(void)
 	if (ret)
 		goto fail_create_attr;
 
-	/* Create global sysfs files */
-	ret = sysfs_create_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
-	if (ret)
-		goto fail_create_attr;
-
 	return ret;
 
 fail_create_attr:
-- 
2.31.1

[PATCH 2/3] platform/x86: think-lmi: Split kobject_init() and kobject_add() calls

[Hans de Goede]

tlmi_sysfs_init() calls tlmi_release_attr() on errors which calls
kobject_put() for attributes created by tlmi_analyze(), but if we
bail early because of an error, then this means that some of the
kobjects will not have been initialized yet; and we should thus not
call kobject_put() on them.

Switch from using kobject_init_and_add() inside tlmi_sysfs_init() to
initializing all the created kobjects directly in tlmi_analyze() and
only adding them from tlmi_sysfs_init(). This way all kobjects will
always be initialized when tlmi_release_attr() gets called.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/think-lmi.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 989a8221dcd8..c22edcf26aaa 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -723,8 +723,8 @@ static int tlmi_sysfs_init(void)
 
 		/* Build attribute */
 		tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset;
-		ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &tlmi_attr_setting_ktype,
-				NULL, "%s", tlmi_priv.setting[i]->display_name);
+		ret = kobject_add(&tlmi_priv.setting[i]->kobj, NULL,
+				  "%s", tlmi_priv.setting[i]->display_name);
 		if (ret)
 			goto fail_create_attr;
 
@@ -745,8 +745,7 @@ static int tlmi_sysfs_init(void)
 		goto fail_create_attr;
 	}
 	tlmi_priv.pwd_admin->kobj.kset = tlmi_priv.authentication_kset;
-	ret = kobject_init_and_add(&tlmi_priv.pwd_admin->kobj, &tlmi_pwd_setting_ktype,
-			NULL, "%s", "Admin");
+	ret = kobject_add(&tlmi_priv.pwd_admin->kobj, NULL, "%s", "Admin");
 	if (ret)
 		goto fail_create_attr;
 
@@ -755,8 +754,7 @@ static int tlmi_sysfs_init(void)
 		goto fail_create_attr;
 
 	tlmi_priv.pwd_power->kobj.kset = tlmi_priv.authentication_kset;
-	ret = kobject_init_and_add(&tlmi_priv.pwd_power->kobj, &tlmi_pwd_setting_ktype,
-			NULL, "%s", "System");
+	ret = kobject_add(&tlmi_priv.pwd_power->kobj, NULL, "%s", "System");
 	if (ret)
 		goto fail_create_attr;
 
@@ -836,6 +834,7 @@ static int tlmi_analyze(void)
 				pr_info("Error retrieving possible values for %d : %s\n",
 						i, setting->display_name);
 		}
+		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
 		tlmi_priv.setting[i] = setting;
 		tlmi_priv.settings_count++;
 		kfree(item);
@@ -862,6 +861,8 @@ static int tlmi_analyze(void)
 	if (pwdcfg.password_state & TLMI_PAP_PWD)
 		tlmi_priv.pwd_admin->valid = true;
 
+	kobject_init(&tlmi_priv.pwd_admin->kobj, &tlmi_pwd_setting_ktype);
+
 	tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
 	if (!tlmi_priv.pwd_power) {
 		ret = -ENOMEM;
@@ -877,6 +878,8 @@ static int tlmi_analyze(void)
 	if (pwdcfg.password_state & TLMI_POP_PWD)
 		tlmi_priv.pwd_power->valid = true;
 
+	kobject_init(&tlmi_priv.pwd_power->kobj, &tlmi_pwd_setting_ktype);
+
 	return 0;
 
 fail_clear_attr:
-- 
2.31.1

[PATCH 3/3] platform/x86: think-lmi: Fix possible mem-leaks on tlmi_analyze() error-exit

[Hans de Goede]

Fix 2 possible memleaks on error-exits from tlmi_analyze():

1. If the kzalloc of pwd_power fails, then not only free the atributes,
   but also the allocated pwd_admin struct.

2. Freeing the attributes should also free the possible_values strings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/think-lmi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c22edcf26aaa..6cfed4427fb0 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -866,7 +866,7 @@ static int tlmi_analyze(void)
 	tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL);
 	if (!tlmi_priv.pwd_power) {
 		ret = -ENOMEM;
-		goto fail_clear_attr;
+		goto fail_free_pwd_admin;
 	}
 	strscpy(tlmi_priv.pwd_power->kbdlang, "us", TLMI_LANG_MAXLEN);
 	tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII;
@@ -882,9 +882,15 @@ static int tlmi_analyze(void)
 
 	return 0;
 
+fail_free_pwd_admin:
+	kfree(tlmi_priv.pwd_admin);
 fail_clear_attr:
-	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i)
-		kfree(tlmi_priv.setting[i]);
+	for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
+		if (tlmi_priv.setting[i]) {
+			kfree(tlmi_priv.setting[i]->possible_values);
+			kfree(tlmi_priv.setting[i]);
+		}
+	}
 	return ret;
 }
 
-- 
2.31.1

Re: [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir

[Hans de Goede]

Hi,

On 7/17/21 4:36 PM, Hans de Goede wrote:
> From: Mark Pearson <markpearson@lenovo.com>
> 
> Move the pending_reboot node under attributes dir where it should live, as
> documented in: Documentation/ABI/testing/sysfs-class-firmware-attributes.
> 
> Also move the create / remove code to be together with the other code
> populating / cleaning the attributes sysfs dir. In the removal path this
> is necessary so that the remove is done before the
> kset_unregister(tlmi_priv.attribute_kset) call.
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> Co-developed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I've added this series to my review-hans and the pdx86/fixes
branches now.

Regards,

Hans


> ---
>  drivers/platform/x86/think-lmi.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 64dcec53a7a0..989a8221dcd8 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -672,6 +672,7 @@ static void tlmi_release_attr(void)
>  			kobject_put(&tlmi_priv.setting[i]->kobj);
>  		}
>  	}
> +	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
>  	kset_unregister(tlmi_priv.attribute_kset);
>  
>  	/* Authentication structures */
> @@ -680,7 +681,6 @@ static void tlmi_release_attr(void)
>  	sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group);
>  	kobject_put(&tlmi_priv.pwd_power->kobj);
>  	kset_unregister(tlmi_priv.authentication_kset);
> -	sysfs_remove_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
>  }
>  
>  static int tlmi_sysfs_init(void)
> @@ -733,6 +733,10 @@ static int tlmi_sysfs_init(void)
>  			goto fail_create_attr;
>  	}
>  
> +	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
> +	if (ret)
> +		goto fail_create_attr;
> +
>  	/* Create authentication entries */
>  	tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
>  								&tlmi_priv.class_dev->kobj);
> @@ -760,11 +764,6 @@ static int tlmi_sysfs_init(void)
>  	if (ret)
>  		goto fail_create_attr;
>  
> -	/* Create global sysfs files */
> -	ret = sysfs_create_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
> -	if (ret)
> -		goto fail_create_attr;
> -
>  	return ret;
>  
>  fail_create_attr:
> 

Re: [External]Re: [PATCH 1/3] platform/x86: think-lmi: Move pending_reboot_attr to the attributes sysfs dir

[Mark Pearson]

On 2021-07-17 11:27 a.m., Hans de Goede wrote:
> Hi,
> 
> On 7/17/21 4:36 PM, Hans de Goede wrote:
>> From: Mark Pearson <markpearson@lenovo.com>
>>
>> Move the pending_reboot node under attributes dir where it should live, as
>> documented in: Documentation/ABI/testing/sysfs-class-firmware-attributes.
>>
>> Also move the create / remove code to be together with the other code
>> populating / cleaning the attributes sysfs dir. In the removal path this
>> is necessary so that the remove is done before the
>> kset_unregister(tlmi_priv.attribute_kset) call.
>>
>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>> Co-developed-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> I've added this series to my review-hans and the pdx86/fixes
> branches now.
> 
Thanks Hans - they all look good to me.
I'll do some testing on a couple of my systems to confirm no issues

Mark
> Regards,
> 
> Hans
> 
> 
>> ---
>>   drivers/platform/x86/think-lmi.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 64dcec53a7a0..989a8221dcd8 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -672,6 +672,7 @@ static void tlmi_release_attr(void)
>>   			kobject_put(&tlmi_priv.setting[i]->kobj);
>>   		}
>>   	}
>> +	sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
>>   	kset_unregister(tlmi_priv.attribute_kset);
>>   
>>   	/* Authentication structures */
>> @@ -680,7 +681,6 @@ static void tlmi_release_attr(void)
>>   	sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group);
>>   	kobject_put(&tlmi_priv.pwd_power->kobj);
>>   	kset_unregister(tlmi_priv.authentication_kset);
>> -	sysfs_remove_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
>>   }
>>   
>>   static int tlmi_sysfs_init(void)
>> @@ -733,6 +733,10 @@ static int tlmi_sysfs_init(void)
>>   			goto fail_create_attr;
>>   	}
>>   
>> +	ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
>> +	if (ret)
>> +		goto fail_create_attr;
>> +
>>   	/* Create authentication entries */
>>   	tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL,
>>   								&tlmi_priv.class_dev->kobj);
>> @@ -760,11 +764,6 @@ static int tlmi_sysfs_init(void)
>>   	if (ret)
>>   		goto fail_create_attr;
>>   
>> -	/* Create global sysfs files */
>> -	ret = sysfs_create_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr);
>> -	if (ret)
>> -		goto fail_create_attr;
>> -
>>   	return ret;
>>   
>>   fail_create_attr:
>>
>