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;
MIME-Version: 1.0
References: <20230420165454.9517-1-jorge.lopez2@hp.com> <20230420165454.9517-13-jorge.lopez2@hp.com>
 <cf54c6f4-d177-4904-82ee-9d33566fb920@t-8ch.de>
In-Reply-To: <cf54c6f4-d177-4904-82ee-9d33566fb920@t-8ch.de>
From:   Jorge Lopez <jorgealtxwork@gmail.com>
Date:   Thu, 27 Apr 2023 17:17:57 -0500
Message-ID: <CAOOmCE8rpA=XvWBxcyRVu_gOHie3qN0E15Rs9bLfhb6tPZ7tyg@mail.gmail.com>
Subject: Re: [PATCH v11 12/14] HP BIOSCFG driver - surestart-attributes
To:     =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <thomas@t-8ch.de>
Cc:     hdegoede@redhat.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Apr 23, 2023 at 7:16=E2=80=AFAM Thomas Wei=C3=9Fschuh <thomas@t-8ch=
.de> wrote:
>
> On 2023-04-20 11:54:52-0500, Jorge Lopez wrote:
> >  .../x86/hp/hp-bioscfg/surestart-attributes.c  | 130 ++++++++++++++++++
> >  1 file changed, 130 insertions(+)
> >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/surestart-attrib=
utes.c
> >
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c =
b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > new file mode 100644
> > index 000000000000..72952758ffe3
> > --- /dev/null
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/surestart-attributes.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Functions corresponding to sure start object type attributes under
> > + * BIOS for use with hp-bioscfg driver
> > + *
> > + *  Copyright (c) 2022 HP Development Company, L.P.
> > + */
> > +
> > +#include "bioscfg.h"
> > +#include <asm-generic/posix_types.h>
>
> Is the asm include needed?
> If yes, why not use linux/types.h?
>

Will change in Version 12

> > +
> > +#define LOG_MAX_ENTRIES              254
>
> A comment on how this values came to be would be good.
>

Done!

> > +#define LOG_ENTRY_SIZE               16
> > +
> > +/*
> > + * audit_log_entry_count_show - Reports the number of
> > + *                           existing audit log entries available
> > + *                           to be read
> > + */
> > +static ssize_t audit_log_entry_count_show(struct kobject *kobj,
> > +                                       struct kobj_attribute *attr, ch=
ar *buf)
> > +{
> > +     int ret;
> > +     u32 count =3D 0;
> > +
> > +     ret =3D hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT,
> > +                                HPWMI_SURESTART,
> > +                                &count, 1, sizeof(count));
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return sysfs_emit(buf, "%d,%d,%d\n", count, LOG_ENTRY_SIZE,
> > +                       LOG_MAX_ENTRIES);
> > +}
> > +
> > +/*
> > + * audit_log_entries_show() - Return all entries found in log file
> > + */
> > +static ssize_t audit_log_entries_show(struct kobject *kobj,
> > +                                   struct kobj_attribute *attr, char *=
buf)
> > +{
> > +     int ret;
> > +     int i;
> > +     u32 count =3D 0;
> > +
> > +     // Get the number of event logs
> > +     ret =3D hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG_COUNT,
> > +                                HPWMI_SURESTART,
> > +                                &count, 1, sizeof(count));
> > +
> > +     /*
> > +      * The show() api will not work if the audit logs ever go
> > +      *  beyond 4KB
> > +      */
> > +     if (count * LOG_ENTRY_SIZE > PAGE_SIZE)
> > +             return -EFAULT;
>
> The error code seems not to match.
>

Changing error to -EINVAL

> Instead of not returning any data, why not show as many results as
> possible?
>

if count * LOG_ENTRY_SIZE > PAGE_SIZE then I prefer to return an error.
if the count is correct but a failure occurs while reading individual
audit logs then we will return a partial list of all audit logs
This changes will be included in Version 12

> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /*
> > +      * We are guaranteed the buffer is 4KB so today all the event
> > +      * logs will fit
> > +      */
> > +
> > +     for (i =3D 0; ((i < count) & (ret >=3D 0)); i++) {
>
> &&
>
> Better yet, pull the condition ret >=3D 0 into the body, as an else-branc=
h
> for the existing check.
>

Done!

> > +             *buf =3D (i + 1);
>
> Isn't this directly overwritten by the query below?

buf input value indicates the audit log to be read hence the reason
why it is overwritten.
This is an expected behavior.
>
> > +             ret =3D hp_wmi_perform_query(HPWMI_SURESTART_GET_LOG,
> > +                                        HPWMI_SURESTART,
> > +                                        buf, 1, 128);
> > +             if (ret >=3D 0)
> > +                     buf +=3D LOG_ENTRY_SIZE;
>
> So 128 bytes are read but only the first 16 bytes are preserved?
>
> The documentation says that each entry has 128 bytes in the file.
> And that they are separated by ";", which is not implemented.

The statement will be removed from documentation  (separated by ";")
audit log size is 16 bytes.
>
> Can the audit-log not contain all-zero bytes?
> If it does this would need to be a bin_attribute.

Bytes 16-127 are ignored and not used at this time.  If the audit log
changes, then the driver will need to change to accommodate the new
audit log size.
The audit log file cannot contain all zero bytes.
>
> > +     }
> > +
> > +     return (count * LOG_ENTRY_SIZE);
>
> No need for braces.

Done!
>
<snip>