diff mbox series

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

Message ID 20230317154635.39692-2-mpearson-lenovo@squebb.ca (mailing list archive)
State Changes Requested, archived
Headers show
Series [v3,1/3] platform/x86: think-lmi: add missing type attribute | expand

Commit Message

Mark Pearson March 17, 2023, 3:46 p.m. UTC
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(-)

Comments

Thomas Weißschuh March 18, 2023, 4:35 p.m. UTC | #1
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
>
Mark Pearson March 18, 2023, 5:53 p.m. UTC | #2
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
>>
Mark Pearson March 18, 2023, 5:59 p.m. UTC | #3
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
>>
Thomas Weißschuh March 18, 2023, 11:52 p.m. UTC | #4
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.
Thomas Weißschuh March 19, 2023, 12:01 a.m. UTC | #5
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
Mark Pearson March 19, 2023, 12:04 a.m. UTC | #6
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
Mark Pearson March 19, 2023, 12:08 a.m. UTC | #7
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
Hans de Goede March 19, 2023, 9:34 a.m. UTC | #8
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
>
diff mbox series

Patch

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;