[PATCH v3 1/3] platform/x86: think-lmi: add missing type attribute

[PATCH v3 1/3] platform/x86: think-lmi: add missing type attribute

[Mark Pearson]

This driver was missing the mandatory type attribute...oops.

Add it in along with logic to determine whether the attribute is an
enumeration type or a string by parsing the possible_values attribute.

Upstream bug https://bugzilla.kernel.org/show_bug.cgi?id=216460

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in v3:
 - Rebased on latest pdx86, review_hans branch
Changes in v2: 
 - Simplify the code and move type determination into show function
 - Don't use Fixes with URL in commit info

 drivers/platform/x86/think-lmi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 86b33b74519b..5fa5451c4802 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -947,6 +947,20 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute
 	return sysfs_emit(buf, "%s\n", setting->possible_values);
 }
 
+static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
+		char *buf)
+{
+	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
+
+	if (setting->possible_values) {
+		/* Figure out what setting type is as BIOS does not return this */
+		if (strchr(setting->possible_values, ','))
+			return sysfs_emit(buf, "enumeration\n");
+	}
+	/* Anything else is going to be a string */
+	return sysfs_emit(buf, "string\n");
+}
+
 static ssize_t current_value_store(struct kobject *kobj,
 		struct kobj_attribute *attr,
 		const char *buf, size_t count)
@@ -1036,10 +1050,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values);
 
 static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
 
+static struct kobj_attribute attr_type = __ATTR_RO(type);
+
 static struct attribute *tlmi_attrs[] = {
 	&attr_displ_name.attr,
 	&attr_current_val.attr,
 	&attr_possible_values.attr,
+	&attr_type.attr,
 	NULL
 };
 
-- 
2.39.2

[PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Mark Pearson]

ThinkStation platforms don't support the API to return possible_values
but instead embed it in the settings string.

Try and extract this information and set the possible_values attribute
appropriately.

If there aren't any values possible then don't display possible_values.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
Changes in V3:
 - Use is_visible attribute to determine if possible_values should be
   available
 - Code got refactored a bit to make compilation cleaner
Changes in V2:
 - Move no value for possible_values handling into show function
 - use kstrndup for allocating string

 drivers/platform/x86/think-lmi.c | 82 ++++++++++++++++++++++----------
 1 file changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 5fa5451c4802..d89a1c9bdbf1 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -917,6 +917,8 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
 	return sysfs_emit(buf, "%s\n", setting->display_name);
 }
 
+static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
+
 static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 {
 	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
@@ -937,30 +939,6 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
 	return ret;
 }
 
-static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
-
-	if (!tlmi_priv.can_get_bios_selections)
-		return -EOPNOTSUPP;
-
-	return sysfs_emit(buf, "%s\n", setting->possible_values);
-}
-
-static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
-		char *buf)
-{
-	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
-
-	if (setting->possible_values) {
-		/* Figure out what setting type is as BIOS does not return this */
-		if (strchr(setting->possible_values, ','))
-			return sysfs_emit(buf, "enumeration\n");
-	}
-	/* Anything else is going to be a string */
-	return sysfs_emit(buf, "string\n");
-}
-
 static ssize_t current_value_store(struct kobject *kobj,
 		struct kobj_attribute *attr,
 		const char *buf, size_t count)
@@ -1044,14 +1022,46 @@ static ssize_t current_value_store(struct kobject *kobj,
 	return ret ?: count;
 }
 
-static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
+static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
+
+static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
+
+	return sysfs_emit(buf, "%s\n", setting->possible_values);
+}
 
 static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values);
 
-static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
+static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
+		char *buf)
+{
+	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
+
+	if (setting->possible_values) {
+		/* Figure out what setting type is as BIOS does not return this */
+		if (strchr(setting->possible_values, ','))
+			return sysfs_emit(buf, "enumeration\n");
+	}
+	/* Anything else is going to be a string */
+	return sysfs_emit(buf, "string\n");
+}
 
 static struct kobj_attribute attr_type = __ATTR_RO(type);
 
+static umode_t attr_is_visible(struct kobject *kobj,
+					     struct attribute *attr, int n)
+{
+	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
+
+	/* We don't want to display possible_values attributes if not available */
+	if (attr == (struct attribute *)&attr_possible_values)
+		if (!setting->possible_values)
+			return 0;
+
+	return attr->mode;
+}
+
 static struct attribute *tlmi_attrs[] = {
 	&attr_displ_name.attr,
 	&attr_current_val.attr,
@@ -1061,6 +1071,7 @@ static struct attribute *tlmi_attrs[] = {
 };
 
 static const struct attribute_group tlmi_attr_group = {
+	.is_visible = attr_is_visible,
 	.attrs = tlmi_attrs,
 };
 
@@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
 			if (ret || !setting->possible_values)
 				pr_info("Error retrieving possible values for %d : %s\n",
 						i, setting->display_name);
+		} else {
+			/*
+			 * Older Thinkstations don't support the bios_selections API.
+			 * Instead they store this as a [Optional:Option1,Option2] section of the
+			 * name string.
+			 * Try and pull that out if it's available.
+			 */
+			char *item, *optstart, *optend;
+
+			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
+				optstart = strstr(item, "[Optional:");
+				if (optstart) {
+					optstart += strlen("[Optional:");
+					optend = strstr(optstart, "]");
+					if (optend)
+						setting->possible_values =
+							kstrndup(optstart, optend - optstart, GFP_KERNEL);
+				}
+			}
 		}
 		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
 		tlmi_priv.setting[i] = setting;
-- 
2.39.2

[PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Mark Pearson]

firmware-attributes class requires that possible values are delimited
using ';' but the Lenovo firmware uses ',' instead.
Parse string and replace where appropriate

Thanks to Thomas W for pointing this out.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 Changes in V3: New patch added to the series. No V1 & V2.

 drivers/platform/x86/think-lmi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index d89a1c9bdbf1..204f1060a533 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
 
 	if (setting->possible_values) {
 		/* Figure out what setting type is as BIOS does not return this */
-		if (strchr(setting->possible_values, ','))
+		if (strchr(setting->possible_values, ';'))
 			return sysfs_emit(buf, "enumeration\n");
 	}
 	/* Anything else is going to be a string */
