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

[PATCH 1/2] 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.

Some platforms (and some attributes) don't return possible_values so to
prevent trying to scan NULL strings mark these as "N/A".

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460
Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++---
 drivers/platform/x86/think-lmi.h |  6 ++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 86b33b74519b..495a5e045069 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -941,12 +941,18 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute
 {
 	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);
+
+	return sysfs_emit(buf, "%s\n",
+			setting->type == TLMI_ENUMERATION ? "enumeration" : "string");
+}
+
 static ssize_t current_value_store(struct kobject *kobj,
 		struct kobj_attribute *attr,
 		const char *buf, size_t count)
@@ -1036,10 +1042,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
 };
 
@@ -1424,6 +1433,17 @@ static int tlmi_analyze(void)
 				pr_info("Error retrieving possible values for %d : %s\n",
 						i, setting->display_name);
 		}
+		/* If we don't have a possible value mark as N/A */
+		if (!setting->possible_values) {
+			setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL);
+			sprintf(setting->possible_values, "N/A");
+		}
+		/* Figure out what setting type is as BIOS does not return this */
+		if (strchr(setting->possible_values, ','))
+			setting->type = TLMI_ENUMERATION;
+		else
+			setting->type = TLMI_STRING;
+
 		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
 		tlmi_priv.setting[i] = setting;
 		kfree(item);
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index 4daba6151cd6..b76edcb9b1af 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -27,6 +27,11 @@ enum level_option {
 	TLMI_LEVEL_MASTER,
 };
 
+enum attr_type {
+	TLMI_ENUMERATION,
+	TLMI_STRING,
+};
+
 /* password configuration details */
 struct tlmi_pwdcfg_core {
 	uint32_t password_mode;
@@ -73,6 +78,7 @@ struct tlmi_attr_setting {
 	int index;
 	char display_name[TLMI_SETTINGS_MAXLEN];
 	char *possible_values;
+	enum attr_type type;
 };
 
 struct think_lmi {
-- 
2.39.1

[PATCH 2/2] 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.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/platform/x86/think-lmi.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 495a5e045069..52f3472fd1e0 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -1432,6 +1432,29 @@ 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 =
+							kmalloc(optend - optstart, GFP_KERNEL);
+						strncpy(setting->possible_values,
+								optstart, optend - optstart);
+						setting->possible_values[optend - optstart] = '\0';
+					}
+				}
+			}
 		}
 		/* If we don't have a possible value mark as N/A */
 		if (!setting->possible_values) {
-- 
2.39.1

Re: [PATCH 1/2] platform/x86: think-lmi: add missing type attribute

[Thomas Weißschuh]

On Sat, Mar 11, 2023 at 09:46:34PM -0500, Mark Pearson wrote:
> 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.
> 
> Some platforms (and some attributes) don't return possible_values so to
> prevent trying to scan NULL strings mark these as "N/A".
> 
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460

Afaik Fixes: is only for references to commits.
Instead a Reported-by/Link would be better.

> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++---
>  drivers/platform/x86/think-lmi.h |  6 ++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 86b33b74519b..495a5e045069 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -941,12 +941,18 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute
>  {
>  	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);
> +
> +	return sysfs_emit(buf, "%s\n",
> +			setting->type == TLMI_ENUMERATION ? "enumeration" : "string");
> +}
> +
>  static ssize_t current_value_store(struct kobject *kobj,
>  		struct kobj_attribute *attr,
>  		const char *buf, size_t count)
> @@ -1036,10 +1042,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
>  };
>  
> @@ -1424,6 +1433,17 @@ static int tlmi_analyze(void)
>  				pr_info("Error retrieving possible values for %d : %s\n",
>  						i, setting->display_name);
>  		}
> +		/* If we don't have a possible value mark as N/A */
> +		if (!setting->possible_values) {
> +			setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL);

kmalloc() can fail.

> +			sprintf(setting->possible_values, "N/A");

This writes the '\0' out of bounds?

kmalloc() and sprintf() could be replaced with kstrdup().

Instead of having to do allocations, check for failure, worry about how
sysfs_emit() will handle the NULL it would be easier to just check of
NULL inside possible_values_show() and fall back to N/A there.

> +		}
> +		/* Figure out what setting type is as BIOS does not return this */
> +		if (strchr(setting->possible_values, ','))

possible_values could be NULL if the sprintf would not have dereferenced
it before.

> +			setting->type = TLMI_ENUMERATION;
> +		else
> +			setting->type = TLMI_STRING;
> +

Is it worth introducing a new enum and field in struct
tlmi_attr_setting?
The check could also be done directly in type_show().
(with a NULL-check).

