| Message ID | 20230313184541.193733-2-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] platform/x86: think-lmi: add missing type attribute | expand |
Hi Mark, some more remarks, sorry not seeing this earlier. On Mon, Mar 13, 2023 at 02:45:41PM -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. > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > Changes in V2: > - Move no value for possible_values handling into show function > - use kstrndup for allocating string > > drivers/platform/x86/think-lmi.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 5fa5451c4802..7dd8f72176f5 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -941,10 +941,9 @@ 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); > + if (setting->possible_values) > + return sysfs_emit(buf, "%s\n", setting->possible_values); > + return sysfs_emit(buf, "not available\n"); > } As the attribute "possible_values" is not mandatory it should be possible to hide it completely with an is_visible callback. This would indicate absence clearer than a magic value. > static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > @@ -1440,6 +1439,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. > + */ The values in possible_values are supposed to be separated by semi-colons, not commas. I don't know how this affects the existing parts of this driver but it affects both patches of this series. > + 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 >
Hi Thomas, On Mon, Mar 13, 2023, at 7:58 PM, Thomas Weißschuh wrote: > Hi Mark, > > some more remarks, sorry not seeing this earlier. No worries :) I appreciate the reviews. > > On Mon, Mar 13, 2023 at 02:45:41PM -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. >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> --- >> Changes in V2: >> - Move no value for possible_values handling into show function >> - use kstrndup for allocating string >> >> drivers/platform/x86/think-lmi.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> index 5fa5451c4802..7dd8f72176f5 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -941,10 +941,9 @@ 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); >> + if (setting->possible_values) >> + return sysfs_emit(buf, "%s\n", setting->possible_values); >> + return sysfs_emit(buf, "not available\n"); >> } > > As the attribute "possible_values" is not mandatory it should be > possible to hide it completely with an is_visible callback. > > This would indicate absence clearer than a magic value. Agreed - I like it. I'll look into implementing that. Thank you. > >> static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >> @@ -1440,6 +1439,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. >> + */ > > The values in possible_values are supposed to be separated by > semi-colons, not commas. > I don't know how this affects the existing parts of this driver but it > affects both patches of this series. Good point, and I'd missed that. The current string is returned directly from the BIOS, so I'll have to manipulate it. I would ask the BIOS team to change it but it will take forever and could impact Windows - so better to tweak it in the driver. I'll do this as a third patch I think. Thanks Mark
Hi, On 3/14/23 21:14, Mark Pearson wrote: > Hi Thomas, > > On Mon, Mar 13, 2023, at 7:58 PM, Thomas Weißschuh wrote: >> Hi Mark, >> >> some more remarks, sorry not seeing this earlier. > > No worries :) I appreciate the reviews. > >> >> On Mon, Mar 13, 2023 at 02:45:41PM -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. >>> >>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >>> --- >>> Changes in V2: >>> - Move no value for possible_values handling into show function >>> - use kstrndup for allocating string >>> >>> drivers/platform/x86/think-lmi.c | 26 ++++++++++++++++++++++---- >>> 1 file changed, 22 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >>> index 5fa5451c4802..7dd8f72176f5 100644 >>> --- a/drivers/platform/x86/think-lmi.c >>> +++ b/drivers/platform/x86/think-lmi.c >>> @@ -941,10 +941,9 @@ 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); >>> + if (setting->possible_values) >>> + return sysfs_emit(buf, "%s\n", setting->possible_values); >>> + return sysfs_emit(buf, "not available\n"); >>> } >> >> As the attribute "possible_values" is not mandatory it should be >> possible to hide it completely with an is_visible callback. >> >> This would indicate absence clearer than a magic value. > > Agreed - I like it. I'll look into implementing that. Thank you. > >> >>> static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >>> @@ -1440,6 +1439,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. >>> + */ >> >> The values in possible_values are supposed to be separated by >> semi-colons, not commas. >> I don't know how this affects the existing parts of this driver but it >> affects both patches of this series. > > Good point, and I'd missed that. > The current string is returned directly from the BIOS, so I'll have to manipulate it. I would ask the BIOS team to change it but it will take forever and could impact Windows - so better to tweak it in the driver. > I'll do this as a third patch I think. Yes putting this in a separate / third patch seems best. Regards, Hans
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index 5fa5451c4802..7dd8f72176f5 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -941,10 +941,9 @@ 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); + if (setting->possible_values) + return sysfs_emit(buf, "%s\n", setting->possible_values); + return sysfs_emit(buf, "not available\n"); } static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, @@ -1440,6 +1439,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. Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- Changes in V2: - Move no value for possible_values handling into show function - use kstrndup for allocating string drivers/platform/x86/think-lmi.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)