@@ -1471,6 +1471,17 @@ static int tlmi_analyze(void)
 				}
 			}
 		}
+		/*
+		 * firmware-attributes requires that possible_values are separated by ';' but
+		 * Lenovo FW uses ','. Replace appropriately.
+		 */
+		if (setting->possible_values) {
+			char *tmp = setting->possible_values;
+
+			while ((tmp = strchr(tmp, ',')) != NULL)
+				*tmp++ = ';';
+		}
+
 		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
 		tlmi_priv.setting[i] = setting;
 		kfree(item);
-- 
2.39.2

Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Barnabás Pőcze]

Hi


2023. március 17., péntek 16:46 keltezéssel, Mark Pearson <mpearson-lenovo@squebb.ca> írta:

> firmware-attributes class requires that possible values are delimited
> using ';' but the Lenovo firmware uses ',' instead.
> Parse string and replace where appropriate
> 
> Thanks to Thomas W for pointing this out.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
> [...]
> +		/*
> +		 * firmware-attributes requires that possible_values are separated by ';' but
> +		 * Lenovo FW uses ','. Replace appropriately.
> +		 */
> +		if (setting->possible_values) {
> +			char *tmp = setting->possible_values;
> +
> +			while ((tmp = strchr(tmp, ',')) != NULL)
> +				*tmp++ = ';';
> +		}

Please see `strreplace()` from `linux/string.h`.


> +
>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>  		tlmi_priv.setting[i] = setting;
>  		kfree(item);
> --
> 2.39.2


Regards,
Barnabás Pőcze

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Thomas Weißschuh]

Hi Mark,

please also CC linux-kernel@vger.kernel.org and previous reviewers.

On Fri, Mar 17, 2023 at 11:46:34AM -0400, Mark Pearson wrote:
> ThinkStation platforms don't support the API to return possible_values
> but instead embed it in the settings string.
> 
> Try and extract this information and set the possible_values attribute
> appropriately.
> 
> If there aren't any values possible then don't display possible_values.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
> Changes in V3:
>  - Use is_visible attribute to determine if possible_values should be
>    available
>  - Code got refactored a bit to make compilation cleaner
> Changes in V2:
>  - Move no value for possible_values handling into show function
>  - use kstrndup for allocating string
> 
>  drivers/platform/x86/think-lmi.c | 82 ++++++++++++++++++++++----------
>  1 file changed, 56 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 5fa5451c4802..d89a1c9bdbf1 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -917,6 +917,8 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
>  	return sysfs_emit(buf, "%s\n", setting->display_name);
>  }
>  
> +static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
> +
>  static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>  {
>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> @@ -937,30 +939,6 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>  	return ret;
>  }
>  
> -static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> -{
> -	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> -
> -	if (!tlmi_priv.can_get_bios_selections)
> -		return -EOPNOTSUPP;
> -
> -	return sysfs_emit(buf, "%s\n", setting->possible_values);
> -}
> -
> -static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> -		char *buf)
> -{
> -	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> -
> -	if (setting->possible_values) {
> -		/* Figure out what setting type is as BIOS does not return this */
> -		if (strchr(setting->possible_values, ','))
> -			return sysfs_emit(buf, "enumeration\n");
> -	}
> -	/* Anything else is going to be a string */
> -	return sysfs_emit(buf, "string\n");
> -}
> -
>  static ssize_t current_value_store(struct kobject *kobj,
>  		struct kobj_attribute *attr,
>  		const char *buf, size_t count)
> @@ -1044,14 +1022,46 @@ static ssize_t current_value_store(struct kobject *kobj,
>  	return ret ?: count;
>  }
>  
> -static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
> +static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
> +
> +static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> +
> +	return sysfs_emit(buf, "%s\n", setting->possible_values);
> +}
>  
>  static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values);
>  
> -static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> +		char *buf)
> +{
> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> +
> +	if (setting->possible_values) {
> +		/* Figure out what setting type is as BIOS does not return this */
> +		if (strchr(setting->possible_values, ','))
> +			return sysfs_emit(buf, "enumeration\n");
> +	}
> +	/* Anything else is going to be a string */
> +	return sysfs_emit(buf, "string\n");
> +}

This patch seems to introduce a lot of churn, is it intentional?
>  
>  static struct kobj_attribute attr_type = __ATTR_RO(type);
>  
> +static umode_t attr_is_visible(struct kobject *kobj,
> +					     struct attribute *attr, int n)
> +{
> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> +
> +	/* We don't want to display possible_values attributes if not available */
> +	if (attr == (struct attribute *)&attr_possible_values)

This cast is unsafe, if the struct kobj_attribute order is randomised it
will break.

You can use

	if (attr == &attr_possible_values.attr)

> +		if (!setting->possible_values)
> +			return 0;
> +
> +	return attr->mode;
> +}
> +
>  static struct attribute *tlmi_attrs[] = {
>  	&attr_displ_name.attr,
>  	&attr_current_val.attr,
> @@ -1061,6 +1071,7 @@ static struct attribute *tlmi_attrs[] = {
>  };
>  
>  static const struct attribute_group tlmi_attr_group = {
> +	.is_visible = attr_is_visible,
>  	.attrs = tlmi_attrs,
>  };
>  
> @@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
>  			if (ret || !setting->possible_values)
>  				pr_info("Error retrieving possible values for %d : %s\n",
>  						i, setting->display_name);
> +		} else {
> +			/*
> +			 * Older Thinkstations don't support the bios_selections API.
> +			 * Instead they store this as a [Optional:Option1,Option2] section of the
> +			 * name string.
> +			 * Try and pull that out if it's available.
> +			 */
> +			char *item, *optstart, *optend;
> +
> +			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
> +				optstart = strstr(item, "[Optional:");
> +				if (optstart) {
> +					optstart += strlen("[Optional:");
> +					optend = strstr(optstart, "]");
> +					if (optend)
> +						setting->possible_values =
> +							kstrndup(optstart, optend - optstart, GFP_KERNEL);
> +				}
> +			}

The patch now does two things:
1) Hide the sysfs attributes if the value is not available
2) Extract the value from the description

Maybe it could be split in two?

