| Message ID | 20230317154635.39692-3-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 2023. március 17., péntek 16:46 keltezéssel, Mark Pearson <mpearson-lenovo@squebb.ca> írta: > firmware-attributes class requires that possible values are delimited > using ';' but the Lenovo firmware uses ',' instead. > Parse string and replace where appropriate > > Thanks to Thomas W for pointing this out. > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > [...] > + /* > + * firmware-attributes requires that possible_values are separated by ';' but > + * Lenovo FW uses ','. Replace appropriately. > + */ > + if (setting->possible_values) { > + char *tmp = setting->possible_values; > + > + while ((tmp = strchr(tmp, ',')) != NULL) > + *tmp++ = ';'; > + } Please see `strreplace()` from `linux/string.h`. > + > kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); > tlmi_priv.setting[i] = setting; > kfree(item); > -- > 2.39.2 Regards, Barnabás Pőcze
On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote: > firmware-attributes class requires that possible values are delimited > using ';' but the Lenovo firmware uses ',' instead. > Parse string and replace where appropriate > > Thanks to Thomas W for pointing this out. > > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > Changes in V3: New patch added to the series. No V1 & V2. > > drivers/platform/x86/think-lmi.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index d89a1c9bdbf1..204f1060a533 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > > if (setting->possible_values) { > /* Figure out what setting type is as BIOS does not return this */ > - if (strchr(setting->possible_values, ',')) > + if (strchr(setting->possible_values, ';')) > return sysfs_emit(buf, "enumeration\n"); I think this patch should be earlier in the series. So the other patches can work directly from the beginning. Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets backported. > } > /* Anything else is going to be a string */ > @@ -1471,6 +1471,17 @@ static int tlmi_analyze(void) > } > } > } > + /* > + * firmware-attributes requires that possible_values are separated by ';' but > + * Lenovo FW uses ','. Replace appropriately. > + */ > + if (setting->possible_values) { > + char *tmp = setting->possible_values; > + > + while ((tmp = strchr(tmp, ',')) != NULL) > + *tmp++ = ';'; > + } > + > kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); > tlmi_priv.setting[i] = setting; > kfree(item); > -- > 2.39.2 >
Thanks Barnabas, On Sat, Mar 18, 2023, at 10:37 AM, Barnabás Pőcze wrote: > Hi > > > 2023. március 17., péntek 16:46 keltezéssel, Mark Pearson > <mpearson-lenovo@squebb.ca> írta: > >> firmware-attributes class requires that possible values are delimited >> using ';' but the Lenovo firmware uses ',' instead. >> Parse string and replace where appropriate >> >> Thanks to Thomas W for pointing this out. >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> --- >> [...] >> + /* >> + * firmware-attributes requires that possible_values are separated by ';' but >> + * Lenovo FW uses ','. Replace appropriately. >> + */ >> + if (setting->possible_values) { >> + char *tmp = setting->possible_values; >> + >> + while ((tmp = strchr(tmp, ',')) != NULL) >> + *tmp++ = ';'; >> + } > > Please see `strreplace()` from `linux/string.h`. Ah - I looked for that and didn't find it. Thank you (and will do) > > >> + >> kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); >> tlmi_priv.setting[i] = setting; >> kfree(item); >> -- >> 2.39.2 > > > Regards, > Barnabás Pőcze
Thanks Thomas, On Sat, Mar 18, 2023, at 12:39 PM, Thomas Weißschuh wrote: > On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote: >> firmware-attributes class requires that possible values are delimited >> using ';' but the Lenovo firmware uses ',' instead. >> Parse string and replace where appropriate >> >> Thanks to Thomas W for pointing this out. >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> --- >> Changes in V3: New patch added to the series. No V1 & V2. >> >> drivers/platform/x86/think-lmi.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> index d89a1c9bdbf1..204f1060a533 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >> >> if (setting->possible_values) { >> /* Figure out what setting type is as BIOS does not return this */ >> - if (strchr(setting->possible_values, ',')) >> + if (strchr(setting->possible_values, ';')) >> return sysfs_emit(buf, "enumeration\n"); > > I think this patch should be earlier in the series. > So the other patches can work directly from the beginning. OK. I was avoiding refactoring everything - my git skills are not great. I'll look at doing that. > > Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets > backported. I wasn't go to label this for stable as it doesn't really have any real world impact that I know of. I figure the stable team have better things to do then backport minor stuff like this especially with it being in a series. If you feel strongly about it I can add it - though I'd rather only do it once the review is complete given the requests to split patches etc. This series has been way messier then I expected. For the Fixes tag - I don't have anything to reference with this apart from your email. What would I put in there? If you want to raise a bugzilla I'll happy reference that. Mark > >> } >> /* Anything else is going to be a string */ >> @@ -1471,6 +1471,17 @@ static int tlmi_analyze(void) >> } >> } >> } >> + /* >> + * firmware-attributes requires that possible_values are separated by ';' but >> + * Lenovo FW uses ','. Replace appropriately. >> + */ >> + if (setting->possible_values) { >> + char *tmp = setting->possible_values; >> + >> + while ((tmp = strchr(tmp, ',')) != NULL) >> + *tmp++ = ';'; >> + } >> + >> kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); >> tlmi_priv.setting[i] = setting; >> kfree(item); >> -- >> 2.39.2 >>
On Sat, Mar 18, 2023 at 02:06:28PM -0400, Mark Pearson wrote: > Thanks Thomas, > > On Sat, Mar 18, 2023, at 12:39 PM, Thomas Weißschuh wrote: > > On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote: > >> firmware-attributes class requires that possible values are delimited > >> using ';' but the Lenovo firmware uses ',' instead. > >> Parse string and replace where appropriate > >> > >> Thanks to Thomas W for pointing this out. This could also be a Reported-by: Thomas Weißschuh <linux@weissschuh.net> > >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > >> --- > >> Changes in V3: New patch added to the series. No V1 & V2. > >> > >> drivers/platform/x86/think-lmi.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > >> index d89a1c9bdbf1..204f1060a533 100644 > >> --- a/drivers/platform/x86/think-lmi.c > >> +++ b/drivers/platform/x86/think-lmi.c > >> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > >> > >> if (setting->possible_values) { > >> /* Figure out what setting type is as BIOS does not return this */ > >> - if (strchr(setting->possible_values, ',')) > >> + if (strchr(setting->possible_values, ';')) > >> return sysfs_emit(buf, "enumeration\n"); > > > > I think this patch should be earlier in the series. > > So the other patches can work directly from the beginning. > OK. I was avoiding refactoring everything - my git skills are not great. I'll look at doing that. I would do it like this with an interactive rebase: b # apply the generic parts of "platform/x86: think-lmi: use correct possible_values delimters" pick c2fbd30a7b15 platform/x86: think-lmi: add missing type attribute pick 644923d17048 platform/x86: think-lmi: Add possible_values for ThinkStation f a92fa3cda0d6 platform/x86: think-lmi: use correct possible_values delimters > > Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets > > backported. > I wasn't go to label this for stable as it doesn't really have any > real world impact that I know of. I figure the stable team have better > things to do then backport minor stuff like this especially with it > being in a series. If you feel strongly about it I can add it - though > I'd rather only do it once the review is complete given the requests > to split patches etc. This series has been way messier then I > expected. The -stable process should be automated with the proper stable Cc. Given that this technically breaks ABI it may better to keep it out of stable, though. FYI I looked at the only user of this ABI that I know, fwupd, and it should gracefully handle this change. It accepts both ',' and ';' as separator. > For the Fixes tag - I don't have anything to reference with this apart > from your email. What would I put in there? If you want to raise a > bugzilla I'll happy reference that. The Fixes tag refers to the original commit that introduced the fixed issue. In this case it would be the commit adding the think-lmi driver: Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms") Thomas
On Sat, Mar 18, 2023, at 8:11 PM, Thomas Weißschuh wrote: > On Sat, Mar 18, 2023 at 02:06:28PM -0400, Mark Pearson wrote: >> Thanks Thomas, >> >> On Sat, Mar 18, 2023, at 12:39 PM, Thomas Weißschuh wrote: >> > On Fri, Mar 17, 2023 at 11:46:35AM -0400, Mark Pearson wrote: >> >> firmware-attributes class requires that possible values are delimited >> >> using ';' but the Lenovo firmware uses ',' instead. >> >> Parse string and replace where appropriate >> >> >> >> Thanks to Thomas W for pointing this out. > > This could also be a > > Reported-by: Thomas Weißschuh <linux@weissschuh.net> Good point - will update > >> >> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> >> --- >> >> Changes in V3: New patch added to the series. No V1 & V2. >> >> >> >> drivers/platform/x86/think-lmi.c | 13 ++++++++++++- >> >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> >> index d89a1c9bdbf1..204f1060a533 100644 >> >> --- a/drivers/platform/x86/think-lmi.c >> >> +++ b/drivers/platform/x86/think-lmi.c >> >> @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >> >> >> >> if (setting->possible_values) { >> >> /* Figure out what setting type is as BIOS does not return this */ >> >> - if (strchr(setting->possible_values, ',')) >> >> + if (strchr(setting->possible_values, ';')) >> >> return sysfs_emit(buf, "enumeration\n"); >> > >> > I think this patch should be earlier in the series. >> > So the other patches can work directly from the beginning. >> OK. I was avoiding refactoring everything - my git skills are not great. I'll look at doing that. > > I would do it like this with an interactive rebase: > > b # apply the generic parts of "platform/x86: think-lmi: use correct > possible_values delimters" > pick c2fbd30a7b15 platform/x86: think-lmi: add missing type attribute > pick 644923d17048 platform/x86: think-lmi: Add possible_values for > ThinkStation > f a92fa3cda0d6 platform/x86: think-lmi: use correct possible_values > delimters Thank you. I have already done this and I dropped the two last ones and then just created them manually but with an extra commit thrown in. It worked out pretty well and let me clean up other pieces at the same time. Slowly learning...appreciate everyone's patience. Every time I think I have this figured a review process teaches me otherwise :) > >> > Also maybe this needs a Fixes: tag and a Cc: stable@ so it gets >> > backported. > >> I wasn't go to label this for stable as it doesn't really have any >> real world impact that I know of. I figure the stable team have better >> things to do then backport minor stuff like this especially with it >> being in a series. If you feel strongly about it I can add it - though >> I'd rather only do it once the review is complete given the requests >> to split patches etc. This series has been way messier then I >> expected. > > The -stable process should be automated with the proper stable Cc. > > Given that this technically breaks ABI it may better to keep it out of > stable, though. > > FYI I looked at the only user of this ABI that I know, fwupd, and it > should gracefully handle this change. > It accepts both ',' and ';' as separator. > >> For the Fixes tag - I don't have anything to reference with this apart >> from your email. What would I put in there? If you want to raise a >> bugzilla I'll happy reference that. > > The Fixes tag refers to the original commit that introduced the fixed > issue. > In this case it would be the commit adding the think-lmi driver: > > Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface > support on Lenovo platforms") > > Thomas Ah - got it. Will add. Thanks. Mark
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index d89a1c9bdbf1..204f1060a533 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -1040,7 +1040,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, if (setting->possible_values) { /* Figure out what setting type is as BIOS does not return this */ - if (strchr(setting->possible_values, ',')) + if (strchr(setting->possible_values, ';')) return sysfs_emit(buf, "enumeration\n"); } /* Anything else is going to be a string */ @@ -1471,6 +1471,17 @@ static int tlmi_analyze(void) } } } + /* + * firmware-attributes requires that possible_values are separated by ';' but + * Lenovo FW uses ','. Replace appropriately. + */ + if (setting->possible_values) { + char *tmp = setting->possible_values; + + while ((tmp = strchr(tmp, ',')) != NULL) + *tmp++ = ';'; + } + kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); tlmi_priv.setting[i] = setting; kfree(item);
firmware-attributes class requires that possible values are delimited using ';' but the Lenovo firmware uses ',' instead. Parse string and replace where appropriate Thanks to Thomas W for pointing this out. Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- Changes in V3: New patch added to the series. No V1 & V2. drivers/platform/x86/think-lmi.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)