Re: [PATCH v8] platform: x86: Add ChromeOS ACPI device driver

[Muhammad Usama Anjum]

On 4/24/22 1:43 PM, Greg Kroah-Hartman wrote:
> On Fri, Apr 15, 2022 at 10:08:15PM +0500, Muhammad Usama Anjum wrote:
>> +	i = 0;
>> +	list_for_each_entry(aag, &chromeos_acpi.groups, list) {
>> +		chromeos_acpi.dev_groups[i] = &aag->group;
>> +		i++;
>> +	}
>> +
>> +	ret = sysfs_create_groups(&dev->kobj, chromeos_acpi.dev_groups);
> 
> You have raced with userspace and lost here :(
> 
Sorry, What does it mean exactly?

> Use the default groups pointer in the platform driver for this, and use
> the is_visible() callback to know to show, or not show, the attribute
> instead of building up dynamic lists of attributes at runtime.  That
> will save you lots of crazy logic and housekeeping _AND_ userspace tools
> will work properly as well.
> 

Driver has the 2 kinds of attributes:

A) Attributes which are always there. For example, CHSW and HWIDs etc.
They can be easily shows via dev_groups pointer in platform driver.

B) Attribute groups which vary between 0 to N. N is platform dependent
and can be determined at runtime. For example, GPIO attribute group
which have 4 sub attributes in it:

Group GPIO.0 --> attributes GPIO.0, GPIO.1, GPIO.2 and GPIO.3
Group GPIO.1 --> attributes GPIO.0, GPIO.1, GPIO.2 and GPIO.3
...
Group GPIO.N --> attributes GPIO.0, GPIO.1, GPIO.2 and GPIO.3

My Chromebook has 2 GPIO attribute groups while I've found logs of a
Chromebook which has 7 GPIO groups.

Why these groups cannot be defined at compile time (Shortcomings):

1) We don't know the total GPIO groups.
Possible solution: Determine GPIO groups' number at run time and define
attributes at run time.

2) We cannot determine from attribute name that this group will be
visible or not as is_visible doesn't provide information about its group
name.
umode_t (*is_visible)(struct kobject *, struct attribute *, int);

3) In attribute.show functions, we only know about the attribute's name
and not the group's name. We cannot evaluate and show the attribute.
ssize_t (*show)(struct device *dev, struct device_attribute *attr, char
*buf);
Possible solution for 2) and 3):
Embed the group name into attribute name like:
attributes GPIO.0_GPIO.0, GPIO.0_GPIO.1, GPIO.0_GPIO.2 and GPIO.0_GPIO.3
attributes GPIO.1_GPIO.0, GPIO.1_GPIO.1, GPIO.2_GPIO.2 and GPIO.3_GPIO.3
But this is completely new ABI which we don't desire.

After looking at dependence on runtime values, can we keep the existing
version of the driver instead of trying to workout some other hybrid
solution?

> thanks,
> 
> greg k-h

-- 
Muhammad Usama Anjum