Another observation:
Would it make sense to remove the part
"[Optional:Option1,Option2]" from the name attribute?

>  		}
>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>  		tlmi_priv.setting[i] = setting;
> -- 
> 2.39.2
> 

Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Thomas Weißschuh]

On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote:
> firmware-attributes class requires that possible values are delimited
> using ';' but the Lenovo firmware uses ',' instead.
> Parse string and replace where appropriate
> 
> Thanks to Thomas W for pointing this out.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  Changes in V3: New patch added to the series. No V1 & V2.
> 
>  drivers/platform/x86/think-lmi.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index d89a1c9bdbf1..204f1060a533 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	if (setting->possible_values) {
>  		/* Figure out what setting type is as BIOS does not return this */
> -		if (strchr(setting->possible_values, ','))
> +		if (strchr(setting->possible_values, ';'))
>  			return sysfs_emit(buf, "enumeration\n");

I think this patch should be earlier in the series.
So the other patches can work directly from the beginning.

Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets
backported.

>  	}
>  	/* Anything else is going to be a string */
> @@ -1471,6 +1471,17 @@ static int tlmi_analyze(void)
>  				}
>  			}
>  		}
> +		/*
> +		 * firmware-attributes requires that possible_values are separated by ';' but
> +		 * Lenovo FW uses ','. Replace appropriately.
> +		 */
> +		if (setting->possible_values) {
> +			char *tmp = setting->possible_values;
> +
> +			while ((tmp = strchr(tmp, ',')) != NULL)
> +				*tmp++ = ';';
> +		}
> +
>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>  		tlmi_priv.setting[i] = setting;
>  		kfree(item);
> -- 
> 2.39.2
> 

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Mark Pearson]

Thanks Thomas

On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
> Hi Mark,
>
> please also CC linux-kernel@vger.kernel.org and previous reviewers.
>
> On Fri, Mar 17, 2023 at 11:46:34AM -0400, Mark Pearson wrote:
>> ThinkStation platforms don't support the API to return possible_values
>> but instead embed it in the settings string.
>> 
>> Try and extract this information and set the possible_values attribute
>> appropriately.
>> 
>> If there aren't any values possible then don't display possible_values.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>> Changes in V3:
>>  - Use is_visible attribute to determine if possible_values should be
>>    available
>>  - Code got refactored a bit to make compilation cleaner
>> Changes in V2:
>>  - Move no value for possible_values handling into show function
>>  - use kstrndup for allocating string
>> 
>>  drivers/platform/x86/think-lmi.c | 82 ++++++++++++++++++++++----------
>>  1 file changed, 56 insertions(+), 26 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 5fa5451c4802..d89a1c9bdbf1 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -917,6 +917,8 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
>>  	return sysfs_emit(buf, "%s\n", setting->display_name);
>>  }
>>  
>> +static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
>> +
>>  static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>>  {
>>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> @@ -937,30 +939,6 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>  	return ret;
>>  }
>>  
>> -static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>> -{
>> -	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> -
>> -	if (!tlmi_priv.can_get_bios_selections)
>> -		return -EOPNOTSUPP;
>> -
>> -	return sysfs_emit(buf, "%s\n", setting->possible_values);
>> -}
>> -
>> -static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> -		char *buf)
>> -{
>> -	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> -
>> -	if (setting->possible_values) {
>> -		/* Figure out what setting type is as BIOS does not return this */
>> -		if (strchr(setting->possible_values, ','))
>> -			return sysfs_emit(buf, "enumeration\n");
>> -	}
>> -	/* Anything else is going to be a string */
>> -	return sysfs_emit(buf, "string\n");
>> -}
>> -
>>  static ssize_t current_value_store(struct kobject *kobj,
>>  		struct kobj_attribute *attr,
>>  		const char *buf, size_t count)
>> @@ -1044,14 +1022,46 @@ static ssize_t current_value_store(struct kobject *kobj,
>>  	return ret ?: count;
>>  }
>>  
>> -static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
>> +static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
>> +
>> +static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>> +{
>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> +	return sysfs_emit(buf, "%s\n", setting->possible_values);
>> +}
>>  
>>  static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values);
>>  
>> -static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
>> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> +		char *buf)
>> +{
>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> +	if (setting->possible_values) {
>> +		/* Figure out what setting type is as BIOS does not return this */
>> +		if (strchr(setting->possible_values, ','))
>> +			return sysfs_emit(buf, "enumeration\n");
>> +	}
>> +	/* Anything else is going to be a string */
>> +	return sysfs_emit(buf, "string\n");
>> +}
>
> This patch seems to introduce a lot of churn, is it intentional?
Yes(ish). It got cleaned up as the functions were in a weird order when I introduced the is_visible. The actual changes are very small - but it did make it look messier than it really is.
Is this a big concern? I know it makes the review a bit more painful and my apologies for that.

>>  
>>  static struct kobj_attribute attr_type = __ATTR_RO(type);
>>  
>> +static umode_t attr_is_visible(struct kobject *kobj,
>> +					     struct attribute *attr, int n)
>> +{
>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> +	/* We don't want to display possible_values attributes if not available */
>> +	if (attr == (struct attribute *)&attr_possible_values)
>
> This cast is unsafe, if the struct kobj_attribute order is randomised it
> will break.
>
> You can use
>
> 	if (attr == &attr_possible_values.attr)
>
Ack. Will change.

>> +		if (!setting->possible_values)
>> +			return 0;
>> +
>> +	return attr->mode;
>> +}
>> +
>>  static struct attribute *tlmi_attrs[] = {
>>  	&attr_displ_name.attr,
>>  	&attr_current_val.attr,
>> @@ -1061,6 +1071,7 @@ static struct attribute *tlmi_attrs[] = {
>>  };
>>  
>>  static const struct attribute_group tlmi_attr_group = {
>> +	.is_visible = attr_is_visible,
>>  	.attrs = tlmi_attrs,
>>  };
>>  
>> @@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
>>  			if (ret || !setting->possible_values)
>>  				pr_info("Error retrieving possible values for %d : %s\n",
>>  						i, setting->display_name);
>> +		} else {
>> +			/*
>> +			 * Older Thinkstations don't support the bios_selections API.
>> +			 * Instead they store this as a [Optional:Option1,Option2] section of the
>> +			 * name string.
>> +			 * Try and pull that out if it's available.
>> +			 */
>> +			char *item, *optstart, *optend;
>> +
>> +			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
>> +				optstart = strstr(item, "[Optional:");
>> +				if (optstart) {
>> +					optstart += strlen("[Optional:");
>> +					optend = strstr(optstart, "]");
>> +					if (optend)
>> +						setting->possible_values =
>> +							kstrndup(optstart, optend - optstart, GFP_KERNEL);
>> +				}
>> +			}
>
> The patch now does two things:
> 1) Hide the sysfs attributes if the value is not available
> 2) Extract the value from the description
>
> Maybe it could be split in two?
Sure. I did contemplate that and then ultimately decided it was all from the same intent so left it. But I can split.

