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@xxxxxxxxxxxxxx>
> >> Signed-off-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> >> ---
> >> 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
