RE: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs

From: "Zheng, Lv" <lv.zheng@intel.com>
To: "Purdila, Octavian" <octavian.purdila@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>,
	Matt Fleming <matt@codeblueprint.co.uk>,
	Mark Brown <broonie@kernel.org>, Wolfram Sang <wsa@the-dreams.de>,
	Joel Becker <jlbec@evilplan.org>, Christoph Hellwig <hch@lst.de>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Tirdea, Irina" <irina.tirdea@intel.com>
Subject: RE: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
Date: Tue, 5 Apr 2016 03:11:21 +0000	[thread overview]
Message-ID: <1AE640813FDE7649BE1B193DEA596E883BB6677B@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <CAE1zotLgjfKvFr0W9CjFbbkxqM6erLGwv-aHzTt=qwdCPzGOhw@mail.gmail.com>

Hi,

> From: Octavian Purdila [mailto:octavian.purdila@intel.com]
> Subject: Re: [RFC PATCH 10/10] acpi: add support for loading SSDTs via configfs
> 
> On Fri, Apr 1, 2016 at 7:55 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi,
> 
> Hi Lv,
> 
> >> Add support for acpi_user_table configfs items that allows the user to
> >> load new tables. The data attributes contains the table data and once it
> >> is filled from userspace the table is loaded and ACPI devices are
> >> enumerated.
> > [Lv Zheng]
> > We've been considering to implement this facility before.
> > The 2 alternative solutions are:
> > 1. implement LOAD/UNLOAD ioctl for /sys/kernel/debug/acpi/acpidbg - this
> will be useful for extracting AML byte stream from kernel to be used by a
> userspace disassembler.
> 
> AFAIK adding new ioctls is discouraged.
[Lv Zheng] 
Tools/power/acpi/tools/acpidbg is a file descriptor based utility.
And it needs a method to obtain an AML byte stream from kernel.
Using ioctl is a best fit design for acpidbg so that it needn't to access any other files.

> 
> > 2. transition /sys/firmware/acpi/tables/xxx into directory and implement
> /sys/firmware/acpi/tables/load, /sys/firmware/acpi/tables/unload - this should
> be able to meet your requirement.
> 
> We can't do that as it would break the ABI.
[Lv Zheng] 
The only user of this directory hierarchy is acpidump.
And the user of this tool are all developers/reporters on the kernel bugzilla.
We've been asking the Bugzilla users to use the up-to-date acpidump instead of the distribution vendor provided one for so many years.
So IMO, this is not a serious problem you should consider.
You only need to think about an acceptable way for the distribution vendors to synchronize the kernel change and the acpidump change.

IMO:
You may expose a version file from /sys/firmware/acpi.
acpidump can be changed accordingly by referencing the version file.
And old directory hierarchy support could be kept in acpidump.

Note that acpidump is also a part of the kernel, so your change could be consistent.
For example,
If you changed acpidump prior than making the kernel change, the distribution vendors might have already released the new acpidump for all old kernels before you transitioned the directory hierarchy.

> 
> > So my first question is:
> > Why do you use configfs rather than the existing mechanisms?
> 
> sysfs is not a good choice for dealing with objects created from
> userspace, configfs was created to address this specific need. Since
> we want to be able to create and load new tables from userspace this
> use-case fits very well with configfs.
[Lv Zheng] 
Was the table binary stream still maintained by the userspace?
If not, I couldn't see the difference/advantages from using /sys/firmware/acpi/tables to using configfs.

Thanks and best regards
-Lv