>
> Another observation:
> Would it make sense to remove the part
> "[Optional:Option1,Option2]" from the name attribute?
>
I considered this previously and I was concerned about if this could have impacts that I couldn't foresee. The BIOS teams do strange things with this string so I was playing safe and leaving it alone (especially as it differs across the different portfolios)

I know it would be nice to have one standard for everything but sadly that's not the case, and not a battle I can win.

>>  		}
>>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>>  		tlmi_priv.setting[i] = setting;
>> -- 
>> 2.39.2
>>

Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Mark Pearson]

Thanks Barnabas,

On Sat, Mar 18, 2023, at 10:37 AM, Barnabás Pőcze wrote:
> Hi
>
>
> 2023. március 17., péntek 16:46 keltezéssel, Mark Pearson 
> <mpearson-lenovo@squebb.ca> írta:
>
>> firmware-attributes class requires that possible values are delimited
>> using ';' but the Lenovo firmware uses ',' instead.
>> Parse string and replace where appropriate
>> 
>> Thanks to Thomas W for pointing this out.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>> [...]
>> +		/*
>> +		 * firmware-attributes requires that possible_values are separated by ';' but
>> +		 * Lenovo FW uses ','. Replace appropriately.
>> +		 */
>> +		if (setting->possible_values) {
>> +			char *tmp = setting->possible_values;
>> +
>> +			while ((tmp = strchr(tmp, ',')) != NULL)
>> +				*tmp++ = ';';
>> +		}
>
> Please see `strreplace()` from `linux/string.h`.
Ah - I looked for that and didn't find it. Thank you (and will do)

>
>
>> +
>>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>>  		tlmi_priv.setting[i] = setting;
>>  		kfree(item);
>> --
>> 2.39.2
>
>
> Regards,
> Barnabás Pőcze

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Mark Pearson]

On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
> Hi Mark,
>
> please also CC linux-kernel@vger.kernel.org and previous reviewers.

My apologies on previous reviewers - that was a miss on my behalf

I've never cc'd linux-kernel previously - is that a requirement? It's new to me if it is - what's the reason? (that mailing list seems unusable to me from my limited experience...)

Mark
>
> On Fri, Mar 17, 2023 at 11:46:34AM -0400, Mark Pearson wrote:
>> ThinkStation platforms don't support the API to return possible_values
>> but instead embed it in the settings string.
>> 
>> Try and extract this information and set the possible_values attribute
>> appropriately.
>> 
>> If there aren't any values possible then don't display possible_values.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>> Changes in V3:
>>  - Use is_visible attribute to determine if possible_values should be
>>    available
>>  - Code got refactored a bit to make compilation cleaner
>> Changes in V2:
>>  - Move no value for possible_values handling into show function
>>  - use kstrndup for allocating string
>> 
>>  drivers/platform/x86/think-lmi.c | 82 ++++++++++++++++++++++----------
>>  1 file changed, 56 insertions(+), 26 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 5fa5451c4802..d89a1c9bdbf1 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -917,6 +917,8 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
>>  	return sysfs_emit(buf, "%s\n", setting->display_name);
>>  }
>>  
>> +static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
>> +
>>  static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>>  {
>>  	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> @@ -937,30 +939,6 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>  	return ret;
>>  }
>>  
>> -static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>> -{
>> -	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> -
>> -	if (!tlmi_priv.can_get_bios_selections)
>> -		return -EOPNOTSUPP;
>> -
>> -	return sysfs_emit(buf, "%s\n", setting->possible_values);
>> -}
>> -
>> -static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> -		char *buf)
>> -{
>> -	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> -
>> -	if (setting->possible_values) {
>> -		/* Figure out what setting type is as BIOS does not return this */
>> -		if (strchr(setting->possible_values, ','))
>> -			return sysfs_emit(buf, "enumeration\n");
>> -	}
>> -	/* Anything else is going to be a string */
>> -	return sysfs_emit(buf, "string\n");
>> -}
>> -
>>  static ssize_t current_value_store(struct kobject *kobj,
>>  		struct kobj_attribute *attr,
>>  		const char *buf, size_t count)
>> @@ -1044,14 +1022,46 @@ static ssize_t current_value_store(struct kobject *kobj,
>>  	return ret ?: count;
>>  }
>>  
>> -static struct kobj_attribute attr_displ_name = __ATTR_RO(display_name);
>> +static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
>> +
>> +static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>> +{
>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> +	return sysfs_emit(buf, "%s\n", setting->possible_values);
>> +}
>>  
>>  static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values);
>>  
>> -static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
>> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> +		char *buf)
>> +{
>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> +	if (setting->possible_values) {
>> +		/* Figure out what setting type is as BIOS does not return this */
>> +		if (strchr(setting->possible_values, ','))
>> +			return sysfs_emit(buf, "enumeration\n");
>> +	}
>> +	/* Anything else is going to be a string */
>> +	return sysfs_emit(buf, "string\n");
>> +}
>
> This patch seems to introduce a lot of churn, is it intentional?
>>  
>>  static struct kobj_attribute attr_type = __ATTR_RO(type);
>>  
>> +static umode_t attr_is_visible(struct kobject *kobj,
>> +					     struct attribute *attr, int n)
>> +{
>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> +
>> +	/* We don't want to display possible_values attributes if not available */
>> +	if (attr == (struct attribute *)&attr_possible_values)
>
> This cast is unsafe, if the struct kobj_attribute order is randomised it
> will break.
>
> You can use
>
> 	if (attr == &attr_possible_values.attr)
>
>> +		if (!setting->possible_values)
>> +			return 0;
>> +
>> +	return attr->mode;
>> +}
>> +
>>  static struct attribute *tlmi_attrs[] = {
>>  	&attr_displ_name.attr,
>>  	&attr_current_val.attr,
>> @@ -1061,6 +1071,7 @@ static struct attribute *tlmi_attrs[] = {
>>  };
>>  
>>  static const struct attribute_group tlmi_attr_group = {
>> +	.is_visible = attr_is_visible,
>>  	.attrs = tlmi_attrs,
>>  };
>>  
>> @@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
>>  			if (ret || !setting->possible_values)
>>  				pr_info("Error retrieving possible values for %d : %s\n",
>>  						i, setting->display_name);
>> +		} else {
>> +			/*
>> +			 * Older Thinkstations don't support the bios_selections API.
>> +			 * Instead they store this as a [Optional:Option1,Option2] section of the
>> +			 * name string.
>> +			 * Try and pull that out if it's available.
>> +			 */
>> +			char *item, *optstart, *optend;
>> +
>> +			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
>> +				optstart = strstr(item, "[Optional:");
>> +				if (optstart) {
>> +					optstart += strlen("[Optional:");
>> +					optend = strstr(optstart, "]");
>> +					if (optend)
>> +						setting->possible_values =
>> +							kstrndup(optstart, optend - optstart, GFP_KERNEL);
>> +				}
>> +			}
>
> The patch now does two things:
> 1) Hide the sysfs attributes if the value is not available
> 2) Extract the value from the description
>
> Maybe it could be split in two?
>
> Another observation:
> Would it make sense to remove the part
> "[Optional:Option1,Option2]" from the name attribute?
>
>>  		}
>>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>>  		tlmi_priv.setting[i] = setting;
>> -- 
>> 2.39.2
>>

Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Mark Pearson]

