On Monday, April 13, 2020 4:12:59 PM CEST Greg Kroah-Hartman wrote:
> Meta-comment to the ACPI developers, shouldn't all of this happen
> "automatically" with the existing ACPI entries in sysfs?
I'm not quite sure what you mean by "all of this" here.
Can you please explain?
> If not, is this driver the proper way to do this?
>
> Minor review comments below:
>
>
> On Mon, Apr 13, 2020 at 03:46:11PM +0200, Enric Balletbo i Serra wrote:
> > +What: /sys/bus/acpi/devices/GGL0001:00/BINF.{0,1,4}
> > +Date: April 2020
> > +KernelVersion: 5.8
> > +Description:
> > + This file is reserved and doesn't shows useful information
> > + for now.
>
> Then do not even have it present. sysfs should never export files that
> nothing can be done with them, userspace "knows" that if a file is not
> present, it can not use it. Bring it back when it is useful.
>
> > +What: /sys/bus/acpi/devices/GGL0001:00/MECK
> > +Date: April 2020
> > +KernelVersion: 5.8
> > +Description:
> > + This binary file returns the SHA-1 or SHA-256 hash that is
> > + read out of the Management Engine extend registers during
> > + boot. The hash is exported vi ACPI so the OS can verify that
> > + the Management Engine firmware has not changed. If Management
> > + Engine is not present, or if the firmware was unable to read the
> > + extend registers, this buffer can be zero.
>
> The size is zero, or the contents are 0?
>
> > +static char *chromeos_acpi_alloc_name(char *name, int count, int index)
> > +{
> > + char *str;
> > +
> > + if (count == 1)
> > + str = kstrdup(name, GFP_KERNEL);
> > + else
> > + str = kasprintf(GFP_KERNEL, "%s.%d", name, index);
>
> That's crazy, make this more obvious that "count" affects the name so
> much. As it is, no one would know this unless they read the function
> code, and not just the name.
>
>
> > +/**
> > + * chromeos_acpi_add_group() - Create a sysfs group including attributes
> > + * representing a nested ACPI package.
> > + *
> > + * @adev: ACPI device.
> > + * @obj: Package contents as returned by ACPI.
> > + * @name: Name of the group.
> > + * @num_attrs: Number of attributes of this package.
> > + * @index: Index number of this particular group.
> > + *
> > + * The created group is called @name in case there is a single instance, or
> > + * @name.@index otherwise.
> > + *
> > + * All group and attribute storage allocations are included in the lists
> > for
> > + * tracking of allocated memory.
> > + *
> > + * Return: 0 on success, negative errno on failure.
> > + */
>
> Meta-comment, no need for kerneldoc on static functions. It's nice to
> see, but nothing is going to notice them...
>
> > +static int chromeos_acpi_add_method(struct acpi_device *adev, char *name)
> > +{
> > + struct device *dev = &adev->dev;
> > + struct acpi_buffer output;
> > + union acpi_object *obj;
> > + acpi_status status;
> > + int ret = 0;
> > +
> > + output.length = ACPI_ALLOCATE_BUFFER;
> > +
> > + status = acpi_evaluate_object(adev->handle, name, NULL, &output);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(dev, "failed to retrieve %s (%d)\n", name, status);
> > + return status;
> > + }
> > +
> > + obj = output.pointer;
> > + if (obj->type == ACPI_TYPE_PACKAGE)
> > + ret = chromeos_acpi_handle_package(adev, obj, name);
> > +
> > + kfree(output.pointer);
>
> Why the need for 'obj' at all in this function? Minor nit.
>
> > +
> > + return ret;
> > +}
> > +
> > +static int chromeos_acpi_device_add(struct acpi_device *adev)
> > +{
> > + struct chromeos_acpi_attribute_group *aag = chromeos_acpi.root;
> > + struct device *dev = &adev->dev;
> > + int i, ret;
> > +
> > + aag = kzalloc(sizeof(*aag), GFP_KERNEL);
> > + if (!aag)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(&aag->attribs);
> > + INIT_LIST_HEAD(&aag->list);
> > + INIT_LIST_HEAD(&chromeos_acpi.groups);
> > +
> > + chromeos_acpi.root = aag;
> > +
> > + /*
> > + * Attempt to add methods by querying the device's MLST method
> > + * for the list of methods.
> > + */
> > + if (!chromeos_acpi_process_mlst(adev))
> > + return 0;
> > +
> > + dev_info(dev, "falling back to default list of methods\n");
>
> Is this debugging code left over? If not, make it an error, and what
> would a user be able to do with it?
Or use dev_dbg() to print it, possibly?
Cheers!
