Re: [PATCH v3 3/3] platform/x86: think-lmi: use correct possible_values delimters

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

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