Thanks Thomas,

On Sat, Mar 18, 2023, at 12:39 PM, Thomas Weißschuh wrote:
> On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote:
>> firmware-attributes class requires that possible values are delimited
>> using ';' but the Lenovo firmware uses ',' instead.
>> Parse string and replace where appropriate
>> 
>> Thanks to Thomas W for pointing this out.
>> 
>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  Changes in V3: New patch added to the series. No V1 & V2.
>> 
>>  drivers/platform/x86/think-lmi.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index d89a1c9bdbf1..204f1060a533 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>>  
>>  	if (setting->possible_values) {
>>  		/* Figure out what setting type is as BIOS does not return this */
>> -		if (strchr(setting->possible_values, ','))
>> +		if (strchr(setting->possible_values, ';'))
>>  			return sysfs_emit(buf, "enumeration\n");
>
> I think this patch should be earlier in the series.
> So the other patches can work directly from the beginning.
OK. I was avoiding refactoring everything - my git skills are not great. I'll look at doing that.

>
> Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets
> backported.
I wasn't go to label this for stable as it doesn't really have any real world impact that I know of. I figure the stable team have better things to do then backport minor stuff like this especially with it being in a series. If you feel strongly about it I can add it - though I'd rather only do it once the review is complete given the requests to split patches etc. This series has been way messier then I expected.

For the Fixes tag - I don't have anything to reference with this apart from your email. What would I put in there? If you want to raise a bugzilla I'll happy reference that.

Mark

>
>>  	}
>>  	/* Anything else is going to be a string */
>> @@ -1471,6 +1471,17 @@ static int tlmi_analyze(void)
>>  				}
>>  			}
>>  		}
>> +		/*
>> +		 * firmware-attributes requires that possible_values are separated by ';' but
>> +		 * Lenovo FW uses ','. Replace appropriately.
>> +		 */
>> +		if (setting->possible_values) {
>> +			char *tmp = setting->possible_values;
>> +
>> +			while ((tmp = strchr(tmp, ',')) != NULL)
>> +				*tmp++ = ';';
>> +		}
>> +
>>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>>  		tlmi_priv.setting[i] = setting;
>>  		kfree(item);
>> -- 
>> 2.39.2
>>

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Thomas Weißschuh]

On Sat, Mar 18, 2023 at 01:53:33PM -0400, Mark Pearson wrote:
> Thanks Thomas
> 
> On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
> > Hi Mark,
> >
> > please also CC linux-kernel@vger.kernel.org and previous reviewers.
> >
> > On Fri, Mar 17, 2023 at 11:46:34AM -0400, Mark Pearson wrote:
> >> -static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
> >> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> >> +		char *buf)
> >> +{
> >> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> >> +
> >> +	if (setting->possible_values) {
> >> +		/* Figure out what setting type is as BIOS does not return this */
> >> +		if (strchr(setting->possible_values, ','))
> >> +			return sysfs_emit(buf, "enumeration\n");
> >> +	}
> >> +	/* Anything else is going to be a string */
> >> +	return sysfs_emit(buf, "string\n");
> >> +}
> >
> > This patch seems to introduce a lot of churn, is it intentional?
> Yes(ish). It got cleaned up as the functions were in a weird order when I introduced the is_visible. The actual changes are very small - but it did make it look messier than it really is.
> Is this a big concern? I know it makes the review a bit more painful and my apologies for that.

Not a big concern. The shuffling around could be done in a dedicated
patch that explicitly only moves code around.

> >> @@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
> >>  			if (ret || !setting->possible_values)
> >>  				pr_info("Error retrieving possible values for %d : %s\n",
> >>  						i, setting->display_name);
> >> +		} else {
> >> +			/*
> >> +			 * Older Thinkstations don't support the bios_selections API.
> >> +			 * Instead they store this as a [Optional:Option1,Option2] section of the
> >> +			 * name string.
> >> +			 * Try and pull that out if it's available.
> >> +			 */
> >> +			char *item, *optstart, *optend;
> >> +
> >> +			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
> >> +				optstart = strstr(item, "[Optional:");
> >> +				if (optstart) {
> >> +					optstart += strlen("[Optional:");
> >> +					optend = strstr(optstart, "]");
> >> +					if (optend)
> >> +						setting->possible_values =
> >> +							kstrndup(optstart, optend - optstart, GFP_KERNEL);
> >> +				}
> >> +			}
> >
> > The patch now does two things:
> > 1) Hide the sysfs attributes if the value is not available
> > 2) Extract the value from the description
> >
> > Maybe it could be split in two?
> Sure. I did contemplate that and then ultimately decided it was all from the same intent so left it. But I can split.