>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>  		tlmi_priv.setting[i] = setting;
>  		kfree(item);
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index 4daba6151cd6..b76edcb9b1af 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -27,6 +27,11 @@ enum level_option {
>  	TLMI_LEVEL_MASTER,
>  };
>  
> +enum attr_type {
> +	TLMI_ENUMERATION,
> +	TLMI_STRING,
> +};
> +
>  /* password configuration details */
>  struct tlmi_pwdcfg_core {
>  	uint32_t password_mode;
> @@ -73,6 +78,7 @@ struct tlmi_attr_setting {
>  	int index;
>  	char display_name[TLMI_SETTINGS_MAXLEN];
>  	char *possible_values;
> +	enum attr_type type;
>  };
>  
>  struct think_lmi {
> -- 
> 2.39.1
> 

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

[Thomas Weißschuh]

On Sat, Mar 11, 2023 at 09:46:35PM -0500, 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.
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/platform/x86/think-lmi.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 495a5e045069..52f3472fd1e0 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -1432,6 +1432,29 @@ 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 =
> +							kmalloc(optend - optstart, GFP_KERNEL);
> +						strncpy(setting->possible_values,
> +								optstart, optend - optstart);
> +						setting->possible_values[optend - optstart] = '\0';

Same as for patch 1.

kmalloc() can fail, etc.

kstrndup() would be much easier.

> +					}
> +				}
> +			}
>  		}
>  		/* If we don't have a possible value mark as N/A */
>  		if (!setting->possible_values) {
> -- 
> 2.39.1
> 

Re: [PATCH 1/2] platform/x86: think-lmi: add missing type attribute

[Mark Pearson]

Apologies for the duplication, I forgot to set email format as text so it got bounced by the mailing list. Resending.
Mark

On Sat, Mar 11, 2023, at 10:44 PM, Mark Pearson wrote:
> Thanks Thomas
> 
> On Sat, Mar 11, 2023, at 10:33 PM, Thomas Weißschuh wrote:
>> On Sat, Mar 11, 2023 at 09:46:34PM -0500, Mark Pearson wrote:
>> > 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.
>> > 
>> > Some platforms (and some attributes) don't return possible_values so to
>> > prevent trying to scan NULL strings mark these as "N/A".
>> > 
>> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460
>> 
>> Afaik Fixes: is only for references to commits.
>> Instead a Reported-by/Link would be better.
> Ah - thanks. My bad.
> 
>> 
>> > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> > ---
>> >  drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++---
>> >  drivers/platform/x86/think-lmi.h |  6 ++++++
>> >  2 files changed, 29 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> > index 86b33b74519b..495a5e045069 100644
>> > --- a/drivers/platform/x86/think-lmi.c
>> > +++ b/drivers/platform/x86/think-lmi.c
>> > @@ -941,12 +941,18 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute
>> >  {
>> >  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);
>> > +
>> > + return sysfs_emit(buf, "%s\n",
>> > + setting->type == TLMI_ENUMERATION ? "enumeration" : "string");
>> > +}
>> > +
>> >  static ssize_t current_value_store(struct kobject *kobj,
>> >  struct kobj_attribute *attr,
>> >  const char *buf, size_t count)
>> > @@ -1036,10 +1042,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
>> >  };
>> >  
>> > @@ -1424,6 +1433,17 @@ static int tlmi_analyze(void)
>> >  pr_info("Error retrieving possible values for %d : %s\n",
>> >  i, setting->display_name);
>> >  }
>> > + /* If we don't have a possible value mark as N/A */
>> > + if (!setting->possible_values) {
>> > + setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL);
>> 
>> kmalloc() can fail.
>> 
>> > + sprintf(setting->possible_values, "N/A");
>> 
>> This writes the '\0' out of bounds?
>> 
>> kmalloc() and sprintf() could be replaced with kstrdup().
>> 
>> Instead of having to do allocations, check for failure, worry about how
>> sysfs_emit() will handle the NULL it would be easier to just check of
>> NULL inside possible_values_show() and fall back to N/A there.
> Good point - that would be better. I'll update.
> 
>> 
>> > + }
>> > + /* Figure out what setting type is as BIOS does not return this */
>> > + if (strchr(setting->possible_values, ','))
>> 
>> possible_values could be NULL if the sprintf would not have dereferenced
>> it before.
> Agreed. This was part of the reason I'd put in the N/A to cover for that case (so it should never be NULL). But I'll revisit this.
> 
>> 
>> > + setting->type = TLMI_ENUMERATION;
>> > + else
>> > + setting->type = TLMI_STRING;
>> > +
>> 
>> Is it worth introducing a new enum and field in struct
>> tlmi_attr_setting?
>> The check could also be done directly in type_show().
>> (with a NULL-check).
> Ack, this makes sense. I'll look at doing that.
> 
> Many thanks for the review
> Mark