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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3444110c-d6c0-16df-9b5d-12578ed442c5@collabora.com>
Date:   Tue, 24 Mar 2020 18:08:03 +0100
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, vbendeb@...omium.org,
        groeck@...omium.org, bleung@...omium.org, dtor@...omium.org,
        gwendal@...omium.org, andy@...radead.org,
        Collabora Kernel ML <kernel@...labora.com>,
        Ayman Bagabas <ayman.bagabas@...il.com>,
        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>,
        Yauhen Kharuzhy <jekhor@...il.com>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2] platform: x86: Add ACPI driver for ChromeOS

Hi Greg,

On 24/3/20 17:49, Greg Kroah-Hartman wrote:
> On Tue, Mar 24, 2020 at 05:31:10PM +0100, Enric Balletbo i Serra wrote:
>> 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@...labora.com>
>>>
>>> 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.
> 
> Are you sure they aren't already there under /sys/firmware/acpi/?  I
> thought all tables and methods were exported there with no need to do
> anything special.
> 

That's the first I did when I started to forward port this patch from chromeos
kernel to mainline.

On my system I get:

/sys/firmware/acpi/tables#
APIC  DSDT  FACP  FACS  HPET  MCFG  SSDT  data  dynamic

(data and dynamic are empty directories)

I quickly concluded (maybe wrong) that as there is no a MLST entry it was not
exported, but maybe one of those already contains the info? Or, should I expect
a MLST entry here?

> What makes these attributes "special" from any other ACPI method?
> 

I can't answer this question right now. I need to investigate more I guess ;-)

Thanks again for your answer,
Enric

>>>> +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?
> 
> How can any userspace be looking for anything that hasn't been submitted
> before?  That's nothing to worry about, we don't have to support things
> like that :)
> 
>> I'll investigate the different places in userspace where this is used and see
>> how difficult it is to do the changes.
> 
> Look at /sys/firmware/acpi/ first please.
> 
> thanks,
> 
> greg k-h
>