Would look nicer to me, but it's only one opinion.

> >
> > Another observation:
> > Would it make sense to remove the part
> > "[Optional:Option1,Option2]" from the name attribute?
> >
> I considered this previously and I was concerned about if this could
> have impacts that I couldn't foresee. The BIOS teams do strange things
> with this string so I was playing safe and leaving it alone
> (especially as it differs across the different portfolios)
> 
> I know it would be nice to have one standard for everything but sadly that's not the case, and not a battle I can win.

Fair enough.

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Thomas Weißschuh]

On Sat, Mar 18, 2023 at 01:59:38PM -0400, Mark Pearson wrote:
> On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
> > Hi Mark,
> >
> > please also CC linux-kernel@vger.kernel.org and previous reviewers.
> 
> My apologies on previous reviewers - that was a miss on my behalf
> 
> I've never cc'd linux-kernel previously - is that a requirement? It's
> new to me if it is - what's the reason? (that mailing list seems
> unusable to me from my limited experience...)

The wording in Documentation/process/submitting-patches.rst is indeed
unclear about always Cc'ing linux-kernel.

My arguments for it are:

- People can look at the linux-kernel archives to see what's going on
  all over the kernel.  (I do that sometimes myself)
  Also it makes it easier to search on lore.kernel.org on linux-specific
  messages/patches. The /all/ archives also contains other projects.

- Some bots are processing proposed patches from mailing lists.
  These are not subscribed to all subsystem lists.

- The b4 tool does it.

- Greg does it:
  https://lore.kernel.org/lkml/20230313182918.1312597-5-gregkh@linuxfoundation.org/

Thomas

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Mark Pearson]

On Sat, Mar 18, 2023, at 8:01 PM, Thomas Weißschuh wrote:
> On Sat, Mar 18, 2023 at 01:59:38PM -0400, Mark Pearson wrote:
>> On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
>> > Hi Mark,
>> >
>> > please also CC linux-kernel@vger.kernel.org and previous reviewers.
>> 
>> My apologies on previous reviewers - that was a miss on my behalf
>> 
>> I've never cc'd linux-kernel previously - is that a requirement? It's
>> new to me if it is - what's the reason? (that mailing list seems
>> unusable to me from my limited experience...)
>
> The wording in Documentation/process/submitting-patches.rst is indeed
> unclear about always Cc'ing linux-kernel.
>
> My arguments for it are:
>
> - People can look at the linux-kernel archives to see what's going on
>   all over the kernel.  (I do that sometimes myself)
>   Also it makes it easier to search on lore.kernel.org on linux-specific
>   messages/patches. The /all/ archives also contains other projects.
>
> - Some bots are processing proposed patches from mailing lists.
>   These are not subscribed to all subsystem lists.
>
> - The b4 tool does it.
>
> - Greg does it:
>   
> https://lore.kernel.org/lkml/20230313182918.1312597-5-gregkh@linuxfoundation.org/
>
> Thomas

All good reasons :) I will start doing that going forwards. 
Thanks for the clarification.
Mark

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Mark Pearson]

On Sat, Mar 18, 2023, at 7:52 PM, Thomas Weißschuh wrote:
> On Sat, Mar 18, 2023 at 01:53:33PM -0400, Mark Pearson wrote:
>> Thanks Thomas
>> 
>> On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
>> > Hi Mark,
>> >
>> > please also CC linux-kernel@vger.kernel.org and previous reviewers.
>> >
>> > On Fri, Mar 17, 2023 at 11:46:34AM -0400, Mark Pearson wrote:
>> >> -static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
>> >> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> >> +		char *buf)
>> >> +{
>> >> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> >> +
>> >> +	if (setting->possible_values) {
>> >> +		/* Figure out what setting type is as BIOS does not return this */
>> >> +		if (strchr(setting->possible_values, ','))
>> >> +			return sysfs_emit(buf, "enumeration\n");
>> >> +	}
>> >> +	/* Anything else is going to be a string */
>> >> +	return sysfs_emit(buf, "string\n");
>> >> +}
>> >
>> > This patch seems to introduce a lot of churn, is it intentional?
>> Yes(ish). It got cleaned up as the functions were in a weird order when I introduced the is_visible. The actual changes are very small - but it did make it look messier than it really is.
>> Is this a big concern? I know it makes the review a bit more painful and my apologies for that.
>
> Not a big concern. The shuffling around could be done in a dedicated
> patch that explicitly only moves code around.
>
>> >> @@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
>> >>  			if (ret || !setting->possible_values)
>> >>  				pr_info("Error retrieving possible values for %d : %s\n",
>> >>  						i, setting->display_name);
>> >> +		} else {
>> >> +			/*
>> >> +			 * Older Thinkstations don't support the bios_selections API.
>> >> +			 * Instead they store this as a [Optional:Option1,Option2] section of the
>> >> +			 * name string.
>> >> +			 * Try and pull that out if it's available.
>> >> +			 */
>> >> +			char *item, *optstart, *optend;
>> >> +
>> >> +			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
>> >> +				optstart = strstr(item, "[Optional:");
>> >> +				if (optstart) {
>> >> +					optstart += strlen("[Optional:");
>> >> +					optend = strstr(optstart, "]");
>> >> +					if (optend)
>> >> +						setting->possible_values =
>> >> +							kstrndup(optstart, optend - optstart, GFP_KERNEL);
>> >> +				}
>> >> +			}
>> >
>> > The patch now does two things:
>> > 1) Hide the sysfs attributes if the value is not available
>> > 2) Extract the value from the description
>> >
>> > Maybe it could be split in two?
>> Sure. I did contemplate that and then ultimately decided it was all from the same intent so left it. But I can split.
>
> Would look nicer to me, but it's only one opinion.

I have worked through this and it is nicer. Next version will be split (and I unwound some of the code re-org too).
I'm going to hold off a couple of days before pushing the changes for review in case there are other pieces of feedback.

