Re: [PATCH v11 03/14] HP BIOSCFG driver - bioscfg

From: Jorge Lopez
Date: Tue May 02 2023 - 17:37:18 EST

On Tue, May 2, 2023 at 4:14 PM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> On 2023-05-02 14:52:14-0500, Jorge Lopez wrote:
> > On Sat, Apr 22, 2023 at 5:16 PM <thomas@xxxxxxxx> wrote:
> > >
> > > On 2023-04-20 11:54:43-0500, Jorge Lopez wrote:
> > > > ---
> > > > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 961 +++++++++++++++++++
> > > > 1 file changed, 961 insertions(+)
> > > > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > > >
> > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > > > new file mode 100644
> > > > index 000000000000..4b0d4f56e65f
> > > > --- /dev/null
> > > > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
>
> <snip>
>
> > > > + int retval = 0;
> > > > + u8 *attr_name;
> > >
> > > const char *
> >
> > Cannot define attr_name as 'const char *'. attr_name value is set
> > within the function
>
> Then use "char *". This is pointing to a NULL-terminated string,
> not a number or binary data.
>
> This is also the type used by the functions this value is passed to.

Done!

>
> <snip>
>
> > > > + retval = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
> > > > + NULL, "%s", str_value);
> > > > +
> > > > + if (retval) {
> > > > + kobject_put(attr_name_kobj);
> > >
> > > The kobj was not created, why does it need the kobj_put() ?
> > As indicated by kobject_init_and_add ...
> >
> > * This function combines the call to kobject_init() and kobject_add().
> > *
> > * If this function returns an error, kobject_put() must be called to
> > * properly clean up the memory associated with the object. This is the
> > * same type of error handling after a call to kobject_add() and kobject
> > * lifetime rules are the same here.
>
> I stand corrected, thanks!
>
> > >
> > > > + goto err_attr_init;
> > > > + }
> > > > +
> > > > + /* enumerate all of these attributes */
> > > > + switch (attr_type) {
> > > > + case HPWMI_STRING_TYPE:
> > > > + retval = populate_string_package_data(elements,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_INTEGER_TYPE:
> > > > + retval = populate_integer_package_data(elements,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_ENUMERATION_TYPE:
> > > > + retval = populate_enumeration_package_data(elements,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_ORDERED_LIST_TYPE:
> > > > + retval = populate_ordered_list_package_data(elements,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_PASSWORD_TYPE:
> > > > + retval = populate_password_package_data(elements,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + default:
> > > > + break;
> > >
> > > This default does nothing.
> > >
> > > > + }
> > > > +
> > > > + kfree(str_value);
> > >
> > > Why is str_value only freed down here? It has not been used for half a
> > > screen of code.
> >
> > Added early in the development process and failed to clean up here.
> > >
> > > > + }
> > >
> > > else
> > >
> > > > +
> > > > + if (obj->type == ACPI_TYPE_BUFFER) {
> > > > +
> > > > + buffer_size = obj->buffer.length;
> > > > + buffer_ptr = obj->buffer.pointer;
> > > > +
> > > > + retval = get_string_from_buffer(&buffer_ptr, &buffer_size, str, MAX_BUFF);
> > > > + if (retval < 0)
> > > > + goto err_attr_init;
> > > > +
> > > > + if (attr_type == HPWMI_PASSWORD_TYPE || attr_type == HPWMI_SECURE_PLATFORM_TYPE)
> > > > + tmp_set = bioscfg_drv.authentication_dir_kset;
> > > > + else
> > > > + tmp_set = bioscfg_drv.main_dir_kset;
> > >
> > > There is a bunch of common logic duplicated in both the buffer and
> > > package branches.
> >
> > Older BIOS reports the ACPI data as objects of type ACPI_TYPE_PACKAGE
> > and the associated data is reported as elements.
> > Newer BIOS reports the ACPI data as objects of type ACPI_TYPE_BUFFER.
> > (actypes.h union acpi_object)
> > The code follows a common logic although the data is acquired
> > differently according to the ACPI object type
> > >
> > > > +
> > > > + if (kset_find_obj(tmp_set, str)) {
> > > > + pr_warn("Duplicate attribute name found - %s\n", str);
> > >
> > > Also mention that it is being ignored.
> > >
> > > > + goto nextobj;
> > > > + }
> > > > +
> > > > + /* build attribute */
> > > > + attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
> > > > + if (!attr_name_kobj) {
> > > > + retval = -ENOMEM;
> > > > + goto err_attr_init;
> > > > + }
> > > > +
> > > > + attr_name_kobj->kset = tmp_set;
> > > > +
> > > > + temp_str = str;
> > > > + if (attr_type == HPWMI_SECURE_PLATFORM_TYPE)
> > > > + temp_str = "SPM";
> > > > +
> > > > + retval = kobject_init_and_add(attr_name_kobj,
> > > > + &attr_name_ktype, NULL, "%s",
> > > > + temp_str);
> > > > + if (retval) {
> > > > + kobject_put(attr_name_kobj);
> > > > + goto err_attr_init;
> > > > + }
> > > > +
> > > > + /* enumerate all of these attributes */
> > > > + switch (attr_type) {
> > > > + case HPWMI_STRING_TYPE:
> > > > + retval = populate_string_buffer_data(buffer_ptr,
> > > > + &buffer_size,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_INTEGER_TYPE:
> > > > + retval = populate_integer_buffer_data(buffer_ptr,
> > > > + &buffer_size,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_ENUMERATION_TYPE:
> > > > + retval = populate_enumeration_buffer_data(buffer_ptr,
> > > > + &buffer_size,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_ORDERED_LIST_TYPE:
> > > > + retval = populate_ordered_list_buffer_data(buffer_ptr,
> > > > + &buffer_size,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + case HPWMI_PASSWORD_TYPE:
> > > > + retval = populate_password_buffer_data(buffer_ptr,
> > > > + &buffer_size,
> > > > + instance_id,
> > > > + attr_name_kobj);
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > + }
> > >
> > > What if it's neither a package nor a buffer?
> >
> > we return an error if it is neither a package nor a buffer.
> >
> > if (obj->type != ACPI_TYPE_PACKAGE && obj->type != ACPI_TYPE_BUFFER) {
> > pr_err("Error: Expected ACPI-package or buffer type, got:
> > %d\n", obj->type);
> > retval = -EIO;
> > goto err_attr_init;
> > }
>
> Indeed, thanks.
>
> This could be pulled together into:
>
> if (package)
> ...
> else if (buffer)
> ...
> else
> report error

Done!

> Note:
>
> The "Error: " prefix is unnecessary. It's already logged as pr_err().
>
Good catch. Thank you. Done!
> <snip>