Re: [PATCH v2] platform: x86: Add ACPI driver for ChromeOS

[Enric Balletbo i Serra]

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
>