Mark

Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Thomas Weißschuh]

On Sat, Mar 18, 2023 at 02:06:28PM -0400, Mark Pearson wrote:
> Thanks Thomas,
> 
> On Sat, Mar 18, 2023, at 12:39 PM, Thomas Weißschuh wrote:
> > On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote:
> >> firmware-attributes class requires that possible values are delimited
> >> using ';' but the Lenovo firmware uses ',' instead.
> >> Parse string and replace where appropriate
> >> 
> >> Thanks to Thomas W for pointing this out.

This could also be a

Reported-by: Thomas Weißschuh <linux@weissschuh.net>

> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> >> ---
> >>  Changes in V3: New patch added to the series. No V1 & V2.
> >> 
> >>  drivers/platform/x86/think-lmi.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> >> index d89a1c9bdbf1..204f1060a533 100644
> >> --- a/drivers/platform/x86/think-lmi.c
> >> +++ b/drivers/platform/x86/think-lmi.c
> >> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> >>  
> >>  	if (setting->possible_values) {
> >>  		/* Figure out what setting type is as BIOS does not return this */
> >> -		if (strchr(setting->possible_values, ','))
> >> +		if (strchr(setting->possible_values, ';'))
> >>  			return sysfs_emit(buf, "enumeration\n");
> >
> > I think this patch should be earlier in the series.
> > So the other patches can work directly from the beginning.
> OK. I was avoiding refactoring everything - my git skills are not great. I'll look at doing that.

I would do it like this with an interactive rebase:

b # apply the generic parts of "platform/x86: think-lmi: use correct possible_values delimters"
pick c2fbd30a7b15 platform/x86: think-lmi: add missing type attribute
pick 644923d17048 platform/x86: think-lmi: Add possible_values for ThinkStation
f a92fa3cda0d6 platform/x86: think-lmi: use correct possible_values delimters

> > Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets
> > backported.

> I wasn't go to label this for stable as it doesn't really have any
> real world impact that I know of. I figure the stable team have better
> things to do then backport minor stuff like this especially with it
> being in a series. If you feel strongly about it I can add it - though
> I'd rather only do it once the review is complete given the requests
> to split patches etc. This series has been way messier then I
> expected.

The -stable process should be automated with the proper stable Cc.

Given that this technically breaks ABI it may better to keep it out of
stable, though.

FYI I looked at the only user of this ABI that I know, fwupd, and it
should gracefully handle this change.
It accepts both ',' and ';' as separator.

> For the Fixes tag - I don't have anything to reference with this apart
> from your email. What would I put in there? If you want to raise a
> bugzilla I'll happy reference that.

The Fixes tag refers to the original commit that introduced the fixed
issue.
In this case it would be the commit adding the think-lmi driver:

Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")

Thomas

Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

[Mark Pearson]

On Sat, Mar 18, 2023, at 8:11 PM, Thomas Weißschuh wrote:
> On Sat, Mar 18, 2023 at 02:06:28PM -0400, Mark Pearson wrote:
>> Thanks Thomas,
>> 
>> On Sat, Mar 18, 2023, at 12:39 PM, Thomas Weißschuh wrote:
>> > On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote:
>> >> firmware-attributes class requires that possible values are delimited
>> >> using ';' but the Lenovo firmware uses ',' instead.
>> >> Parse string and replace where appropriate
>> >> 
>> >> Thanks to Thomas W for pointing this out.
>
> This could also be a
>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
Good point - will update

>
>> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> >> ---
>> >>  Changes in V3: New patch added to the series. No V1 & V2.
>> >> 
>> >>  drivers/platform/x86/think-lmi.c | 13 ++++++++++++-
>> >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> >> index d89a1c9bdbf1..204f1060a533 100644
>> >> --- a/drivers/platform/x86/think-lmi.c
>> >> +++ b/drivers/platform/x86/think-lmi.c
>> >> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> >>  
>> >>  	if (setting->possible_values) {
>> >>  		/* Figure out what setting type is as BIOS does not return this */
>> >> -		if (strchr(setting->possible_values, ','))
>> >> +		if (strchr(setting->possible_values, ';'))
>> >>  			return sysfs_emit(buf, "enumeration\n");
>> >
>> > I think this patch should be earlier in the series.
>> > So the other patches can work directly from the beginning.
>> OK. I was avoiding refactoring everything - my git skills are not great. I'll look at doing that.
>
> I would do it like this with an interactive rebase:
>
> b # apply the generic parts of "platform/x86: think-lmi: use correct 
> possible_values delimters"
> pick c2fbd30a7b15 platform/x86: think-lmi: add missing type attribute
> pick 644923d17048 platform/x86: think-lmi: Add possible_values for 
> ThinkStation
> f a92fa3cda0d6 platform/x86: think-lmi: use correct possible_values 
> delimters

Thank you.
I have already done this and I dropped the two last ones and then just created them manually but with an extra commit thrown in. It worked out pretty well and let me clean up other pieces at the same time. 
Slowly learning...appreciate everyone's patience. Every time I think I have this figured a review process teaches me otherwise :)

>
>> > Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets
>> > backported.
>
>> I wasn't go to label this for stable as it doesn't really have any
>> real world impact that I know of. I figure the stable team have better
>> things to do then backport minor stuff like this especially with it
>> being in a series. If you feel strongly about it I can add it - though
>> I'd rather only do it once the review is complete given the requests
>> to split patches etc. This series has been way messier then I
>> expected.
>
> The -stable process should be automated with the proper stable Cc.
>
> Given that this technically breaks ABI it may better to keep it out of
> stable, though.
>
> FYI I looked at the only user of this ABI that I know, fwupd, and it
> should gracefully handle this change.
> It accepts both ',' and ';' as separator.
>
>> For the Fixes tag - I don't have anything to reference with this apart
>> from your email. What would I put in there? If you want to raise a
>> bugzilla I'll happy reference that.
>
> The Fixes tag refers to the original commit that introduced the fixed
> issue.
> In this case it would be the commit adding the think-lmi driver:
>
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface 
> support on Lenovo platforms")
>
> Thomas
Ah - got it. Will add. Thanks.

Mark

Re: [PATCH v3 2/3] platform/x86: think-lmi: Add possible_values for ThinkStation

[Hans de Goede]

Hi,

On 3/19/23 01:08, Mark Pearson wrote:
> On Sat, Mar 18, 2023, at 7:52 PM, Thomas Weißschuh wrote:
>> On Sat, Mar 18, 2023 at 01:53:33PM -0400, Mark Pearson wrote:
>>> Thanks Thomas
>>>
>>> On Sat, Mar 18, 2023, at 12:35 PM, Thomas Weißschuh wrote:
>>>> Hi Mark,
>>>>
>>>> please also CC linux-kernel@vger.kernel.org and previous reviewers.
>>>>
>>>> On Fri, Mar 17, 2023 at 11:46:34AM -0400, Mark Pearson wrote:
>>>>> -static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600);
>>>>> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>>>>> +		char *buf)
>>>>> +{
>>>>> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>>>>> +
>>>>> +	if (setting->possible_values) {
>>>>> +		/* Figure out what setting type is as BIOS does not return this */
>>>>> +		if (strchr(setting->possible_values, ','))
>>>>> +			return sysfs_emit(buf, "enumeration\n");
>>>>> +	}
>>>>> +	/* Anything else is going to be a string */
>>>>> +	return sysfs_emit(buf, "string\n");
>>>>> +}
>>>>
>>>> This patch seems to introduce a lot of churn, is it intentional?
>>> Yes(ish). It got cleaned up as the functions were in a weird order when I introduced the is_visible. The actual changes are very small - but it did make it look messier than it really is.
>>> Is this a big concern? I know it makes the review a bit more painful and my apologies for that.
>>
>> Not a big concern. The shuffling around could be done in a dedicated
>> patch that explicitly only moves code around.
>>
>>>>> @@ -1440,6 +1451,25 @@ static int tlmi_analyze(void)
>>>>>  			if (ret || !setting->possible_values)
>>>>>  				pr_info("Error retrieving possible values for %d : %s\n",
>>>>>  						i, setting->display_name);
>>>>> +		} else {
>>>>> +			/*
>>>>> +			 * Older Thinkstations don't support the bios_selections API.
>>>>> +			 * Instead they store this as a [Optional:Option1,Option2] section of the
>>>>> +			 * name string.
>>>>> +			 * Try and pull that out if it's available.
>>>>> +			 */
>>>>> +			char *item, *optstart, *optend;
>>>>> +
>>>>> +			if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
>>>>> +				optstart = strstr(item, "[Optional:");
>>>>> +				if (optstart) {
>>>>> +					optstart += strlen("[Optional:");
>>>>> +					optend = strstr(optstart, "]");
>>>>> +					if (optend)
>>>>> +						setting->possible_values =
>>>>> +							kstrndup(optstart, optend - optstart, GFP_KERNEL);
>>>>> +				}
>>>>> +			}
>>>>
>>>> The patch now does two things:
>>>> 1) Hide the sysfs attributes if the value is not available
>>>> 2) Extract the value from the description
>>>>
>>>> Maybe it could be split in two?
>>> Sure. I did contemplate that and then ultimately decided it was all from the same intent so left it. But I can split.
>>
>> Would look nicer to me, but it's only one opinion.
> 
> I have worked through this and it is nicer. Next version will be split (and I unwound some of the code re-org too).
> I'm going to hold off a couple of days before pushing the changes for review in case there are other pieces of feedback.

