[PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Jan Kiszka]

Enhance acpi_load_table to also return the table index. Use that index
to unload the table again when the corresponding directory in configfs
gets removed. This allows to change SSDTs without rebooting the system.
It also allows to destroy devices again that a dynamically loaded SSDT
created.

This is widely similar to the DT overlay behavior.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Can someone explain to me why an unloaded table still sticks around in
sysfs and why we cannot release its ID and rather have to use a new one
when loading a modified version?

 drivers/acpi/acpi_configfs.c      | 12 +++++++++++-
 drivers/acpi/acpica/dbfileio.c    |  2 +-
 drivers/acpi/acpica/tbxfload.c    | 38 +++++++++++++++++++++++++++++++++++---
 drivers/firmware/efi/efi.c        |  2 +-
 drivers/pci/hotplug/sgi_hotplug.c |  2 +-
 include/acpi/acpixf.h             |  6 +++++-
 6 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/acpi_configfs.c b/drivers/acpi/acpi_configfs.c
index 146a77fb762d..dac8dbf16cc0 100644
--- a/drivers/acpi/acpi_configfs.c
+++ b/drivers/acpi/acpi_configfs.c
@@ -20,6 +20,7 @@ static struct config_group *acpi_table_group;
 struct acpi_table {
 	struct config_item cfg;
 	struct acpi_table_header *header;
+	u32 index;
 };
 
 static ssize_t acpi_table_aml_write(struct config_item *cfg,
@@ -52,7 +53,7 @@ static ssize_t acpi_table_aml_write(struct config_item *cfg,
 	if (!table->header)
 		return -ENOMEM;
 
-	ret = acpi_load_table(table->header);
+	ret = acpi_load_table(table->header, &table->index);
 	if (ret) {
 		kfree(table->header);
 		table->header = NULL;
@@ -215,8 +216,17 @@ static struct config_item *acpi_table_make_item(struct config_group *group,
 	return &table->cfg;
 }
 
+static void acpi_table_drop_item(struct config_group *group,
+				 struct config_item *cfg)
+{
+	struct acpi_table *table = container_of(cfg, struct acpi_table, cfg);
+
+	acpi_unload_table(table->index);
+}
+
 struct configfs_group_operations acpi_table_group_ops = {
 	.make_item = acpi_table_make_item,
+	.drop_item = acpi_table_drop_item,
 };
 
 static struct config_item_type acpi_tables_type = {
diff --git a/drivers/acpi/acpica/dbfileio.c b/drivers/acpi/acpica/dbfileio.c
index 4d81ea291d93..4486ec8b995b 100644
--- a/drivers/acpi/acpica/dbfileio.c
+++ b/drivers/acpi/acpica/dbfileio.c
@@ -129,7 +129,7 @@ acpi_status acpi_db_load_tables(struct acpi_new_table_desc *list_head)
 	while (table_list_head) {
 		table = table_list_head->table;
 
-		status = acpi_load_table(table);
+		status = acpi_load_table(table, NULL);
 		if (ACPI_FAILURE(status)) {
 			if (status == AE_ALREADY_EXISTS) {
 				acpi_os_printf
diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
index b71ce3b817ea..44e719303b58 100644
--- a/drivers/acpi/acpica/tbxfload.c
+++ b/drivers/acpi/acpica/tbxfload.c
@@ -304,6 +304,8 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_install_table)
  *
  * PARAMETERS:  table               - Pointer to a buffer containing the ACPI
  *                                    table to be loaded.
+ *              table_index         - Pointer to a variable receiving the table
+ *                                    index, or NULL.
  *
  * RETURN:      Status
  *
@@ -314,10 +316,10 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_install_table)
  *              to ensure that the table is not deleted or unmapped.
  *
  ******************************************************************************/
-acpi_status acpi_load_table(struct acpi_table_header *table)
+acpi_status acpi_load_table(struct acpi_table_header *table, u32 *table_index)
 {
+	u32 table_index_dummy;
 	acpi_status status;
-	u32 table_index;
 
 	ACPI_FUNCTION_TRACE(acpi_load_table);
 
@@ -327,12 +329,15 @@ acpi_status acpi_load_table(struct acpi_table_header *table)
 		return_ACPI_STATUS(AE_BAD_PARAMETER);
 	}
 
+	if (!table_index)
+		table_index = &table_index_dummy;
+
 	/* Install the table and load it into the namespace */
 
 	ACPI_INFO(("Host-directed Dynamic ACPI Table Load:"));
 	status = acpi_tb_install_and_load_table(ACPI_PTR_TO_PHYSADDR(table),
 						ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL,
-						FALSE, &table_index);
+						FALSE, table_index);
 	return_ACPI_STATUS(status);
 }
 
@@ -340,6 +345,33 @@ ACPI_EXPORT_SYMBOL(acpi_load_table)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_unload_table
+ *
+ * PARAMETERS:  table_index         - Index of the table to be unloaded.
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Unloads the table and deletes all namespace objects associated
+ *              with that table. Unloading of the DSDT is not allowed.
+ *              Note: Mainly intended to support hotplug removal of SSDTs.
+ *
+ ******************************************************************************/
+acpi_status acpi_unload_table(u32 table_index)
+{
+	ACPI_FUNCTION_TRACE(acpi_unload_table);
+
+	if (!table_index) {
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+	}
+
+	ACPI_INFO(("Host-directed Dynamic ACPI Table Unload:"));
+	return acpi_tb_unload_table(table_index);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_unload_table)
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_unload_parent_table
  *
  * PARAMETERS:  object              - Handle to any namespace object owned by
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index b372aad3b449..8681c5536bfc 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -274,7 +274,7 @@ static __init int efivar_ssdt_load(void)
 			goto free_data;
 		}
 
-		ret = acpi_load_table(data);
+		ret = acpi_load_table(data, NULL);
 		if (ret) {
 			pr_err("failed to load table: %d\n", ret);
 			goto free_data;
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index 339bce0403dd..57d627d699b4 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -355,7 +355,7 @@ static int enable_slot(struct hotplug_slot *bss_hotplug_slot)
 	if (SN_ACPI_BASE_SUPPORT() && ssdt) {
 		acpi_status ret;
 
-		ret = acpi_load_table((struct acpi_table_header *)ssdt);
+		ret = acpi_load_table((struct acpi_table_header *)ssdt, NULL);
 		if (ACPI_FAILURE(ret)) {
 			printk(KERN_ERR "%s: acpi_load_table failed (0x%x)\n",
 			       __func__, ret);
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 15c86ce4df53..8d36a9a72532 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -487,7 +487,11 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION
 					       u8 physical))
 
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
-			    acpi_load_table(struct acpi_table_header *table))
+			    acpi_load_table(struct acpi_table_header *table,
+					    u32 *table_index))
+
+ACPI_EXTERNAL_RETURN_STATUS(acpi_status
+			    acpi_unload_table(u32 table_index))
 
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 			    acpi_unload_parent_table(acpi_handle object))

Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Mika Westerberg]

