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

[Enric Balletbo i Serra]

Hi Srinivas,

On 25/3/20 22:41, Srinivas Pandruvada wrote:
> Hi Enric,
> 
> On Wed, 2020-03-25 at 21:34 +0100, Enric Balletbo i Serra wrote:
>> Hi Srinivas,
>>
>> On 24/3/20 18:20, Srinivas Pandruvada wrote:
>>> On Tue, 2020-03-24 at 18:08 +0100, Enric Balletbo i Serra wrote:
>>>> 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.
>>>>>>
>>> [...]
>>>
>>>>> 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?
>>>>
>>> If the data you are reading doesn't depend on any runtime variable
>>> in
>>> ACPI tables then you can read from firmware tables as is.
>>>
>>> You can download acpica tools and run your method on acpi dump
>>> using
>>> acpiexec tool. Once you can take dump, you can run on any Linux
>>> system.
>>>
>>> If you can get what you need from running on the dump, then you can
>>> do
>>> by directly reading from /sys/firmware/acpi/tables/ from user space
>>> without kernel change. Sometimes it is enough as lots of config
>>> data
>>> tend to be static.
>>>
>>
>> As I said I'm not an ACPI expert, so thanks in advance for your help.
>>
>> I am trying to look if I can get from userspace the value of the HWID
>> entry
>> exported from the driver.
>>
>> $ cat /sys/devices/platform/chromeos_acpi/HWID
>> SAMUS E25-G7R-W35
>>
>> Using acpiexec I get the element list of the MLST method, but I don't
>> know how
>> to get the HWID value.
>>
>> - evaluate crhw.mlst
>> Evaluating \CRHW.MLST
>> Evaluation of \CRHW.MLST returned object 0x55f17a7aed60, external
>> buffer length 158
>>   [Package] Contains 10 Elements:
>>     [String] Length 04 = "CHSW"
>>     [String] Length 04 = "FWID"
>>     [String] Length 04 = "HWID"
>>     [String] Length 04 = "FRID"
>>     [String] Length 04 = "BINF"
>>     [String] Length 04 = "GPIO"
>>     [String] Length 04 = "VBNV"
>>     [String] Length 04 = "VDAT"
>>     [String] Length 04 = "FMAP"
>>     [String] Length 04 = "MECK"
>>
>> Any clue?
>>
> So I guess your  mlst method gives list of methods you can call.
> So here your can directly evaluate
> 
> - evaluate crhw.HWID
> 

Right, I tried, but that doesn't gives the result I want, for example:

Evaluating \CRHW.HWID
0x1 Outstanding allocations after evaluation of \CRHW.HWID
Evaluation of \CRHW.HWID returned object 0x55b9e373fd60, external buffer length 38
  [Package] Contains 1 Elements:
    [String] Length 00 = ""


I found that the VBTx are addresses setup in the following file.

src/vendorcode/google/chromeos/acpi/gnvs.asl:

VBT0,   32,     // 0x000 - Boot Reason
VBT1,   32,     // 0x004 - Active Main Firmware
VBT2,   32,     // 0x008 - Active EC Firmware
VBT3,   16,     // 0x00c - CHSW
VBT4, 2048,     // 0x00e - HWID
VBT5,  512,     // 0x10e - FWID
VBT6,  512,     // 0x14e - FRID
VBT7,   32,     // 0x18e - active main firmware type
VBT8,   32,     // 0x192 - Recovery Reason
VBT9,   32,     // 0x196 - FMAP base address
CHVD, 24576,    // 0x19a - VDAT space filled by verified boot
VBTA,   32,     // 0xd9a - pointer to smbios FWID
MEHH,  256,     // 0xd9e - Management Engine Hash
RMOB,   32,     // 0xdbe - RAM oops base address
RMOL,   32,     // 0xdc2 - RAM oops length
ROVP,   32,     // 0xdc6 - pointer to RO_VPD
ROVL,   32,     // 0xdca - size of RO_VPD
RWVP,   32,     // 0xdce - pointer to RW_VPD
RWVL,   32,     // 0xdd2 - size of RW_VPD
                // 0xdd6

Can I assume that the info I want is only accessible in runtime and is not
exported via the static tables?

Thanks,
 Enric


> 
> Thanks,
> Srinivas
> 
> 
> 
>> Thanks in advance,
>> Enric
>>
>>
>>> Thanks,
>>> Srinivas
>>>
>>>
>>>
>>>
>>>
>>>
>>>>> 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
>>>>>
>