Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 28 Apr 2023 17:36:17 +0200
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <thomas@t-8ch.de>
To:     Jorge Lopez <jorgealtxwork@gmail.com>
Cc:     hdegoede@redhat.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v11 08/14] HP BIOSCFG driver - bioscfg-h
Message-ID: <462b5d12-0430-4fbe-8c26-7b6126556ec8@t-8ch.de>
References: <20230420165454.9517-1-jorge.lopez2@hp.com>
 <20230420165454.9517-9-jorge.lopez2@hp.com>
 <ca74121b-bb78-4093-8625-13359c324c28@t-8ch.de>
 <CAOOmCE_MpCBFOHd6QtzD5ufcwEz_FhJvqevj68pVeY_JS+V=Rg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAOOmCE_MpCBFOHd6QtzD5ufcwEz_FhJvqevj68pVeY_JS+V=Rg@mail.gmail.com>
Precedence: bulk

On 2023-04-28 10:24:40-0500, Jorge Lopez wrote:
> On Sun, Apr 23, 2023 at 7:01 AM Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >
> > On 2023-04-20 11:54:48-0500, Jorge Lopez wrote:
> > > ---
> > >  drivers/platform/x86/hp/hp-bioscfg/bioscfg.h | 613 +++++++++++++++++++
> > >  1 file changed, 613 insertions(+)
> > >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/bioscfg.h

<snip>

> > > +/* global structure used by multiple WMI interfaces */
> > > +extern struct bioscfg_priv bioscfg_drv;
> > > +
> > > +enum hp_wmi_data_type {
> > > +     HPWMI_STRING_TYPE               = 0x00,
> > > +     HPWMI_INTEGER_TYPE              = 0x01,
> > > +     HPWMI_ENUMERATION_TYPE          = 0x02,
> > > +     HPWMI_ORDERED_LIST_TYPE         = 0x03,
> > > +     HPWMI_PASSWORD_TYPE             = 0x04,
> > > +     HPWMI_SECURE_PLATFORM_TYPE      = 0x05,
> > > +     HPWMI_SURE_START_TYPE           = 0x06
> > > +};
> >
> > Unused.
> 
> Both hp_wmi_data_type and hp_wmi_data_elements are used
> for instance  HP_WMI_STRING_TYPE
> 
> bioscfg.c:338: case HPWMI_STRING_TYPE:
> bioscfg.c:626: case HPWMI_STRING_TYPE:
> bioscfg.c:722: case HPWMI_STRING_TYPE:
> bioscfg.c:798: case HPWMI_STRING_TYPE:
> bioscfg.c:906: ret = hp_init_bios_attributes(HPWMI_STRING_TYPE,
> HP_WMI_BIOS_STRING_GUID);
> bioscfg.h:247: HPWMI_STRING_TYPE

Indeed. I think I just searched for "hp_wmi_data_type".

The proper enum hp_wmi_data_type type should be used instead of
"int attr_type".

<snip>

> > > +
> > > +enum hp_wmi_elements_count {
> > > +     STRING_ELEM_CNT         = 12,
> > > +     INTEGER_ELEM_CNT        = 13,
> > > +     ENUM_ELEM_CNT           = 13,
> > > +     ORDERED_ELEM_CNT        = 12,
> > > +     PASSWORD_ELEM_CNT       = 15
> > > +};
> >
> > To make it clearer where these values come from you could put them into
> > the enum hp_wmi_data_elements.
> >
> > ...
> >         ORD_LIST_ELEMENTS = 11,
> >         ORD_LIST_ELEM_CNT = 12,
> > ...
> 
> Done!  changes provided across all files affected.
> 
> >
> > But replacing the loop logic would remove the need for these enums
> > completely.
> >
> 
> _CNT values are necessary when elements are read from a buffer (
> populate_string_elements_from_buffer).
> _CNT values are not needed when elements are read from a package
> (populate_string_package_data)

Hm, I don't see why populate_string_elements_from_buffer() would need
the _CNT define.

(In another review mail I wrote down how I would expect it to look
without the loop)

<snip>

> > > +
> > > +#define ATTRIBUTE_PROPERTY_STORE(curr_val, type)                     \
> > > +     static ssize_t curr_val##_store(struct kobject *kobj,           \
> > > +                                     struct kobj_attribute *attr,    \
> > > +                                     const char *buf, size_t count)  \
> > > +     {                                                               \
> > > +             char *p = NULL;                                         \
> > > +             char *attr_value = NULL;                                \
> > > +             int i;                                                  \
> > > +             int ret = -EIO;                                         \
> > > +                                                                     \
> > > +             attr_value = kstrdup(buf, GFP_KERNEL);                  \
> > > +             if (!attr_value)                                        \
> > > +                     return -ENOMEM;                                 \
> > > +                                                                     \
> > > +             p = memchr(attr_value, '\n', count);                    \
> > > +             if (p != NULL)                                          \
> > > +                     *p = '\0';                                      \
> >
> > This can also truncate the string if there is data after the newline.
> 
> This is a expected behavior as described by Hans in a later email

I'm fine with stripping a trailing newline.

But this truncates the string at the first newline.

"foo\nbar" -> "foo"
"\nfoo" -> ""

<snip>