| Message ID | 9a6d4e50-41e7-28f6-b19d-f5f989da032d@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, On 14-03-17 09:56, Hans de Goede wrote: > Hi, > > On 14-03-17 09:15, Zheng, Lv wrote: >> Hi, Hans >> >>> From: Hans de Goede [mailto:hdegoede@redhat.com] >>> Subject: Re: [PATCH] ACPICA: Log Exceptions and Errors as warning while loading extra tables >>> >>> Hi, >>> >>> On 13-03-17 10:52, Zheng, Lv wrote: >>>> Hi, Hans >>>> >>>> For log level issue, is this fix useful for you? >>>> https://github.com/acpica/acpica/pull/121/commits/a505d3942 >>> >>> That fixes reloading already loaded tables, the problem I'm >>> getting errors about its loading a different table with identical >>> contents. >>> >>> I will look into your suggestion to do something similar to >>> AcpiTbInstallStandardTable using AcpiTbCompareTables for the >>> SSDT tables. I will send a new patch when I can make some time >>> to look into this. >> >> I just completed a prototype here: >> https://github.com/acpica/acpica/pull/121 >> >> I guess the original "duplicate table check" couldn't cover static SSDTs. >> Actually the duplicate table check should be a sanity check of table load. >> And for table install, we should have a different sanity check like: >> https://github.com/acpica/acpica/pull/121/commits/6e825cae5e5 >> I'm not sure if this is what you want. > > This checks for having 2 table_descriptors pointing to the same table > (address in memory). But in my case there are 2 identical copies of > the table at 2 different addresses in memory, so this won't work. > > After looking at this a bit myself, I think fixing this is actually > quite easy (now that you've pointed me to acpi_tb_install_standard_table() > > I've come up with the attached patch to fix this. I will give this a test > spin and then submit it officially (assuming it works). Ok the approach of doing the check during acpi_tb_install_standard_table does not work because then acpi_gbl_verify_table_checksum is false so we are only loading the header of the table, that and we are not supposed to load more data / use more mem this early, which the call to acpi_tb_acquire_table() will do for the table being compared against. So it looks like we will need to go with some version of my patch which does the check later when acpi_gbl_verify_table_checksum is true. Regards, Hans -- 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
Hi, Hans > From: Hans de Goede [mailto:hdegoede@redhat.com] > Subject: Re: [PATCH] ACPICA: Log Exceptions and Errors as warning while loading extra tables > > Hi, > > On 14-03-17 09:15, Zheng, Lv wrote: > > Hi, Hans > > > >> From: Hans de Goede [mailto:hdegoede@redhat.com] > >> Subject: Re: [PATCH] ACPICA: Log Exceptions and Errors as warning while loading extra tables > >> > >> Hi, > >> > >> On 13-03-17 10:52, Zheng, Lv wrote: > >>> Hi, Hans > >>> > >>> For log level issue, is this fix useful for you? > >>> https://github.com/acpica/acpica/pull/121/commits/a505d3942 > >> > >> That fixes reloading already loaded tables, the problem I'm > >> getting errors about its loading a different table with identical > >> contents. > >> > >> I will look into your suggestion to do something similar to > >> AcpiTbInstallStandardTable using AcpiTbCompareTables for the > >> SSDT tables. I will send a new patch when I can make some time > >> to look into this. > > > > I just completed a prototype here: > > https://github.com/acpica/acpica/pull/121 > > > > I guess the original "duplicate table check" couldn't cover static SSDTs. > > Actually the duplicate table check should be a sanity check of table load. > > And for table install, we should have a different sanity check like: > > https://github.com/acpica/acpica/pull/121/commits/6e825cae5e5 > > I'm not sure if this is what you want. > > This checks for having 2 table_descriptors pointing to the same table > (address in memory). But in my case there are 2 identical copies of > the table at 2 different addresses in memory, so this won't work. > > After looking at this a bit myself, I think fixing this is actually > quite easy (now that you've pointed me to acpi_tb_install_standard_table() > > I've come up with the attached patch to fix this. I will give this a test > spin and then submit it officially (assuming it works). In the pull request, there are 4 fixes: 1. add table install sanity check by detecting duplicates with table address and table type. 2. extend table load sanity check to LoadTable opcode. 3. extend table load sanity check to static loaded SSDTs - I think this should be able to cover your case. 4. if install/load sanity checks detect duplicate tables, they return AE_ALREADY_EXISTS and the error is silently discarded. That's the whole story for the pull request. I think you only checked one of them. Thanks and best regards Lv -- 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
From ded25995e0071c8e48f7e634e50efd9c675dcfce Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 14 Mar 2017 09:49:03 +0100
Subject: [PATCH v3] ACPICA: Always check for duplicate tables in
acpi_tb_install_standard_table
Always check for duplicate tables in acpi_tb_install_standard_table,
not just when the reload flag is set.
Some firmware contains duplicate tables in their RSDT or XSDT, leading
to a bunch of errors being printed on Linux boot.
This commit moves the check for duplicate tables in
acpi_tb_install_standard_table out of the if (reload) {} block,
to catch this and ignore the duplicate tables.
Note for reviewers: all this does is move the check outside of the
if (reload) {} block, no other changes are made.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/acpi/acpica/tbinstal.c | 86 +++++++++++++++++++++---------------------
1 file changed, 43 insertions(+), 43 deletions(-)
diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c
index 4620f3c..8622a73 100644
--- a/drivers/acpi/acpica/tbinstal.c
+++ b/drivers/acpi/acpica/tbinstal.c
@@ -250,54 +250,54 @@ acpi_tb_install_standard_table(acpi_physical_address address,
status = AE_BAD_SIGNATURE;
goto unlock_and_exit;
}
+ }
- /* Check if table is already registered */
+ /* Check if table is already registered */
- for (i = 0; i < acpi_gbl_root_table_list.current_table_count;
- ++i) {
- /*
- * Check for a table match on the entire table length,
- * not just the header.
- */
- if (!acpi_tb_compare_tables(&new_table_desc, i)) {
- continue;
- }
+ for (i = 0; i < acpi_gbl_root_table_list.current_table_count;
+ ++i) {
+ /*
+ * Check for a table match on the entire table length,
+ * not just the header.
+ */
+ if (!acpi_tb_compare_tables(&new_table_desc, i)) {
+ continue;
+ }
+ /*
+ * Note: the current mechanism does not unregister a table if it is
+ * dynamically unloaded. The related namespace entries are deleted,
+ * but the table remains in the root table list.
+ *
+ * The assumption here is that the number of different tables that
+ * will be loaded is actually small, and there is minimal overhead
+ * in just keeping the table in case it is needed again.
+ *
+ * If this assumption changes in the future (perhaps on large
+ * machines with many table load/unload operations), tables will
+ * need to be unregistered when they are unloaded, and slots in the
+ * root table list should be reused when empty.
+ */
+ if (acpi_gbl_root_table_list.tables[i].flags &
+ ACPI_TABLE_IS_LOADED) {
+
+ /* Table is still loaded, this is an error */
+
+ status = AE_ALREADY_EXISTS;
+ goto unlock_and_exit;
+ } else {
/*
- * Note: the current mechanism does not unregister a table if it is
- * dynamically unloaded. The related namespace entries are deleted,
- * but the table remains in the root table list.
- *
- * The assumption here is that the number of different tables that
- * will be loaded is actually small, and there is minimal overhead
- * in just keeping the table in case it is needed again.
- *
- * If this assumption changes in the future (perhaps on large
- * machines with many table load/unload operations), tables will
- * need to be unregistered when they are unloaded, and slots in the
- * root table list should be reused when empty.
+ * Table was unloaded, allow it to be reloaded.
+ * As we are going to return AE_OK to the caller, we should
+ * take the responsibility of freeing the input descriptor.
+ * Refill the input descriptor to ensure
+ * acpi_tb_install_table_with_override() can be called again to
+ * indicate the re-installation.
*/
- if (acpi_gbl_root_table_list.tables[i].flags &
- ACPI_TABLE_IS_LOADED) {
-
- /* Table is still loaded, this is an error */
-
- status = AE_ALREADY_EXISTS;
- goto unlock_and_exit;
- } else {
- /*
- * Table was unloaded, allow it to be reloaded.
- * As we are going to return AE_OK to the caller, we should
- * take the responsibility of freeing the input descriptor.
- * Refill the input descriptor to ensure
- * acpi_tb_install_table_with_override() can be called again to
- * indicate the re-installation.
- */
- acpi_tb_uninstall_table(&new_table_desc);
- (void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
- *table_index = i;
- return_ACPI_STATUS(AE_OK);
- }
+ acpi_tb_uninstall_table(&new_table_desc);
+ (void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
+ *table_index = i;
+ return_ACPI_STATUS(AE_OK);
}
}
--
2.9.3