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