Re: [PATCH 1/2] platform/x86: think-lmi: add missing type attribute

From: "Mark Pearson" <mpearson-lenovo@squebb.ca>
To: "Thomas Weißschuh" <thomas@t-8ch.de>
Cc: "Hans de Goede" <hdegoede@redhat.com>,
	"markgross@kernel.org" <markgross@kernel.org>,
	"Mark Pearson" <markpearson@lenovo.com>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>
Subject: Re: [PATCH 1/2] platform/x86: think-lmi: add missing type attribute
Date: Sun, 12 Mar 2023 09:19:29 -0400	[thread overview]
Message-ID: <1815e7b3-8199-46e9-aa3d-1d31bd14001f@app.fastmail.com> (raw)
In-Reply-To: <87957353-0778-46ca-9906-411022b55ded@app.fastmail.com>

Apologies for the duplication, I forgot to set email format as text so it got bounced by the mailing list. Resending.
Mark

On Sat, Mar 11, 2023, at 10:44 PM, Mark Pearson wrote:
> Thanks Thomas
> 
> On Sat, Mar 11, 2023, at 10:33 PM, Thomas Weißschuh wrote:
>> On Sat, Mar 11, 2023 at 09:46:34PM -0500, Mark Pearson wrote:
>> > This driver was missing the mandatory type attribute...oops.
>> > 
>> > Add it in along with logic to determine whether the attribute is an
>> > enumeration type or a string by parsing the possible_values attribute.
>> > 
>> > Some platforms (and some attributes) don't return possible_values so to
>> > prevent trying to scan NULL strings mark these as "N/A".
>> > 
>> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460
>> 
>> Afaik Fixes: is only for references to commits.
>> Instead a Reported-by/Link would be better.
> Ah - thanks. My bad.
> 
>> 
>> > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> > ---
>> >  drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++---
>> >  drivers/platform/x86/think-lmi.h |  6 ++++++
>> >  2 files changed, 29 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> > index 86b33b74519b..495a5e045069 100644
>> > --- a/drivers/platform/x86/think-lmi.c
>> > +++ b/drivers/platform/x86/think-lmi.c
>> > @@ -941,12 +941,18 @@ 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);
>> >  }
>> >  
>> > +static ssize_t type_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->type == TLMI_ENUMERATION ? "enumeration" : "string");
>> > +}
>> > +
>> >  static ssize_t current_value_store(struct kobject *kobj,
>> >  struct kobj_attribute *attr,
>> >  const char *buf, size_t count)
>> > @@ -1036,10 +1042,13 @@ 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 struct kobj_attribute attr_type = __ATTR_RO(type);
>> > +
>> >  static struct attribute *tlmi_attrs[] = {
>> >  &attr_displ_name.attr,
>> >  &attr_current_val.attr,
>> >  &attr_possible_values.attr,
>> > + &attr_type.attr,
>> >  NULL
>> >  };
>> >  
>> > @@ -1424,6 +1433,17 @@ static int tlmi_analyze(void)
>> >  pr_info("Error retrieving possible values for %d : %s\n",
>> >  i, setting->display_name);
>> >  }
>> > + /* If we don't have a possible value mark as N/A */
>> > + if (!setting->possible_values) {
>> > + setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL);
>> 
>> kmalloc() can fail.
>> 
>> > + sprintf(setting->possible_values, "N/A");
>> 
>> This writes the '\0' out of bounds?
>> 
>> kmalloc() and sprintf() could be replaced with kstrdup().
>> 
>> Instead of having to do allocations, check for failure, worry about how
>> sysfs_emit() will handle the NULL it would be easier to just check of
>> NULL inside possible_values_show() and fall back to N/A there.
> Good point - that would be better. I'll update.
> 
>> 
>> > + }
>> > + /* Figure out what setting type is as BIOS does not return this */
>> > + if (strchr(setting->possible_values, ','))
>> 
>> possible_values could be NULL if the sprintf would not have dereferenced
>> it before.
> Agreed. This was part of the reason I'd put in the N/A to cover for that case (so it should never be NULL). But I'll revisit this.
> 
>> 
>> > + setting->type = TLMI_ENUMERATION;
>> > + else
>> > + setting->type = TLMI_STRING;
>> > +
>> 
>> Is it worth introducing a new enum and field in struct
>> tlmi_attr_setting?
>> The check could also be done directly in type_show().
>> (with a NULL-check).
> Ack, this makes sense. I'll look at doing that.
> 
> Many thanks for the review
> Mark