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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YmqLYyUY3sutmMTI@kroah.com>
Date:   Thu, 28 Apr 2022 14:41:07 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Collabora Kernel ML <kernel@...labora.com>,
        groeck@...omium.org, bleung@...omium.org, dtor@...omium.org,
        gwendal@...omium.org, vbendeb@...omium.org, andy@...radead.org,
        Ayman Bagabas <ayman.bagabas@...il.com>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Blaž Hrastnik <blaz@...n.io>,
        Darren Hart <dvhart@...radead.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Jeremy Soller <jeremy@...tem76.com>,
        Mattias Jacobsson <2pi@....nu>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Rajat Jain <rajatja@...gle.com>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Enric Balletbo i Serra <eballetbo@...il.com>
Subject: Re: [PATCH v8] platform: x86: Add ChromeOS ACPI device driver

On Thu, Apr 28, 2022 at 02:40:08PM +0200, Greg Kroah-Hartman wrote:
> 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!

Also remember, each group has a is_visible function, so you know what
group this is implicitly.

thanks,

greg k-h