Hi Greg,
Many thanks for your quick answer, some comments below.
On 22/3/20 12:10, Greg Kroah-Hartman wrote:
> On Sun, Mar 22, 2020 at 10:43:34AM +0100, Enric Balletbo i Serra wrote:
>> This driver attaches to the ChromeOS ACPI device and then exports the values
>> reported by the ACPI in a sysfs directory. The ACPI values are presented in
>> the string form (numbers as decimal values) or binary blobs, and can be
>> accessed as the contents of the appropriate read only files in the sysfs
>> directory tree originating in /sys/devices/platform/chromeos_acpi.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>
> What is wrong with the "default" ACPI sysfs access? Why do you need a
> special driver just for this specific ACPI firmware?
>
Please correct me if I am wrong, as I'm not an ACPI expert and I probably have
some ACPI leaks and misunderstandings.
What is exporting this driver is the attributes for the non-default Chromebook
specific MLST ACPI method. Hence, I assumed we needed a special driver to expose
these values that can't be done using "default" ACPI sysfs. Note that these
attributes are dynamically created and are different between Chromebooks so need
some parsing.
I didn't find a "standard" way to expose these attributes to userspace, so,
please kindly point me to one if there is one.
> Also, you forgot to add Documentation/ABI/ entries for your new files :(
>
Right, my bad. Not all Chromebooks have the same values. I can document the ones
that are created on the devices I have but I'll probably miss some of them. I'll
do some firmware research regarding this.
>> +config ACPI_CHROMEOS
>> + tristate "ChromeOS specific ACPI extensions"
>> + depends on ACPI
>> + depends on CHROME_PLATFORMS
>
> No BUILD_TEST?
>
Will add in the next version.
>
>> +static void
>> +chromeos_acpi_remove_attribs(struct chromeos_acpi_attribute_group *aag)
>> +{
>> + struct chromeos_acpi_attribute *attr, *tmp_attr;
>> +
>> + list_for_each_entry_safe(attr, tmp_attr, &aag->attribs, list) {
>> + sysfs_remove_bin_file(aag->kobj, &attr->bin_attr);
>
> Attribute groups are your friend, do not do this "by hand".
>
I thought that the code is more readable doing it attribute by attribute, and
the reason is that apart from remove the bin file I should also free the name,
the data and the specific struct to store the attribute itself as all are
dynamically allocated.
Using attribute groups I should do two steps:
sysfs_remove_group()
list_for_each_entry_safe(attr, tmp_attr, &aag->attribs, list) {
kfree(attr->name);
kfree(attr->data);
kfree(attr);
}
Ok, will do that in next version.
>> + kfree(attr->name);
>> + kfree(attr->data);
>> + kfree(attr);
>> + }
>> +}
>> +
>> +/**
>> + * chromeos_acpi_add_group() - Create a sysfs group including attributes
>> + * representing a nested ACPI package.
>> + *
>> + * @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.
>> + */
>> +static int chromeos_acpi_add_group(union acpi_object *obj, char *name,
>> + int num_attrs, int index)
>> +{
>> + struct device *dev = &chromeos_acpi.pdev->dev;
>> + struct chromeos_acpi_attribute_group *aag;
>> + union acpi_object *element;
>> + int i, count, ret;
>> +
>> + aag = kzalloc(sizeof(*aag), GFP_KERNEL);
>> + if (!aag)
>> + return -ENOMEM;
>> +
>> + aag->name = chromeos_acpi_alloc_name(name, num_attrs, index);
>> + if (!aag->name) {
>> + ret = -ENOMEM;
>> + goto free_group;
>> + }
>> +
>> + aag->kobj = kobject_create_and_add(aag->name, &dev->kobj);
>
> By using "raw" kobjects, you just now prevented any userspace tool from
> seeing these attributes (like udev). Not nice :(
>
> Why, if you really really have to do this, are you not just using
> "normal" struct device attributes instead?
>
Ok.
>> +static int __init chromeos_acpi_init(void)
>> +{
>> + int ret;
>> +
>> + chromeos_acpi.pdev = platform_device_register_simple("chromeos_acpi",
>> + PLATFORM_DEVID_NONE, NULL, 0);
>> + if (IS_ERR(chromeos_acpi.pdev)) {
>> + pr_err("unable to register chromeos_acpi platform device\n");
>> + return PTR_ERR(chromeos_acpi.pdev);
>> + }
>
> Only use platform devices and drivers for things that are actually
> platform devices and drivers. That's not what this is, it is an ACPI
> device and driver. Don't abuse the platform interface please.
>
Ok. The purpose was to not break ChromeOS userspace since is looking for the
attributes inside /sys/devices/platform/chromeos_acpi. Not a good reason, I
know, and I assume we will need to change userspace instead, and convert this to
a ACPI device and driver only, right?
I'll investigate the different places in userspace where this is used and see
how difficult it is to do the changes.
Thanks,
~Enric
> thanks,
>
> greg k-h
>
