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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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.

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