On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
> Enhance acpi_load_table to also return the table index. Use that index
> to unload the table again when the corresponding directory in configfs
> gets removed. This allows to change SSDTs without rebooting the system.
> It also allows to destroy devices again that a dynamically loaded SSDT
> created.
> 
> This is widely similar to the DT overlay behavior.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Can someone explain to me why an unloaded table still sticks around in
> sysfs and why we cannot release its ID and rather have to use a new one
> when loading a modified version?

IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
can correct me if I got it wrong.

>  drivers/acpi/acpi_configfs.c      | 12 +++++++++++-
>  drivers/acpi/acpica/dbfileio.c    |  2 +-
>  drivers/acpi/acpica/tbxfload.c    | 38 +++++++++++++++++++++++++++++++++++---
>  drivers/firmware/efi/efi.c        |  2 +-
>  drivers/pci/hotplug/sgi_hotplug.c |  2 +-
>  include/acpi/acpixf.h             |  6 +++++-
>  6 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_configfs.c b/drivers/acpi/acpi_configfs.c
> index 146a77fb762d..dac8dbf16cc0 100644
> --- a/drivers/acpi/acpi_configfs.c
> +++ b/drivers/acpi/acpi_configfs.c
> @@ -20,6 +20,7 @@ static struct config_group *acpi_table_group;
>  struct acpi_table {
>  	struct config_item cfg;
>  	struct acpi_table_header *header;
> +	u32 index;
>  };
>  
>  static ssize_t acpi_table_aml_write(struct config_item *cfg,
> @@ -52,7 +53,7 @@ static ssize_t acpi_table_aml_write(struct config_item *cfg,
>  	if (!table->header)
>  		return -ENOMEM;
>  
> -	ret = acpi_load_table(table->header);
> +	ret = acpi_load_table(table->header, &table->index);
>  	if (ret) {
>  		kfree(table->header);
>  		table->header = NULL;
> @@ -215,8 +216,17 @@ static struct config_item *acpi_table_make_item(struct config_group *group,
>  	return &table->cfg;
>  }
>  
> +static void acpi_table_drop_item(struct config_group *group,
> +				 struct config_item *cfg)
> +{
> +	struct acpi_table *table = container_of(cfg, struct acpi_table, cfg);
> +
> +	acpi_unload_table(table->index);
> +}
> +
>  struct configfs_group_operations acpi_table_group_ops = {
>  	.make_item = acpi_table_make_item,
> +	.drop_item = acpi_table_drop_item,
>  };
>  
>  static struct config_item_type acpi_tables_type = {
> diff --git a/drivers/acpi/acpica/dbfileio.c b/drivers/acpi/acpica/dbfileio.c
> index 4d81ea291d93..4486ec8b995b 100644
> --- a/drivers/acpi/acpica/dbfileio.c
> +++ b/drivers/acpi/acpica/dbfileio.c
> @@ -129,7 +129,7 @@ acpi_status acpi_db_load_tables(struct acpi_new_table_desc *list_head)
>  	while (table_list_head) {
>  		table = table_list_head->table;
>  
> -		status = acpi_load_table(table);
> +		status = acpi_load_table(table, NULL);
>  		if (ACPI_FAILURE(status)) {
>  			if (status == AE_ALREADY_EXISTS) {
>  				acpi_os_printf
> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
> index b71ce3b817ea..44e719303b58 100644
> --- a/drivers/acpi/acpica/tbxfload.c
> +++ b/drivers/acpi/acpica/tbxfload.c
> @@ -304,6 +304,8 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_install_table)
>   *
>   * PARAMETERS:  table               - Pointer to a buffer containing the ACPI
>   *                                    table to be loaded.
> + *              table_index         - Pointer to a variable receiving the table
> + *                                    index, or NULL.
>   *
>   * RETURN:      Status
>   *
> @@ -314,10 +316,10 @@ ACPI_EXPORT_SYMBOL_INIT(acpi_install_table)
>   *              to ensure that the table is not deleted or unmapped.
>   *
>   ******************************************************************************/
> -acpi_status acpi_load_table(struct acpi_table_header *table)
> +acpi_status acpi_load_table(struct acpi_table_header *table, u32 *table_index)
>  {
> +	u32 table_index_dummy;
>  	acpi_status status;
> -	u32 table_index;
>  
>  	ACPI_FUNCTION_TRACE(acpi_load_table);
>  
> @@ -327,12 +329,15 @@ acpi_status acpi_load_table(struct acpi_table_header *table)
>  		return_ACPI_STATUS(AE_BAD_PARAMETER);
>  	}
>  
> +	if (!table_index)
> +		table_index = &table_index_dummy;
> +
>  	/* Install the table and load it into the namespace */
>  
>  	ACPI_INFO(("Host-directed Dynamic ACPI Table Load:"));
>  	status = acpi_tb_install_and_load_table(ACPI_PTR_TO_PHYSADDR(table),
>  						ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL,
> -						FALSE, &table_index);
> +						FALSE, table_index);
>  	return_ACPI_STATUS(status);
>  }
>  
> @@ -340,6 +345,33 @@ ACPI_EXPORT_SYMBOL(acpi_load_table)
>  
>  /*******************************************************************************
>   *
> + * FUNCTION:    acpi_unload_table
> + *
> + * PARAMETERS:  table_index         - Index of the table to be unloaded.
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Unloads the table and deletes all namespace objects associated
> + *              with that table. Unloading of the DSDT is not allowed.
> + *              Note: Mainly intended to support hotplug removal of SSDTs.
> + *
> + ******************************************************************************/
> +acpi_status acpi_unload_table(u32 table_index)
> +{
> +	ACPI_FUNCTION_TRACE(acpi_unload_table);
> +
> +	if (!table_index) {
> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +	}
> +
> +	ACPI_INFO(("Host-directed Dynamic ACPI Table Unload:"));
> +	return acpi_tb_unload_table(table_index);
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_unload_table)
> +
> +/*******************************************************************************
> + *
>   * FUNCTION:    acpi_unload_parent_table
>   *
>   * PARAMETERS:  object              - Handle to any namespace object owned by
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index b372aad3b449..8681c5536bfc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -274,7 +274,7 @@ static __init int efivar_ssdt_load(void)
>  			goto free_data;
>  		}
>  
> -		ret = acpi_load_table(data);
> +		ret = acpi_load_table(data, NULL);
>  		if (ret) {
>  			pr_err("failed to load table: %d\n", ret);
>  			goto free_data;
> diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
> index 339bce0403dd..57d627d699b4 100644
> --- a/drivers/pci/hotplug/sgi_hotplug.c
> +++ b/drivers/pci/hotplug/sgi_hotplug.c
> @@ -355,7 +355,7 @@ static int enable_slot(struct hotplug_slot *bss_hotplug_slot)
>  	if (SN_ACPI_BASE_SUPPORT() && ssdt) {
>  		acpi_status ret;
>  
> -		ret = acpi_load_table((struct acpi_table_header *)ssdt);
> +		ret = acpi_load_table((struct acpi_table_header *)ssdt, NULL);
>  		if (ACPI_FAILURE(ret)) {
>  			printk(KERN_ERR "%s: acpi_load_table failed (0x%x)\n",
>  			       __func__, ret);
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 15c86ce4df53..8d36a9a72532 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -487,7 +487,11 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_INIT_FUNCTION
>  					       u8 physical))
>  
>  ACPI_EXTERNAL_RETURN_STATUS(acpi_status
> -			    acpi_load_table(struct acpi_table_header *table))
> +			    acpi_load_table(struct acpi_table_header *table,
> +					    u32 *table_index))
> +
> +ACPI_EXTERNAL_RETURN_STATUS(acpi_status
> +			    acpi_unload_table(u32 table_index))
>  
>  ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>  			    acpi_unload_parent_table(acpi_handle object))
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Jan Kiszka]

On 2017-05-29 14:47, Mika Westerberg wrote:
> On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
>> Enhance acpi_load_table to also return the table index. Use that index
>> to unload the table again when the corresponding directory in configfs
>> gets removed. This allows to change SSDTs without rebooting the system.
>> It also allows to destroy devices again that a dynamically loaded SSDT
>> created.
>>
>> This is widely similar to the DT overlay behavior.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Can someone explain to me why an unloaded table still sticks around in
>> sysfs and why we cannot release its ID and rather have to use a new one
>> when loading a modified version?
> 
> IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
> can correct me if I got it wrong.

OK... Is that standard-driven or just a limitation of this implementation?

Is there an upper limit of tables? I'm thinking of lengthy development
sessions that play with tables, loading and unloading modified versions.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

RE: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Moore, Robert]

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
> Sent: Monday, May 29, 2017 5:53 AM
> To: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> Zheng, Lv <lv.zheng@intel.com>; linux-acpi@vger.kernel.org; Linux Kernel
> Mailing List <linux-kernel@vger.kernel.org>; devel@acpica.org; Moore,
> Robert <robert.moore@intel.com>
> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> removal
> 
> On 2017-05-29 14:47, Mika Westerberg wrote:
> > On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
> >> Enhance acpi_load_table to also return the table index. Use that
> >> index to unload the table again when the corresponding directory in
> >> configfs gets removed. This allows to change SSDTs without rebooting
> the system.
> >> It also allows to destroy devices again that a dynamically loaded
> >> SSDT created.
> >>
> >> This is widely similar to the DT overlay behavior.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>
> >> Can someone explain to me why an unloaded table still sticks around
> >> in sysfs and why we cannot release its ID and rather have to use a
> >> new one when loading a modified version?
> >
> > IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
> > can correct me if I got it wrong.
> 


[Moore, Robert] 

I'm not entirely sure what the table manager code looks like at this time, but ACPICA does in fact support table unloading.

It is a rather dangerous thing to do, however -- unless you are careful about it. Basically, all handles that reference the table to be unloaded will go bad.

> OK... Is that standard-driven or just a limitation of this
> implementation?
> 
> Is there an upper limit of tables? I'm thinking of lengthy development
> sessions that play with tables, loading and unloading modified versions.
> 
 
[Moore, Robert] 

I think that the maximum number of loaded ACPI tables is 255 at any given time. However, things are cleaned up after an unload such that repeated load/unload cycles should not consume resources.


> Jan
> 
> --
> Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence
> Center Embedded Linux

Re: [Devel] [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]



> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka(a)siemens.com]
> Sent: Monday, May 29, 2017 5:53 AM
> To: Mika Westerberg <mika.westerberg(a)linux.intel.com>
> Cc: Rafael J. Wysocki <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>;
> Zheng, Lv <lv.zheng(a)intel.com>; linux-acpi(a)vger.kernel.org; Linux Kernel
> Mailing List <linux-kernel(a)vger.kernel.org>; devel(a)acpica.org; Moore,
> Robert <robert.moore(a)intel.com>
> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> removal
> 
> On 2017-05-29 14:47, Mika Westerberg wrote:
> > On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
> >> Enhance acpi_load_table to also return the table index. Use that
> >> index to unload the table again when the corresponding directory in
> >> configfs gets removed. This allows to change SSDTs without rebooting
> the system.
> >> It also allows to destroy devices again that a dynamically loaded
> >> SSDT created.
> >>
> >> This is widely similar to the DT overlay behavior.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com>
> >> ---
> >>
> >> Can someone explain to me why an unloaded table still sticks around
> >> in sysfs and why we cannot release its ID and rather have to use a
> >> new one when loading a modified version?
> >
> > IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
> > can correct me if I got it wrong.
> 


[Moore, Robert] 

I'm not entirely sure what the table manager code looks like at this time, but ACPICA does in fact support table unloading.

It is a rather dangerous thing to do, however -- unless you are careful about it. Basically, all handles that reference the table to be unloaded will go bad.

> OK... Is that standard-driven or just a limitation of this
> implementation?
> 
> Is there an upper limit of tables? I'm thinking of lengthy development
> sessions that play with tables, loading and unloading modified versions.
> 
 
[Moore, Robert] 

I think that the maximum number of loaded ACPI tables is 255 at any given time. However, things are cleaned up after an unload such that repeated load/unload cycles should not consume resources.


> Jan
> 
> --
> Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence
> Center Embedded Linux

Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Rafael J. Wysocki]

On Tue, May 30, 2017 at 11:16 PM, Moore, Robert <robert.moore@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>> Sent: Monday, May 29, 2017 5:53 AM
>> To: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>> Zheng, Lv <lv.zheng@intel.com>; linux-acpi@vger.kernel.org; Linux Kernel
>> Mailing List <linux-kernel@vger.kernel.org>; devel@acpica.org; Moore,
>> Robert <robert.moore@intel.com>
>> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
>> removal
>>
>> On 2017-05-29 14:47, Mika Westerberg wrote:
>> > On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
>> >> Enhance acpi_load_table to also return the table index. Use that
>> >> index to unload the table again when the corresponding directory in
>> >> configfs gets removed. This allows to change SSDTs without rebooting
>> the system.
>> >> It also allows to destroy devices again that a dynamically loaded
>> >> SSDT created.
>> >>
>> >> This is widely similar to the DT overlay behavior.
>> >>
>> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> >> ---
>> >>
>> >> Can someone explain to me why an unloaded table still sticks around
>> >> in sysfs and why we cannot release its ID and rather have to use a
>> >> new one when loading a modified version?
>> >
>> > IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
>> > can correct me if I got it wrong.
>>
>
>
> [Moore, Robert]
>
> I'm not entirely sure what the table manager code looks like at this time, but ACPICA does in fact support table unloading.
>
> It is a rather dangerous thing to do, however -- unless you are careful about it. Basically, all handles that reference the table to be unloaded will go bad.

Right.

Linux should handle that in theory, but the code in there is mostly
very lightly tested AFAICS.

>> OK... Is that standard-driven or just a limitation of this
>> implementation?
>>
>> Is there an upper limit of tables? I'm thinking of lengthy development
>> sessions that play with tables, loading and unloading modified versions.
>>
>
> [Moore, Robert]
>
> I think that the maximum number of loaded ACPI tables is 255 at any given time. However, things are cleaned up after an unload such that repeated load/unload cycles should not consume resources.

I'm not sure if this is going to work seamlessly right away, but it
certainly can be made work.

That said, the change as proposed would be an API modification forcing
all of the OSes using ACPICA to change (or to carry out-of-the-tree
patches), so not nice.

What about adding a separate version of acpi_load_table() returning an
index (or an error on failures) instead of the status and leaving the
existing acpi_load_table() as is?

Thanks,
Rafael

RE: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Moore, Robert]

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Tuesday, May 30, 2017 2:42 PM
> To: Moore, Robert <robert.moore@intel.com>; Jan Kiszka
> <jan.kiszka@siemens.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>; Rafael J. Wysocki
> <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>; Zheng, Lv
> <lv.zheng@intel.com>; linux-acpi@vger.kernel.org; Linux Kernel Mailing
> List <linux-kernel@vger.kernel.org>; devel@acpica.org
> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> removal
> 
> On Tue, May 30, 2017 at 11:16 PM, Moore, Robert <robert.moore@intel.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
> >> Sent: Monday, May 29, 2017 5:53 AM
> >> To: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Len Brown
> >> <lenb@kernel.org>; Zheng, Lv <lv.zheng@intel.com>;
> >> linux-acpi@vger.kernel.org; Linux Kernel Mailing List
> >> <linux-kernel@vger.kernel.org>; devel@acpica.org; Moore, Robert
> >> <robert.moore@intel.com>
> >> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> >> removal
> >>
> >> On 2017-05-29 14:47, Mika Westerberg wrote:
> >> > On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
> >> >> Enhance acpi_load_table to also return the table index. Use that
> >> >> index to unload the table again when the corresponding directory
> >> >> in configfs gets removed. This allows to change SSDTs without
> >> >> rebooting
> >> the system.
> >> >> It also allows to destroy devices again that a dynamically loaded
> >> >> SSDT created.
> >> >>
> >> >> This is widely similar to the DT overlay behavior.
> >> >>
> >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> >> ---
> >> >>
> >> >> Can someone explain to me why an unloaded table still sticks
> >> >> around in sysfs and why we cannot release its ID and rather have
> >> >> to use a new one when loading a modified version?
> >> >
> >> > IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob
> >> > (CC'd) can correct me if I got it wrong.
> >>
> >
> >
> > [Moore, Robert]
> >
> > I'm not entirely sure what the table manager code looks like at this
> time, but ACPICA does in fact support table unloading.
> >
> > It is a rather dangerous thing to do, however -- unless you are
> careful about it. Basically, all handles that reference the table to be
> unloaded will go bad.
> 
> Right.
> 
> Linux should handle that in theory, but the code in there is mostly very
> lightly tested AFAICS.
> 
[Moore, Robert] 

The load/unload functionality works with the current table interfaces. For example, the AML debugger supports both Load and Unload commands:


  Load <Input Filename>           Load ACPI table from a file
  Unload <Namepath>               Unload an ACPI table via namespace object


- load ssdt.aml
Input file ssdt.aml, Length 0x3A (58) bytes
ACPI: Host-directed Dynamic ACPI Table Load:
ACPI: SSDT 0x00000000004A1B18 00003A (v02 Intel  _SSDT_01 00000001 INTL 20170303)
ACPI Exec: Table Event INSTALL, [SSDT] 004A1B18
ACPI Exec: Table Event LOAD, [SSDT] 004A1B18


- unload _T32
ACPI Exec: Table Event UNLOAD, [SSDT] 004A1B18
Parent of [_T32] (004A1028) unloaded and uninstalled



> >> OK... Is that standard-driven or just a limitation of this
> >> implementation?
> >>
> >> Is there an upper limit of tables? I'm thinking of lengthy
> >> development sessions that play with tables, loading and unloading
> modified versions.
> >>
> >
> > [Moore, Robert]
> >
> > I think that the maximum number of loaded ACPI tables is 255 at any
> given time. However, things are cleaned up after an unload such that
> repeated load/unload cycles should not consume resources.
> 
> I'm not sure if this is going to work seamlessly right away, but it
> certainly can be made work.
> 
> That said, the change as proposed would be an API modification forcing
> all of the OSes using ACPICA to change (or to carry out-of-the-tree
> patches), so not nice.
> 
> What about adding a separate version of acpi_load_table() returning an
> index (or an error on failures) instead of the status and leaving the
> existing acpi_load_table() as is?
> 
> Thanks,
> Rafael

Re: [Devel] [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 4452 bytes --]



> -----Original Message-----
> From: rjwysocki(a)gmail.com [mailto:rjwysocki(a)gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Tuesday, May 30, 2017 2:42 PM
> To: Moore, Robert <robert.moore(a)intel.com>; Jan Kiszka
> <jan.kiszka(a)siemens.com>
> Cc: Mika Westerberg <mika.westerberg(a)linux.intel.com>; Rafael J. Wysocki
> <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; Zheng, Lv
> <lv.zheng(a)intel.com>; linux-acpi(a)vger.kernel.org; Linux Kernel Mailing
> List <linux-kernel(a)vger.kernel.org>; devel(a)acpica.org
> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> removal
> 
> On Tue, May 30, 2017 at 11:16 PM, Moore, Robert <robert.moore(a)intel.com>
> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jan Kiszka [mailto:jan.kiszka(a)siemens.com]
> >> Sent: Monday, May 29, 2017 5:53 AM
> >> To: Mika Westerberg <mika.westerberg(a)linux.intel.com>
> >> Cc: Rafael J. Wysocki <rjw(a)rjwysocki.net>; Len Brown
> >> <lenb(a)kernel.org>; Zheng, Lv <lv.zheng(a)intel.com>;
> >> linux-acpi(a)vger.kernel.org; Linux Kernel Mailing List
> >> <linux-kernel(a)vger.kernel.org>; devel(a)acpica.org; Moore, Robert
> >> <robert.moore(a)intel.com>
> >> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> >> removal
> >>
> >> On 2017-05-29 14:47, Mika Westerberg wrote:
> >> > On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
> >> >> Enhance acpi_load_table to also return the table index. Use that
> >> >> index to unload the table again when the corresponding directory
> >> >> in configfs gets removed. This allows to change SSDTs without
> >> >> rebooting
> >> the system.
> >> >> It also allows to destroy devices again that a dynamically loaded
> >> >> SSDT created.
> >> >>
> >> >> This is widely similar to the DT overlay behavior.
> >> >>
> >> >> Signed-off-by: Jan Kiszka <jan.kiszka(a)siemens.com>
> >> >> ---
> >> >>
> >> >> Can someone explain to me why an unloaded table still sticks
> >> >> around in sysfs and why we cannot release its ID and rather have
> >> >> to use a new one when loading a modified version?
> >> >
> >> > IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob
> >> > (CC'd) can correct me if I got it wrong.
> >>
> >
> >
> > [Moore, Robert]
> >
> > I'm not entirely sure what the table manager code looks like at this
> time, but ACPICA does in fact support table unloading.
> >
> > It is a rather dangerous thing to do, however -- unless you are
> careful about it. Basically, all handles that reference the table to be
> unloaded will go bad.
> 
> Right.
> 
> Linux should handle that in theory, but the code in there is mostly very
> lightly tested AFAICS.
> 
[Moore, Robert] 

The load/unload functionality works with the current table interfaces. For example, the AML debugger supports both Load and Unload commands:


  Load <Input Filename>           Load ACPI table from a file
  Unload <Namepath>               Unload an ACPI table via namespace object


- load ssdt.aml
Input file ssdt.aml, Length 0x3A (58) bytes
ACPI: Host-directed Dynamic ACPI Table Load:
ACPI: SSDT 0x00000000004A1B18 00003A (v02 Intel  _SSDT_01 00000001 INTL 20170303)
ACPI Exec: Table Event INSTALL, [SSDT] 004A1B18
ACPI Exec: Table Event LOAD, [SSDT] 004A1B18


- unload _T32
ACPI Exec: Table Event UNLOAD, [SSDT] 004A1B18
Parent of [_T32] (004A1028) unloaded and uninstalled



> >> OK... Is that standard-driven or just a limitation of this
> >> implementation?
> >>
> >> Is there an upper limit of tables? I'm thinking of lengthy
> >> development sessions that play with tables, loading and unloading
> modified versions.
> >>
> >
> > [Moore, Robert]
> >
> > I think that the maximum number of loaded ACPI tables is 255 at any
> given time. However, things are cleaned up after an unload such that
> repeated load/unload cycles should not consume resources.
> 
> I'm not sure if this is going to work seamlessly right away, but it
> certainly can be made work.
> 
> That said, the change as proposed would be an API modification forcing
> all of the OSes using ACPICA to change (or to carry out-of-the-tree
> patches), so not nice.
> 
> What about adding a separate version of acpi_load_table() returning an
> index (or an error on failures) instead of the status and leaving the
> existing acpi_load_table() as is?
> 
> Thanks,
> Rafael

Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Jan Kiszka]

On 2017-05-30 23:41, Rafael J. Wysocki wrote:
> On Tue, May 30, 2017 at 11:16 PM, Moore, Robert <robert.moore@intel.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>>> Sent: Monday, May 29, 2017 5:53 AM
>>> To: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
>>> Zheng, Lv <lv.zheng@intel.com>; linux-acpi@vger.kernel.org; Linux Kernel
>>> Mailing List <linux-kernel@vger.kernel.org>; devel@acpica.org; Moore,
>>> Robert <robert.moore@intel.com>
>>> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
>>> removal
>>>
>>> On 2017-05-29 14:47, Mika Westerberg wrote:
>>>> On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
>>>>> Enhance acpi_load_table to also return the table index. Use that
>>>>> index to unload the table again when the corresponding directory in
>>>>> configfs gets removed. This allows to change SSDTs without rebooting
>>> the system.
>>>>> It also allows to destroy devices again that a dynamically loaded
>>>>> SSDT created.
>>>>>
>>>>> This is widely similar to the DT overlay behavior.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> Can someone explain to me why an unloaded table still sticks around
>>>>> in sysfs and why we cannot release its ID and rather have to use a
>>>>> new one when loading a modified version?
>>>>
>>>> IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
>>>> can correct me if I got it wrong.
>>>
>>
>>
>> [Moore, Robert]
>>
>> I'm not entirely sure what the table manager code looks like at this time, but ACPICA does in fact support table unloading.
>>
>> It is a rather dangerous thing to do, however -- unless you are careful about it. Basically, all handles that reference the table to be unloaded will go bad.
> 
> Right.
> 
> Linux should handle that in theory, but the code in there is mostly
> very lightly tested AFAICS.
> 
>>> OK... Is that standard-driven or just a limitation of this
>>> implementation?
>>>
>>> Is there an upper limit of tables? I'm thinking of lengthy development
>>> sessions that play with tables, loading and unloading modified versions.
>>>
>>
>> [Moore, Robert]
>>
>> I think that the maximum number of loaded ACPI tables is 255 at any given time. However, things are cleaned up after an unload such that repeated load/unload cycles should not consume resources.
> 
> I'm not sure if this is going to work seamlessly right away, but it
> certainly can be made work.
> 
> That said, the change as proposed would be an API modification forcing
> all of the OSes using ACPICA to change (or to carry out-of-the-tree
> patches), so not nice.
> 
> What about adding a separate version of acpi_load_table() returning an
> index (or an error on failures) instead of the status and leaving the
> existing acpi_load_table() as is?

Sure. Any reference/preference in naming and locating that version?
Should I leave acpica/ untouched at best? acpi_table_load/unload look
simple enough to carry the logic in acpi_configfs directly.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry removal

[Rafael J. Wysocki]

On Wednesday, May 31, 2017 06:39:05 PM Jan Kiszka wrote:
> On 2017-05-30 23:41, Rafael J. Wysocki wrote:
> > On Tue, May 30, 2017 at 11:16 PM, Moore, Robert <robert.moore@intel.com> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
> >>> Sent: Monday, May 29, 2017 5:53 AM
> >>> To: Mika Westerberg <mika.westerberg@linux.intel.com>
> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; Len Brown <lenb@kernel.org>;
> >>> Zheng, Lv <lv.zheng@intel.com>; linux-acpi@vger.kernel.org; Linux Kernel
> >>> Mailing List <linux-kernel@vger.kernel.org>; devel@acpica.org; Moore,
> >>> Robert <robert.moore@intel.com>
> >>> Subject: Re: [PATCH] acpi: configfs: Unload SSDT on configfs entry
> >>> removal
> >>>
> >>> On 2017-05-29 14:47, Mika Westerberg wrote:
> >>>> On Mon, May 29, 2017 at 01:33:29PM +0200, Jan Kiszka wrote:
> >>>>> Enhance acpi_load_table to also return the table index. Use that
> >>>>> index to unload the table again when the corresponding directory in
> >>>>> configfs gets removed. This allows to change SSDTs without rebooting
> >>> the system.
> >>>>> It also allows to destroy devices again that a dynamically loaded
> >>>>> SSDT created.
> >>>>>
> >>>>> This is widely similar to the DT overlay behavior.
> >>>>>
> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>> ---
> >>>>>
> >>>>> Can someone explain to me why an unloaded table still sticks around
> >>>>> in sysfs and why we cannot release its ID and rather have to use a
> >>>>> new one when loading a modified version?
> >>>>
> >>>> IIRC ACPICA relies the fact that SSDTs are never unloaded. Bob (CC'd)
> >>>> can correct me if I got it wrong.
> >>>
> >>
> >>
> >> [Moore, Robert]
> >>
> >> I'm not entirely sure what the table manager code looks like at this time, but ACPICA does in fact support table unloading.
> >>
> >> It is a rather dangerous thing to do, however -- unless you are careful about it. Basically, all handles that reference the table to be unloaded will go bad.
> > 
> > Right.
> > 
> > Linux should handle that in theory, but the code in there is mostly
> > very lightly tested AFAICS.
> > 
> >>> OK... Is that standard-driven or just a limitation of this
> >>> implementation?
> >>>
> >>> Is there an upper limit of tables? I'm thinking of lengthy development
> >>> sessions that play with tables, loading and unloading modified versions.
> >>>
> >>
> >> [Moore, Robert]
> >>
> >> I think that the maximum number of loaded ACPI tables is 255 at any given time. However, things are cleaned up after an unload such that repeated load/unload cycles should not consume resources.
> > 
> > I'm not sure if this is going to work seamlessly right away, but it
> > certainly can be made work.
> > 
> > That said, the change as proposed would be an API modification forcing
> > all of the OSes using ACPICA to change (or to carry out-of-the-tree
> > patches), so not nice.
> > 
> > What about adding a separate version of acpi_load_table() returning an
> > index (or an error on failures) instead of the status and leaving the
> > existing acpi_load_table() as is?
> 
> Sure. Any reference/preference in naming and locating that version?
> Should I leave acpica/ untouched at best? acpi_table_load/unload look
> simple enough to carry the logic in acpi_configfs directly.

Let's do that for the time being.

Later on we may decide to move that back into upstream ACPICA, but let's
get things to work on the Linux side first IMO.

Thanks,
Rafael