Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 28 Apr 2022 14:40:08 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Collabora Kernel ML <kernel@collabora.com>,
        groeck@chromium.org, bleung@chromium.org, dtor@chromium.org,
        gwendal@chromium.org, vbendeb@chromium.org, andy@infradead.org,
        Ayman Bagabas <ayman.bagabas@gmail.com>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        =?utf-8?B?Qmxhxb4=?= Hrastnik <blaz@mxxn.io>,
        Darren Hart <dvhart@infradead.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Jeremy Soller <jeremy@system76.com>,
        Mattias Jacobsson <2pi@mok.nu>,
        Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
        Rajat Jain <rajatja@google.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-acpi@vger.kernel.org,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Enric Balletbo i Serra <eballetbo@gmail.com>
Subject: Re: [PATCH v8] platform: x86: Add ChromeOS ACPI device driver
Message-ID: <YmqLKBVPMl4AFzCE@kroah.com>
References: <Ylmmf03fewXEjRr0@debian-BULLSEYE-live-builder-AMD64>
 <YmUNnq5tH8mykNOF@kroah.com>
 <c195573c-e7c7-a2dd-7f29-c6d4625fefdb@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c195573c-e7c7-a2dd-7f29-c6d4625fefdb@collabora.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 05:24:04PM +0500, Muhammad Usama Anjum wrote:
> 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?

Long old post that describes the issue in detail is here:
	http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

> > 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.

Great.

> 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.

What is the max number of groups you can ever have?  10?  100?  1000?
Pick a high number, define them all (macros make this easy), and then
only enable the ones that you need at runtime.

> 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);

Look at the attribute pointer.  That's all you care about.  Compare it
to a real pointer and away you go!

> 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.

This whole thing is a new abi :)

> 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?

Again, you are racing with userspace and loosing.  If you do not mind
userspace not noticing the attributes, fine, leave it as-is, but odds
are you don't want that.

thanks,

greg k-h