Thomas, many thanks for all the reviews!

Mark, since Thomas is doing such a great job of reviewing this patch-set, don't expect any remarks from me before you post the next version. IOW if the next version is ready, don't wait for my feedback before submitting it :)

Regards,

Hans


> 
> Mark
> 

RE: [PATCH v3 1/3] platform/x86: think-lmi: add missing type attribute

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Mark Pearson <mpearson-lenovo@squebb.ca>
> Sent: Friday, March 17, 2023 10:47
> To: mpearson-lenovo@squebb.ca
> Cc: hdegoede@redhat.com; markgross@kernel.org;
> markpearson@lenovo.com; platform-driver-x86@vger.kernel.org
> Subject: [PATCH v3 1/3] platform/x86: think-lmi: add missing type attribute
> 
> This driver was missing the mandatory type attribute...oops.
> 
> Add it in along with logic to determine whether the attribute is an
> enumeration type or a string by parsing the possible_values attribute.
> 
> Upstream bug https://bugzilla.kernel.org/show_bug.cgi?id=216460'

The tag you should use here is "Link".
IOW:

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216460

> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
> Changes in v3:
>  - Rebased on latest pdx86, review_hans branch
> Changes in v2:
>  - Simplify the code and move type determination into show function
>  - Don't use Fixes with URL in commit info
> 
>  drivers/platform/x86/think-lmi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-
> lmi.c
> index 86b33b74519b..5fa5451c4802 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -947,6 +947,20 @@ static ssize_t possible_values_show(struct kobject
> *kobj, struct kobj_attribute
>  	return sysfs_emit(buf, "%s\n", setting->possible_values);
>  }
> 
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> +		char *buf)
> +{
> +	struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> +
> +	if (setting->possible_values) {
> +		/* Figure out what setting type is as BIOS does not return this
> */
> +		if (strchr(setting->possible_values, ','))
> +			return sysfs_emit(buf, "enumeration\n");
> +	}
> +	/* Anything else is going to be a string */
> +	return sysfs_emit(buf, "string\n");
> +}
> +
>  static ssize_t current_value_store(struct kobject *kobj,
>  		struct kobj_attribute *attr,
>  		const char *buf, size_t count)
> @@ -1036,10 +1050,13 @@ static struct kobj_attribute attr_possible_values
> = __ATTR_RO(possible_values);
> 
>  static struct kobj_attribute attr_current_val =
> __ATTR_RW_MODE(current_value, 0600);
> 
> +static struct kobj_attribute attr_type = __ATTR_RO(type);
> +
>  static struct attribute *tlmi_attrs[] = {
>  	&attr_displ_name.attr,
>  	&attr_current_val.attr,
>  	&attr_possible_values.attr,
> +	&attr_type.attr,
>  	NULL
>  };
> 
> --
> 2.39.2