This set of patches provide some cleanup in ACPI for minor issues
found while correcting a bogus error message (the first two patches),
and the correction for the error message itself (patch 3/3). Note
that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only
changes a comment and 2/3 makes an ACPI table parsing loop a wee bit
more robust.
For patch 3/3, many systems on boot have been reporting "Error parsing
PCC subspaces from PCCT" which turns out to not be an error at all.
The issue is that the probe for ACPI mailboxes defined in the PCCT
(Platform Communications Channel Table) makes a faulty assumption about
the content of the PCCT. What's more, when the error is reported, no
further PCC mailboxes are set up, even when they have been defined
in the PCCT. So, in the reported cases, there was no error and the
data in the PCCT was being ignored. This is described in more detail
in patch 3/3.
Since these patches primarily involve ACPI usages, it may make
sense for all of them to go through the linux-acpi tree; clearly,
this is up to the maintainers, though.
v3:
-- properly format docbook info in patch 1/3
-- remove extra parens in patch 2/3
-- clean up formatting, remove pr_warn() calls used in debugging but
providing no value, clean up docbook info for count_pcc_subspaces()
and parse_pcc_subspaces(), all in patch 3/3
v2:
-- removed one extraneous '+' in a comment in patch 3/3
-- fixed an if test that had a predicate that kbuild pointed out would
always be zero
Al Stone (3):
ACPI: improve function documentation for acpi_parse_entries_array()
ACPI: ensure acpi_parse_entries_array() does not access non-existent
table data
mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
drivers/acpi/tables.c | 12 ++++---
drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
2 files changed, 71 insertions(+), 37 deletions(-)
--
2.14.3
For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory. The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.
In the middle of this subtable loop, a callback is used to examine the
subtable in detail.
Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault. And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.
To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table. While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.
Found by inspection. There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().
Signed-off-by: Al Stone <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Len Brown <[email protected]>
---
drivers/acpi/tables.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 4a3410aa6540..82c3e2c52dd9 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
entry = (struct acpi_subtable_header *)
((unsigned long)table_header + table_size);
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
+ while ((unsigned long)entry + entry->length <= table_end) {
if (max_entries && count >= max_entries)
break;
--
2.14.3
There have been multiple reports of the following error message:
[ 0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 0000006E
Revision : 05
Checksum : A9
Oem ID : "XXXXXX"
Oem Table ID : "XXXXX "
Oem Revision : 00002280
Asl Compiler ID : "XXXX"
Asl Compiler Revision : 00000002
Flags (decoded below) : 00000001
Platform : 1
Reserved : 0000000000000000
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved : 000000000000
Base Address : 00000000DCE43018
Address Length : 0000000000001000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 0000000000001842
Preserve Mask : 00000000000000FD
Write Mask : 0000000000000002
Command Latency : 00001388
Maximum Access Rate : 00000000
Minimum Turnaround Time : 0000
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone <[email protected]>
Cc: Jassi Brar <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Len Brown <[email protected]>
---
drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
1 file changed, 63 insertions(+), 33 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..72af37d7e95e 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
.send_data = pcc_send_data,
};
+/*
+ *
+ * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: If we find a PCC subspace entry that is one of the types used
+ * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
+ *
+ * This gets called for each subtable in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *) header;
+
+ if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+ (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
+ return 0;
+
+ return -EINVAL;
+}
+
/**
- * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
- * entries. There should be one entry per PCC client.
+ * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
* @header: Pointer to the ACPI subtable header under the PCCT.
* @end: End of subtable entry.
*
- * Return: 0 for Success, else errno.
+ * Return: If we find a PCC subspace entry that is one of the types used
+ * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
*
* This gets called for each entry in the PCC table.
*/
@@ -393,10 +417,8 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
if ((pcct_ss->header.type !=
ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
&& (pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
- pr_err("Incorrect PCC Subspace type detected\n");
+ ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
return -EINVAL;
- }
}
return 0;
@@ -449,8 +471,8 @@ static int __init acpi_pcc_probe(void)
struct acpi_table_header *pcct_tbl;
struct acpi_subtable_header *pcct_entry;
struct acpi_table_pcct *acpi_pcct_tbl;
+ struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
int count, i, rc;
- int sum = 0;
acpi_status status = AE_OK;
/* Search for PCCT */
@@ -459,43 +481,45 @@ static int __init acpi_pcc_probe(void)
if (ACPI_FAILURE(status) || !pcct_tbl)
return -ENODEV;
- count = acpi_table_parse_entries(ACPI_SIG_PCCT,
- sizeof(struct acpi_table_pcct),
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
- parse_pcc_subspace, MAX_PCC_SUBSPACES);
- sum += (count > 0) ? count : 0;
-
- count = acpi_table_parse_entries(ACPI_SIG_PCCT,
- sizeof(struct acpi_table_pcct),
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
- parse_pcc_subspace, MAX_PCC_SUBSPACES);
- sum += (count > 0) ? count : 0;
+ /* Set up the subtable handlers */
+ for (i = ACPI_PCCT_TYPE_GENERIC_SUBSPACE;
+ i < ACPI_PCCT_TYPE_RESERVED; i++) {
+ proc[i].id = i;
+ proc[i].count = 0;
+ if (i == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
+ i == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
+ proc[i].handler = parse_pcc_subspace;
+ else
+ proc[i].handler = count_pcc_subspaces;
+ }
- if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
- pr_err("Error parsing PCC subspaces from PCCT\n");
+ count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
+ sizeof(struct acpi_table_pcct), proc,
+ ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
+ if (count == 0 || count > MAX_PCC_SUBSPACES) {
+ pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
return -EINVAL;
}
- pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
- sum, GFP_KERNEL);
+ pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) * count, GFP_KERNEL);
if (!pcc_mbox_channels) {
pr_err("Could not allocate space for PCC mbox channels\n");
return -ENOMEM;
}
- pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+ pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
if (!pcc_doorbell_vaddr) {
rc = -ENOMEM;
goto err_free_mbox;
}
- pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+ pcc_doorbell_ack_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
if (!pcc_doorbell_ack_vaddr) {
rc = -ENOMEM;
goto err_free_db_vaddr;
}
- pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
+ pcc_doorbell_irq = kcalloc(count, sizeof(int), GFP_KERNEL);
if (!pcc_doorbell_irq) {
rc = -ENOMEM;
goto err_free_db_ack_vaddr;
@@ -509,18 +533,24 @@ static int __init acpi_pcc_probe(void)
if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
pcc_mbox_ctrl.txdone_irq = true;
- for (i = 0; i < sum; i++) {
+ for (i = 0; i < count; i++) {
struct acpi_generic_address *db_reg;
- struct acpi_pcct_hw_reduced *pcct_ss;
+ struct acpi_pcct_subspace *pcct_ss;
pcc_mbox_channels[i].con_priv = pcct_entry;
- pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
+ if (pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
+ pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+ struct acpi_pcct_hw_reduced *pcct_hrss;
+
+ pcct_hrss = (struct acpi_pcct_hw_reduced *) pcct_entry;
- if (pcc_mbox_ctrl.txdone_irq) {
- rc = pcc_parse_subspace_irq(i, pcct_ss);
- if (rc < 0)
- goto err;
+ if (pcc_mbox_ctrl.txdone_irq) {
+ rc = pcc_parse_subspace_irq(i, pcct_hrss);
+ if (rc < 0)
+ goto err;
+ }
}
+ pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
/* If doorbell is in system memory cache the virt address */
db_reg = &pcct_ss->doorbell_register;
@@ -531,7 +561,7 @@ static int __init acpi_pcc_probe(void)
((unsigned long) pcct_entry + pcct_entry->length);
}
- pcc_mbox_ctrl.num_chans = sum;
+ pcc_mbox_ctrl.num_chans = count;
pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
--
2.14.3
I found the description of the table_size argument to the function
acpi_parse_entries_array() unclear and ambiguous. This is a minor
documentation change to improve that description so I don't misuse
the argument again in the future, and it is hopefully clearer to
other future users.
Signed-off-by: Al Stone <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Len Brown <[email protected]>
---
drivers/acpi/tables.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 849c4fb19b03..4a3410aa6540 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -222,7 +222,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
* acpi_parse_entries_array - for each proc_num find a suitable subtable
*
* @id: table id (for debugging purposes)
- * @table_size: single entry size
+ * @table_size: size of the root table
* @table_header: where does the table start?
* @proc: array of acpi_subtable_proc struct containing entry id
* and associated handler with it
@@ -233,6 +233,11 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
* on it. Assumption is that there's only single handler for particular
* entry id.
*
+ * The table_size is not the size of the complete ACPI table (the length
+ * field in the header struct), but only the size of the root table; i.e.,
+ * the offset from the very first byte of the complete ACPI table, to the
+ * first byte of the very first subtable.
+ *
* On success returns sum of all matching entries for all proc handlers.
* Otherwise, -ENODEV or -EINVAL is returned.
*/
@@ -400,7 +405,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
return -ENODEV;
}
-/*
+/*
* The BIOS is supposed to supply a single APIC/MADT,
* but some report two. Provide a knob to use either.
* (don't you wish instance 0 and 1 were not the same?)
--
2.14.3
On Tue, May 1, 2018 at 2:39 AM, Al Stone <[email protected]> wrote:
> There have been multiple reports of the following error message:
>
> [ 0.068293] Error parsing PCC subspaces from PCCT
>
> This error message is not correct. In multiple cases examined, the PCCT
> (Platform Communications Channel Table) concerned is actually properly
> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
> is making the assumption that the only valid PCCT is one that contains
> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
> types are counted and as long as there is at least one of the desired
> types, the acpi_pcc_probe() succeeds. When no subtables of these types
> are found, regardless of whether or not any other subtable types are
> present, the error mentioned above is reported.
>
> In the cases reported to me personally, the PCCT contains exactly one
> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
> acpi_pcc_probe() does not count it as a valid subtable, so believes
> there to be no valid subtables, and hence outputs the error message.
>
> An example of the PCCT being reported as erroneous yet perfectly fine
> is the following:
>
> Signature : "PCCT"
> Table Length : 0000006E
> Revision : 05
> Checksum : A9
> Oem ID : "XXXXXX"
> Oem Table ID : "XXXXX "
> Oem Revision : 00002280
> Asl Compiler ID : "XXXX"
> Asl Compiler Revision : 00000002
>
> Flags (decoded below) : 00000001
> Platform : 1
> Reserved : 0000000000000000
>
> Subtable Type : 00 [Generic Communications Subspace]
> Length : 3E
>
> Reserved : 000000000000
> Base Address : 00000000DCE43018
> Address Length : 0000000000001000
>
> Doorbell Register : [Generic Address Structure]
> Space ID : 01 [SystemIO]
> Bit Width : 08
> Bit Offset : 00
> Encoded Access Width : 01 [Byte Access:8]
> Address : 0000000000001842
>
> Preserve Mask : 00000000000000FD
> Write Mask : 0000000000000002
> Command Latency : 00001388
> Maximum Access Rate : 00000000
> Minimum Turnaround Time : 0000
>
> To fix this, we count up all of the possible subtable types for the
> PCCT, and only report an error when there are none (which could mean
> either no subtables, or no valid subtables), or there are too many.
> We also change the logic so that if there is a valid subtable, we
> do try to initialize it per the PCCT subtable contents. This is a
> change in functionality; previously, the probe would have returned
> right after the error message and would not have tried to use any
> other subtable definition.
>
> Tested on my personal laptop which showed the error previously; the
> error message no longer appears and the laptop appears to operate
> normally.
>
> Signed-off-by: Al Stone <[email protected]>
> Cc: Jassi Brar <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: Len Brown <[email protected]>
Prashanth, any commens or concerns regarding this?
> ---
> drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 3ef7f036ceea..72af37d7e95e 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
> .send_data = pcc_send_data,
> };
>
> +/*
> + *
> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
> + * @header: Pointer to the ACPI subtable header under the PCCT.
> + * @end: End of subtable entry.
> + *
> + * Return: If we find a PCC subspace entry that is one of the types used
> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
> + *
> + * This gets called for each subtable in the PCC table.
> + */
> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *) header;
> +
> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
> + return 0;
> +
> + return -EINVAL;
> +}
> +
> /**
> - * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
> - * entries. There should be one entry per PCC client.
> + * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
> * @header: Pointer to the ACPI subtable header under the PCCT.
> * @end: End of subtable entry.
> *
> - * Return: 0 for Success, else errno.
> + * Return: If we find a PCC subspace entry that is one of the types used
> + * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
> *
> * This gets called for each entry in the PCC table.
> */
> @@ -393,10 +417,8 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
> if ((pcct_ss->header.type !=
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
> && (pcct_ss->header.type !=
> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
> - pr_err("Incorrect PCC Subspace type detected\n");
> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
> return -EINVAL;
> - }
> }
>
> return 0;
> @@ -449,8 +471,8 @@ static int __init acpi_pcc_probe(void)
> struct acpi_table_header *pcct_tbl;
> struct acpi_subtable_header *pcct_entry;
> struct acpi_table_pcct *acpi_pcct_tbl;
> + struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
> int count, i, rc;
> - int sum = 0;
> acpi_status status = AE_OK;
>
> /* Search for PCCT */
> @@ -459,43 +481,45 @@ static int __init acpi_pcc_probe(void)
> if (ACPI_FAILURE(status) || !pcct_tbl)
> return -ENODEV;
>
> - count = acpi_table_parse_entries(ACPI_SIG_PCCT,
> - sizeof(struct acpi_table_pcct),
> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
> - parse_pcc_subspace, MAX_PCC_SUBSPACES);
> - sum += (count > 0) ? count : 0;
> -
> - count = acpi_table_parse_entries(ACPI_SIG_PCCT,
> - sizeof(struct acpi_table_pcct),
> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
> - parse_pcc_subspace, MAX_PCC_SUBSPACES);
> - sum += (count > 0) ? count : 0;
> + /* Set up the subtable handlers */
> + for (i = ACPI_PCCT_TYPE_GENERIC_SUBSPACE;
> + i < ACPI_PCCT_TYPE_RESERVED; i++) {
> + proc[i].id = i;
> + proc[i].count = 0;
> + if (i == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
> + i == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
> + proc[i].handler = parse_pcc_subspace;
> + else
> + proc[i].handler = count_pcc_subspaces;
> + }
>
> - if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
> - pr_err("Error parsing PCC subspaces from PCCT\n");
> + count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
> + sizeof(struct acpi_table_pcct), proc,
> + ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
> + if (count == 0 || count > MAX_PCC_SUBSPACES) {
> + pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
> return -EINVAL;
> }
>
> - pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
> - sum, GFP_KERNEL);
> + pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) * count, GFP_KERNEL);
> if (!pcc_mbox_channels) {
> pr_err("Could not allocate space for PCC mbox channels\n");
> return -ENOMEM;
> }
>
> - pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
> + pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
> if (!pcc_doorbell_vaddr) {
> rc = -ENOMEM;
> goto err_free_mbox;
> }
>
> - pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
> + pcc_doorbell_ack_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
> if (!pcc_doorbell_ack_vaddr) {
> rc = -ENOMEM;
> goto err_free_db_vaddr;
> }
>
> - pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
> + pcc_doorbell_irq = kcalloc(count, sizeof(int), GFP_KERNEL);
> if (!pcc_doorbell_irq) {
> rc = -ENOMEM;
> goto err_free_db_ack_vaddr;
> @@ -509,18 +533,24 @@ static int __init acpi_pcc_probe(void)
> if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
> pcc_mbox_ctrl.txdone_irq = true;
>
> - for (i = 0; i < sum; i++) {
> + for (i = 0; i < count; i++) {
> struct acpi_generic_address *db_reg;
> - struct acpi_pcct_hw_reduced *pcct_ss;
> + struct acpi_pcct_subspace *pcct_ss;
> pcc_mbox_channels[i].con_priv = pcct_entry;
>
> - pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
> + if (pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
> + pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> + struct acpi_pcct_hw_reduced *pcct_hrss;
> +
> + pcct_hrss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>
> - if (pcc_mbox_ctrl.txdone_irq) {
> - rc = pcc_parse_subspace_irq(i, pcct_ss);
> - if (rc < 0)
> - goto err;
> + if (pcc_mbox_ctrl.txdone_irq) {
> + rc = pcc_parse_subspace_irq(i, pcct_hrss);
> + if (rc < 0)
> + goto err;
> + }
> }
> + pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
>
> /* If doorbell is in system memory cache the virt address */
> db_reg = &pcct_ss->doorbell_register;
> @@ -531,7 +561,7 @@ static int __init acpi_pcc_probe(void)
> ((unsigned long) pcct_entry + pcct_entry->length);
> }
>
> - pcc_mbox_ctrl.num_chans = sum;
> + pcc_mbox_ctrl.num_chans = count;
>
> pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
>
> --
> 2.14.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, May 1, 2018 2:39:04 AM CEST Al Stone wrote:
> This set of patches provide some cleanup in ACPI for minor issues
> found while correcting a bogus error message (the first two patches),
> and the correction for the error message itself (patch 3/3). Note
> that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only
> changes a comment and 2/3 makes an ACPI table parsing loop a wee bit
> more robust.
>
> For patch 3/3, many systems on boot have been reporting "Error parsing
> PCC subspaces from PCCT" which turns out to not be an error at all.
> The issue is that the probe for ACPI mailboxes defined in the PCCT
> (Platform Communications Channel Table) makes a faulty assumption about
> the content of the PCCT. What's more, when the error is reported, no
> further PCC mailboxes are set up, even when they have been defined
> in the PCCT. So, in the reported cases, there was no error and the
> data in the PCCT was being ignored. This is described in more detail
> in patch 3/3.
>
> Since these patches primarily involve ACPI usages, it may make
> sense for all of them to go through the linux-acpi tree; clearly,
> this is up to the maintainers, though.
>
> v3:
> -- properly format docbook info in patch 1/3
> -- remove extra parens in patch 2/3
> -- clean up formatting, remove pr_warn() calls used in debugging but
> providing no value, clean up docbook info for count_pcc_subspaces()
> and parse_pcc_subspaces(), all in patch 3/3
>
> v2:
> -- removed one extraneous '+' in a comment in patch 3/3
> -- fixed an if test that had a predicate that kbuild pointed out would
> always be zero
>
> Al Stone (3):
> ACPI: improve function documentation for acpi_parse_entries_array()
> ACPI: ensure acpi_parse_entries_array() does not access non-existent
> table data
> mailbox: ACPI: erroneous error message when parsing the ACPI PCCT
>
> drivers/acpi/tables.c | 12 ++++---
> drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
> 2 files changed, 71 insertions(+), 37 deletions(-)
I've applied [1-2/3] and I'm waiting on input from Prashanth on the [3/3].
Thanks,
Rafael
On 4/30/2018 6:39 PM, Al Stone wrote:
> There have been multiple reports of the following error message:
>
> [ 0.068293] Error parsing PCC subspaces from PCCT
>
> This error message is not correct. In multiple cases examined, the PCCT
> (Platform Communications Channel Table) concerned is actually properly
> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
> is making the assumption that the only valid PCCT is one that contains
> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
> types are counted and as long as there is at least one of the desired
> types, the acpi_pcc_probe() succeeds. When no subtables of these types
> are found, regardless of whether or not any other subtable types are
> present, the error mentioned above is reported.
>
> In the cases reported to me personally, the PCCT contains exactly one
> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
> acpi_pcc_probe() does not count it as a valid subtable, so believes
> there to be no valid subtables, and hence outputs the error message.
>
> An example of the PCCT being reported as erroneous yet perfectly fine
> is the following:
>
> Signature : "PCCT"
> Table Length : 0000006E
> Revision : 05
> Checksum : A9
> Oem ID : "XXXXXX"
> Oem Table ID : "XXXXX "
> Oem Revision : 00002280
> Asl Compiler ID : "XXXX"
> Asl Compiler Revision : 00000002
>
> Flags (decoded below) : 00000001
> Platform : 1
> Reserved : 0000000000000000
>
> Subtable Type : 00 [Generic Communications Subspace]
> Length : 3E
>
> Reserved : 000000000000
> Base Address : 00000000DCE43018
> Address Length : 0000000000001000
>
> Doorbell Register : [Generic Address Structure]
> Space ID : 01 [SystemIO]
> Bit Width : 08
> Bit Offset : 00
> Encoded Access Width : 01 [Byte Access:8]
> Address : 0000000000001842
>
> Preserve Mask : 00000000000000FD
> Write Mask : 0000000000000002
> Command Latency : 00001388
> Maximum Access Rate : 00000000
> Minimum Turnaround Time : 0000
>
> To fix this, we count up all of the possible subtable types for the
> PCCT, and only report an error when there are none (which could mean
> either no subtables, or no valid subtables), or there are too many.
> We also change the logic so that if there is a valid subtable, we
> do try to initialize it per the PCCT subtable contents. This is a
> change in functionality; previously, the probe would have returned
> right after the error message and would not have tried to use any
> other subtable definition.
>
> Tested on my personal laptop which showed the error previously; the
> error message no longer appears and the laptop appears to operate
> normally.
>
> Signed-off-by: Al Stone <[email protected]>
> Cc: Jassi Brar <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: Len Brown <[email protected]>
> ---
> drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 3ef7f036ceea..72af37d7e95e 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
> .send_data = pcc_send_data,
> };
>
> +/*
> + *
> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
> + * @header: Pointer to the ACPI subtable header under the PCCT.
> + * @end: End of subtable entry.
> + *
> + * Return: If we find a PCC subspace entry that is one of the types used
> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
> + *
> + * This gets called for each subtable in the PCC table.
> + */
> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *) header;
> +
> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
> + return 0;
> +
> + return -EINVAL;
> +}
> +
> /**
> - * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
> - * entries. There should be one entry per PCC client.
> + * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
> * @header: Pointer to the ACPI subtable header under the PCCT.
> * @end: End of subtable entry.
> *
> - * Return: 0 for Success, else errno.
> + * Return: If we find a PCC subspace entry that is one of the types used
> + * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
> *
> * This gets called for each entry in the PCC table.
> */
> @@ -393,10 +417,8 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
> if ((pcct_ss->header.type !=
> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
> && (pcct_ss->header.type !=
> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
> - pr_err("Incorrect PCC Subspace type detected\n");
> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
> return -EINVAL;
> - }
> }
>
> return 0;
Can't we combine parse_pcc_subspace and count_pcc_subspaces into a
single function? parse_pcc_subspace can return 0 for supported subspace
types and -EINVAL for others.
The limitation on number of subspaces(max = 256) applies to all types of PCC
subspaces (see Table 14-351 in ACPI 6.2). The MAX_PCC_SUBSPACES check in
parse_pcc_subspace seems invalid as pcc_mbox_ctrl.num_chans will not be
initialized yet at that moment.
Given above, I think it is probably better to update parse_pcc_subspace to
only check for a valid PCC subspace type. The check to make sure overall count
of subspace is less than 256 is already present following the call to
acpi_table_parse_entries_array().
--
Thanks,
Prashanth
On 05/14/2018 03:04 PM, Prakash, Prashanth wrote:
>
>
> On 4/30/2018 6:39 PM, Al Stone wrote:
>> There have been multiple reports of the following error message:
>>
>> [ 0.068293] Error parsing PCC subspaces from PCCT
>>
>> This error message is not correct. In multiple cases examined, the PCCT
>> (Platform Communications Channel Table) concerned is actually properly
>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
>> is making the assumption that the only valid PCCT is one that contains
>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
>> types are counted and as long as there is at least one of the desired
>> types, the acpi_pcc_probe() succeeds. When no subtables of these types
>> are found, regardless of whether or not any other subtable types are
>> present, the error mentioned above is reported.
>>
>> In the cases reported to me personally, the PCCT contains exactly one
>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
>> acpi_pcc_probe() does not count it as a valid subtable, so believes
>> there to be no valid subtables, and hence outputs the error message.
>>
>> An example of the PCCT being reported as erroneous yet perfectly fine
>> is the following:
>>
>> Signature : "PCCT"
>> Table Length : 0000006E
>> Revision : 05
>> Checksum : A9
>> Oem ID : "XXXXXX"
>> Oem Table ID : "XXXXX "
>> Oem Revision : 00002280
>> Asl Compiler ID : "XXXX"
>> Asl Compiler Revision : 00000002
>>
>> Flags (decoded below) : 00000001
>> Platform : 1
>> Reserved : 0000000000000000
>>
>> Subtable Type : 00 [Generic Communications Subspace]
>> Length : 3E
>>
>> Reserved : 000000000000
>> Base Address : 00000000DCE43018
>> Address Length : 0000000000001000
>>
>> Doorbell Register : [Generic Address Structure]
>> Space ID : 01 [SystemIO]
>> Bit Width : 08
>> Bit Offset : 00
>> Encoded Access Width : 01 [Byte Access:8]
>> Address : 0000000000001842
>>
>> Preserve Mask : 00000000000000FD
>> Write Mask : 0000000000000002
>> Command Latency : 00001388
>> Maximum Access Rate : 00000000
>> Minimum Turnaround Time : 0000
>>
>> To fix this, we count up all of the possible subtable types for the
>> PCCT, and only report an error when there are none (which could mean
>> either no subtables, or no valid subtables), or there are too many.
>> We also change the logic so that if there is a valid subtable, we
>> do try to initialize it per the PCCT subtable contents. This is a
>> change in functionality; previously, the probe would have returned
>> right after the error message and would not have tried to use any
>> other subtable definition.
>>
>> Tested on my personal laptop which showed the error previously; the
>> error message no longer appears and the laptop appears to operate
>> normally.
>>
>> Signed-off-by: Al Stone <[email protected]>
>> Cc: Jassi Brar <[email protected]>
>> Cc: Rafael J. Wysocki <[email protected]>
>> Cc: Len Brown <[email protected]>
>> ---
>> drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 63 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 3ef7f036ceea..72af37d7e95e 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>> .send_data = pcc_send_data,
>> };
>>
>> +/*
>> + *
>> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>> + * @end: End of subtable entry.
>> + *
>> + * Return: If we find a PCC subspace entry that is one of the types used
>> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
>> + *
>> + * This gets called for each subtable in the PCC table.
>> + */
>> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *) header;
>> +
>> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
>> + return 0;
>> +
>> + return -EINVAL;
>> +}
>> +
>> /**
>> - * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
>> - * entries. There should be one entry per PCC client.
>> + * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
>> * @header: Pointer to the ACPI subtable header under the PCCT.
>> * @end: End of subtable entry.
>> *
>> - * Return: 0 for Success, else errno.
>> + * Return: If we find a PCC subspace entry that is one of the types used
>> + * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
>> *
>> * This gets called for each entry in the PCC table.
>> */
>> @@ -393,10 +417,8 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
>> if ((pcct_ss->header.type !=
>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
>> && (pcct_ss->header.type !=
>> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>> - pr_err("Incorrect PCC Subspace type detected\n");
>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
>> return -EINVAL;
>> - }
>> }
>>
>> return 0;
> Can't we combine parse_pcc_subspace and count_pcc_subspaces into a
> single function? parse_pcc_subspace can return 0 for supported subspace
> types and -EINVAL for others.
I did think about that. The issue is that we have subspaces that are only
valid in reduced hardware systems, and subspaces that are not. It might make
sense to use different names, as in 'count_reduced_hw_subspaces()' and
'count_general_subspaces()' (or something like those) but we do have the two
separate classes and hardware belonging to each of those classes.
That being said, you raise a good point: this would only be useful if the
mailbox code needed to know the classes of subspaces were different; I saw
no such code but I could have missed it. If you're aware of any such cases,
let me know. Otherwise, I'll combine the two counting routines and test it.
> The limitation on number of subspaces(max = 256) applies to all types of PCC
> subspaces (see Table 14-351 in ACPI 6.2). The MAX_PCC_SUBSPACES check in
> parse_pcc_subspace seems invalid as pcc_mbox_ctrl.num_chans will not be
> initialized yet at that moment.
Good catch. Thanks. That test was there prior to my patches, but I'll pull
it out.
> Given above, I think it is probably better to update parse_pcc_subspace to
> only check for a valid PCC subspace type. The check to make sure overall count
> of subspace is less than 256 is already present following the call to
> acpi_table_parse_entries_array().
>
> --
> Thanks,
> Prashanth
>
Thanks, Prashanth.
Rafael: do you want me to just re-send this patch or the whole series? Either
way works for me; what's easiest for you since the first two have been applied?
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On Tue, May 15, 2018 at 12:49 AM, Al Stone <[email protected]> wrote:
> On 05/14/2018 03:04 PM, Prakash, Prashanth wrote:
>>
>>
>> On 4/30/2018 6:39 PM, Al Stone wrote:
>>> There have been multiple reports of the following error message:
>>>
>>> [ 0.068293] Error parsing PCC subspaces from PCCT
>>>
>>> This error message is not correct. In multiple cases examined, the PCCT
>>> (Platform Communications Channel Table) concerned is actually properly
>>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
>>> is making the assumption that the only valid PCCT is one that contains
>>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
>>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
>>> types are counted and as long as there is at least one of the desired
>>> types, the acpi_pcc_probe() succeeds. When no subtables of these types
>>> are found, regardless of whether or not any other subtable types are
>>> present, the error mentioned above is reported.
>>>
>>> In the cases reported to me personally, the PCCT contains exactly one
>>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
>>> acpi_pcc_probe() does not count it as a valid subtable, so believes
>>> there to be no valid subtables, and hence outputs the error message.
>>>
>>> An example of the PCCT being reported as erroneous yet perfectly fine
>>> is the following:
>>>
>>> Signature : "PCCT"
>>> Table Length : 0000006E
>>> Revision : 05
>>> Checksum : A9
>>> Oem ID : "XXXXXX"
>>> Oem Table ID : "XXXXX "
>>> Oem Revision : 00002280
>>> Asl Compiler ID : "XXXX"
>>> Asl Compiler Revision : 00000002
>>>
>>> Flags (decoded below) : 00000001
>>> Platform : 1
>>> Reserved : 0000000000000000
>>>
>>> Subtable Type : 00 [Generic Communications Subspace]
>>> Length : 3E
>>>
>>> Reserved : 000000000000
>>> Base Address : 00000000DCE43018
>>> Address Length : 0000000000001000
>>>
>>> Doorbell Register : [Generic Address Structure]
>>> Space ID : 01 [SystemIO]
>>> Bit Width : 08
>>> Bit Offset : 00
>>> Encoded Access Width : 01 [Byte Access:8]
>>> Address : 0000000000001842
>>>
>>> Preserve Mask : 00000000000000FD
>>> Write Mask : 0000000000000002
>>> Command Latency : 00001388
>>> Maximum Access Rate : 00000000
>>> Minimum Turnaround Time : 0000
>>>
>>> To fix this, we count up all of the possible subtable types for the
>>> PCCT, and only report an error when there are none (which could mean
>>> either no subtables, or no valid subtables), or there are too many.
>>> We also change the logic so that if there is a valid subtable, we
>>> do try to initialize it per the PCCT subtable contents. This is a
>>> change in functionality; previously, the probe would have returned
>>> right after the error message and would not have tried to use any
>>> other subtable definition.
>>>
>>> Tested on my personal laptop which showed the error previously; the
>>> error message no longer appears and the laptop appears to operate
>>> normally.
>>>
>>> Signed-off-by: Al Stone <[email protected]>
>>> Cc: Jassi Brar <[email protected]>
>>> Cc: Rafael J. Wysocki <[email protected]>
>>> Cc: Len Brown <[email protected]>
>>> ---
>>> drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
>>> 1 file changed, 63 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index 3ef7f036ceea..72af37d7e95e 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>>> .send_data = pcc_send_data,
>>> };
>>>
>>> +/*
>>> + *
>>> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
>>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>>> + * @end: End of subtable entry.
>>> + *
>>> + * Return: If we find a PCC subspace entry that is one of the types used
>>> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
>>> + *
>>> + * This gets called for each subtable in the PCC table.
>>> + */
>>> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
>>> + const unsigned long end)
>>> +{
>>> + struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *) header;
>>> +
>>> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
>>> + return 0;
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> /**
>>> - * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
>>> - * entries. There should be one entry per PCC client.
>>> + * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
>>> * @header: Pointer to the ACPI subtable header under the PCCT.
>>> * @end: End of subtable entry.
>>> *
>>> - * Return: 0 for Success, else errno.
>>> + * Return: If we find a PCC subspace entry that is one of the types used
>>> + * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
>>> *
>>> * This gets called for each entry in the PCC table.
>>> */
>>> @@ -393,10 +417,8 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
>>> if ((pcct_ss->header.type !=
>>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
>>> && (pcct_ss->header.type !=
>>> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>>> - pr_err("Incorrect PCC Subspace type detected\n");
>>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
>>> return -EINVAL;
>>> - }
>>> }
>>>
>>> return 0;
>> Can't we combine parse_pcc_subspace and count_pcc_subspaces into a
>> single function? parse_pcc_subspace can return 0 for supported subspace
>> types and -EINVAL for others.
>
> I did think about that. The issue is that we have subspaces that are only
> valid in reduced hardware systems, and subspaces that are not. It might make
> sense to use different names, as in 'count_reduced_hw_subspaces()' and
> 'count_general_subspaces()' (or something like those) but we do have the two
> separate classes and hardware belonging to each of those classes.
>
> That being said, you raise a good point: this would only be useful if the
> mailbox code needed to know the classes of subspaces were different; I saw
> no such code but I could have missed it. If you're aware of any such cases,
> let me know. Otherwise, I'll combine the two counting routines and test it.
>
>> The limitation on number of subspaces(max = 256) applies to all types of PCC
>> subspaces (see Table 14-351 in ACPI 6.2). The MAX_PCC_SUBSPACES check in
>> parse_pcc_subspace seems invalid as pcc_mbox_ctrl.num_chans will not be
>> initialized yet at that moment.
>
> Good catch. Thanks. That test was there prior to my patches, but I'll pull
> it out.
>
>> Given above, I think it is probably better to update parse_pcc_subspace to
>> only check for a valid PCC subspace type. The check to make sure overall count
>> of subspace is less than 256 is already present following the call to
>> acpi_table_parse_entries_array().
>>
>> --
>> Thanks,
>> Prashanth
>>
>
> Thanks, Prashanth.
>
> Rafael: do you want me to just re-send this patch or the whole series? Either
> way works for me; what's easiest for you since the first two have been applied?
Just this patch, please.
I've applied the other two already.
Thanks,
Rafael
On Tue, May 1, 2018 at 2:39 AM, Al Stone <[email protected]> wrote:
> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
> to step through each of the subtables in memory. The primary loop for this
> was checking that the beginning location of the subtable being examined
> plus the length of struct acpi_subtable_header was not beyond the end of
> the complete ACPI table; if it wasn't, the subtable could be examined, but
> if it was the loop would terminate as it should.
>
> In the middle of this subtable loop, a callback is used to examine the
> subtable in detail.
>
> Should the callback function try to examine elements of the subtable that
> are located past the subtable header, and the ACPI table containing this
> subtable has an incorrect length, it is possible to access either invalid
> or protected memory and cause a fault. And, the length of struct
> acpi_subtable_header will always be smaller than the length of the actual
> subtable.
>
> To fix this, we make the main loop check that the beginning of the
> subtable being examined plus the actual length of the subtable does
> not go past the end of the enclosing ACPI table. While this cannot
> protect us from malicious callback functions, it can prevent us from
> failing because of some poorly constructed ACPI tables.
>
> Found by inspection. There is no functional change to existing code
> that is known to work when calling acpi_parse_entries_array().
>
> Signed-off-by: Al Stone <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: Len Brown <[email protected]>
> ---
> drivers/acpi/tables.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 4a3410aa6540..82c3e2c52dd9 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
> entry = (struct acpi_subtable_header *)
> ((unsigned long)table_header + table_size);
>
> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
> - table_end) {
> + while ((unsigned long)entry + entry->length <= table_end) {
> if (max_entries && count >= max_entries)
> break;
>
> --
This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
other things), so I'm dropping it. I can send you acpidump output
from that machine if need be.
Thanks,
Rafael
On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote:
> On Tue, May 1, 2018 at 2:39 AM, Al Stone <[email protected]> wrote:
>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>> to step through each of the subtables in memory. The primary loop for this
>> was checking that the beginning location of the subtable being examined
>> plus the length of struct acpi_subtable_header was not beyond the end of
>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>> if it was the loop would terminate as it should.
>>
>> In the middle of this subtable loop, a callback is used to examine the
>> subtable in detail.
>>
>> Should the callback function try to examine elements of the subtable that
>> are located past the subtable header, and the ACPI table containing this
>> subtable has an incorrect length, it is possible to access either invalid
>> or protected memory and cause a fault. And, the length of struct
>> acpi_subtable_header will always be smaller than the length of the actual
>> subtable.
>>
>> To fix this, we make the main loop check that the beginning of the
>> subtable being examined plus the actual length of the subtable does
>> not go past the end of the enclosing ACPI table. While this cannot
>> protect us from malicious callback functions, it can prevent us from
>> failing because of some poorly constructed ACPI tables.
>>
>> Found by inspection. There is no functional change to existing code
>> that is known to work when calling acpi_parse_entries_array().
>>
>> Signed-off-by: Al Stone <[email protected]>
>> Cc: Rafael J. Wysocki <[email protected]>
>> Cc: Len Brown <[email protected]>
>> ---
>> drivers/acpi/tables.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 4a3410aa6540..82c3e2c52dd9 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>> entry = (struct acpi_subtable_header *)
>> ((unsigned long)table_header + table_size);
>>
>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>> - table_end) {
>> + while ((unsigned long)entry + entry->length <= table_end) {
>> if (max_entries && count >= max_entries)
>> break;
>>
>> --
>
> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
> other things), so I'm dropping it. I can send you acpidump output
> from that machine if need be.
>
> Thanks,
> Rafael
>
Yes, please. My fear is that there are a bunch of MADT tables in the real world
that aren't quite right so that while this may be theoretically correct, it may
be wrong as a practical matter.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On 05/15/2018 03:53 PM, Al Stone wrote:
> On 05/15/2018 11:19 AM, Rafael J. Wysocki wrote:
>> On Tue, May 1, 2018 at 2:39 AM, Al Stone <[email protected]> wrote:
>>> For ACPI tables that have subtables, acpi_parse_entries_array() gets used
>>> to step through each of the subtables in memory. The primary loop for this
>>> was checking that the beginning location of the subtable being examined
>>> plus the length of struct acpi_subtable_header was not beyond the end of
>>> the complete ACPI table; if it wasn't, the subtable could be examined, but
>>> if it was the loop would terminate as it should.
>>>
>>> In the middle of this subtable loop, a callback is used to examine the
>>> subtable in detail.
>>>
>>> Should the callback function try to examine elements of the subtable that
>>> are located past the subtable header, and the ACPI table containing this
>>> subtable has an incorrect length, it is possible to access either invalid
>>> or protected memory and cause a fault. And, the length of struct
>>> acpi_subtable_header will always be smaller than the length of the actual
>>> subtable.
>>>
>>> To fix this, we make the main loop check that the beginning of the
>>> subtable being examined plus the actual length of the subtable does
>>> not go past the end of the enclosing ACPI table. While this cannot
>>> protect us from malicious callback functions, it can prevent us from
>>> failing because of some poorly constructed ACPI tables.
>>>
>>> Found by inspection. There is no functional change to existing code
>>> that is known to work when calling acpi_parse_entries_array().
>>>
>>> Signed-off-by: Al Stone <[email protected]>
>>> Cc: Rafael J. Wysocki <[email protected]>
>>> Cc: Len Brown <[email protected]>
>>> ---
>>> drivers/acpi/tables.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>>> index 4a3410aa6540..82c3e2c52dd9 100644
>>> --- a/drivers/acpi/tables.c
>>> +++ b/drivers/acpi/tables.c
>>> @@ -274,8 +274,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>>> entry = (struct acpi_subtable_header *)
>>> ((unsigned long)table_header + table_size);
>>>
>>> - while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>>> - table_end) {
>>> + while ((unsigned long)entry + entry->length <= table_end) {
>>> if (max_entries && count >= max_entries)
>>> break;
>>>
>>> --
>>
>> This breaks the CPU enumeration on my Dell XPS13 9360 (possibly among
>> other things), so I'm dropping it. I can send you acpidump output
>> from that machine if need be.
>>
>> Thanks,
>> Rafael
Let's just drop this completely -- but please do send the acpidump. It's
going to take me a while to figure why this innocuous little loop does not
behave the way I expect it to. I'll send a separate patch, if needed.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
There have been multiple reports of the following error message:
[ 0.068293] Error parsing PCC subspaces from PCCT
This error message is not correct. In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds. When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.
In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.
An example of the PCCT being reported as erroneous yet perfectly fine
is the following:
Signature : "PCCT"
Table Length : 0000006E
Revision : 05
Checksum : A9
Oem ID : "XXXXXX"
Oem Table ID : "XXXXX "
Oem Revision : 00002280
Asl Compiler ID : "XXXX"
Asl Compiler Revision : 00000002
Flags (decoded below) : 00000001
Platform : 1
Reserved : 0000000000000000
Subtable Type : 00 [Generic Communications Subspace]
Length : 3E
Reserved : 000000000000
Base Address : 00000000DCE43018
Address Length : 0000000000001000
Doorbell Register : [Generic Address Structure]
Space ID : 01 [SystemIO]
Bit Width : 08
Bit Offset : 00
Encoded Access Width : 01 [Byte Access:8]
Address : 0000000000001842
Preserve Mask : 00000000000000FD
Write Mask : 0000000000000002
Command Latency : 00001388
Maximum Access Rate : 00000000
Minimum Turnaround Time : 0000
To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents. This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.
Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.
Signed-off-by: Al Stone <[email protected]>
Cc: Jassi Brar <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Len Brown <[email protected]>
---
drivers/mailbox/pcc.c | 81 ++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 43 deletions(-)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..fc3c237daef2 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -373,33 +373,24 @@ static const struct mbox_chan_ops pcc_chan_ops = {
};
/**
- * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
- * entries. There should be one entry per PCC client.
+ * parse_pcc_subspaces -- Count PCC subspaces defined
* @header: Pointer to the ACPI subtable header under the PCCT.
* @end: End of subtable entry.
*
- * Return: 0 for Success, else errno.
+ * Return: If we find a PCC subspace entry of a valid type, return 0.
+ * Otherwise, return -EINVAL.
*
* This gets called for each entry in the PCC table.
*/
static int parse_pcc_subspace(struct acpi_subtable_header *header,
const unsigned long end)
{
- struct acpi_pcct_hw_reduced *pcct_ss;
-
- if (pcc_mbox_ctrl.num_chans <= MAX_PCC_SUBSPACES) {
- pcct_ss = (struct acpi_pcct_hw_reduced *) header;
+ struct acpi_pcct_subspace *ss = (struct acpi_pcct_subspace *) header;
- if ((pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
- && (pcct_ss->header.type !=
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
- pr_err("Incorrect PCC Subspace type detected\n");
- return -EINVAL;
- }
- }
+ if (ss->header.type < ACPI_PCCT_TYPE_RESERVED)
+ return 0;
- return 0;
+ return -EINVAL;
}
/**
@@ -449,8 +440,8 @@ static int __init acpi_pcc_probe(void)
struct acpi_table_header *pcct_tbl;
struct acpi_subtable_header *pcct_entry;
struct acpi_table_pcct *acpi_pcct_tbl;
+ struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
int count, i, rc;
- int sum = 0;
acpi_status status = AE_OK;
/* Search for PCCT */
@@ -459,43 +450,41 @@ static int __init acpi_pcc_probe(void)
if (ACPI_FAILURE(status) || !pcct_tbl)
return -ENODEV;
- count = acpi_table_parse_entries(ACPI_SIG_PCCT,
- sizeof(struct acpi_table_pcct),
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
- parse_pcc_subspace, MAX_PCC_SUBSPACES);
- sum += (count > 0) ? count : 0;
-
- count = acpi_table_parse_entries(ACPI_SIG_PCCT,
- sizeof(struct acpi_table_pcct),
- ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
- parse_pcc_subspace, MAX_PCC_SUBSPACES);
- sum += (count > 0) ? count : 0;
+ /* Set up the subtable handlers */
+ for (i = ACPI_PCCT_TYPE_GENERIC_SUBSPACE;
+ i < ACPI_PCCT_TYPE_RESERVED; i++) {
+ proc[i].id = i;
+ proc[i].count = 0;
+ proc[i].handler = parse_pcc_subspace;
+ }
- if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
- pr_err("Error parsing PCC subspaces from PCCT\n");
+ count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
+ sizeof(struct acpi_table_pcct), proc,
+ ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
+ if (count == 0 || count > MAX_PCC_SUBSPACES) {
+ pr_warn("Invalid PCCT: %d PCC subspaces\n", count);
return -EINVAL;
}
- pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
- sum, GFP_KERNEL);
+ pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) * count, GFP_KERNEL);
if (!pcc_mbox_channels) {
pr_err("Could not allocate space for PCC mbox channels\n");
return -ENOMEM;
}
- pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+ pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
if (!pcc_doorbell_vaddr) {
rc = -ENOMEM;
goto err_free_mbox;
}
- pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+ pcc_doorbell_ack_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
if (!pcc_doorbell_ack_vaddr) {
rc = -ENOMEM;
goto err_free_db_vaddr;
}
- pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
+ pcc_doorbell_irq = kcalloc(count, sizeof(int), GFP_KERNEL);
if (!pcc_doorbell_irq) {
rc = -ENOMEM;
goto err_free_db_ack_vaddr;
@@ -509,18 +498,24 @@ static int __init acpi_pcc_probe(void)
if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
pcc_mbox_ctrl.txdone_irq = true;
- for (i = 0; i < sum; i++) {
+ for (i = 0; i < count; i++) {
struct acpi_generic_address *db_reg;
- struct acpi_pcct_hw_reduced *pcct_ss;
+ struct acpi_pcct_subspace *pcct_ss;
pcc_mbox_channels[i].con_priv = pcct_entry;
- pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
+ if (pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
+ pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+ struct acpi_pcct_hw_reduced *pcct_hrss;
+
+ pcct_hrss = (struct acpi_pcct_hw_reduced *) pcct_entry;
- if (pcc_mbox_ctrl.txdone_irq) {
- rc = pcc_parse_subspace_irq(i, pcct_ss);
- if (rc < 0)
- goto err;
+ if (pcc_mbox_ctrl.txdone_irq) {
+ rc = pcc_parse_subspace_irq(i, pcct_hrss);
+ if (rc < 0)
+ goto err;
+ }
}
+ pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
/* If doorbell is in system memory cache the virt address */
db_reg = &pcct_ss->doorbell_register;
@@ -531,7 +526,7 @@ static int __init acpi_pcc_probe(void)
((unsigned long) pcct_entry + pcct_entry->length);
}
- pcc_mbox_ctrl.num_chans = sum;
+ pcc_mbox_ctrl.num_chans = count;
pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
--
2.17.0
On 05/15/2018 02:00 AM, Rafael J. Wysocki wrote:
> On Tue, May 15, 2018 at 12:49 AM, Al Stone <[email protected]> wrote:
>> On 05/14/2018 03:04 PM, Prakash, Prashanth wrote:
>>>
>>>
>>> On 4/30/2018 6:39 PM, Al Stone wrote:
>>>> There have been multiple reports of the following error message:
>>>>
>>>> [ 0.068293] Error parsing PCC subspaces from PCCT
>>>>
>>>> This error message is not correct. In multiple cases examined, the PCCT
>>>> (Platform Communications Channel Table) concerned is actually properly
>>>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
>>>> is making the assumption that the only valid PCCT is one that contains
>>>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
>>>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
>>>> types are counted and as long as there is at least one of the desired
>>>> types, the acpi_pcc_probe() succeeds. When no subtables of these types
>>>> are found, regardless of whether or not any other subtable types are
>>>> present, the error mentioned above is reported.
>>>>
>>>> In the cases reported to me personally, the PCCT contains exactly one
>>>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
>>>> acpi_pcc_probe() does not count it as a valid subtable, so believes
>>>> there to be no valid subtables, and hence outputs the error message.
>>>>
>>>> An example of the PCCT being reported as erroneous yet perfectly fine
>>>> is the following:
>>>>
>>>> Signature : "PCCT"
>>>> Table Length : 0000006E
>>>> Revision : 05
>>>> Checksum : A9
>>>> Oem ID : "XXXXXX"
>>>> Oem Table ID : "XXXXX "
>>>> Oem Revision : 00002280
>>>> Asl Compiler ID : "XXXX"
>>>> Asl Compiler Revision : 00000002
>>>>
>>>> Flags (decoded below) : 00000001
>>>> Platform : 1
>>>> Reserved : 0000000000000000
>>>>
>>>> Subtable Type : 00 [Generic Communications Subspace]
>>>> Length : 3E
>>>>
>>>> Reserved : 000000000000
>>>> Base Address : 00000000DCE43018
>>>> Address Length : 0000000000001000
>>>>
>>>> Doorbell Register : [Generic Address Structure]
>>>> Space ID : 01 [SystemIO]
>>>> Bit Width : 08
>>>> Bit Offset : 00
>>>> Encoded Access Width : 01 [Byte Access:8]
>>>> Address : 0000000000001842
>>>>
>>>> Preserve Mask : 00000000000000FD
>>>> Write Mask : 0000000000000002
>>>> Command Latency : 00001388
>>>> Maximum Access Rate : 00000000
>>>> Minimum Turnaround Time : 0000
>>>>
>>>> To fix this, we count up all of the possible subtable types for the
>>>> PCCT, and only report an error when there are none (which could mean
>>>> either no subtables, or no valid subtables), or there are too many.
>>>> We also change the logic so that if there is a valid subtable, we
>>>> do try to initialize it per the PCCT subtable contents. This is a
>>>> change in functionality; previously, the probe would have returned
>>>> right after the error message and would not have tried to use any
>>>> other subtable definition.
>>>>
>>>> Tested on my personal laptop which showed the error previously; the
>>>> error message no longer appears and the laptop appears to operate
>>>> normally.
>>>>
>>>> Signed-off-by: Al Stone <[email protected]>
>>>> Cc: Jassi Brar <[email protected]>
>>>> Cc: Rafael J. Wysocki <[email protected]>
>>>> Cc: Len Brown <[email protected]>
>>>> ---
>>>> drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------
>>>> 1 file changed, 63 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 3ef7f036ceea..72af37d7e95e 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -372,13 +372,37 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>>>> .send_data = pcc_send_data,
>>>> };
>>>>
>>>> +/*
>>>> + *
>>>> + * count_pcc_subspaces -- Count PCC subspaces not used in reduced HW systems.
>>>> + * @header: Pointer to the ACPI subtable header under the PCCT.
>>>> + * @end: End of subtable entry.
>>>> + *
>>>> + * Return: If we find a PCC subspace entry that is one of the types used
>>>> + * in reduced hardware systems, return -EINVAL. Otherwise, return 0.
>>>> + *
>>>> + * This gets called for each subtable in the PCC table.
>>>> + */
>>>> +static int count_pcc_subspaces(struct acpi_subtable_header *header,
>>>> + const unsigned long end)
>>>> +{
>>>> + struct acpi_pcct_subspace *pcct_ss = (struct acpi_pcct_subspace *) header;
>>>> +
>>>> + if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>>>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>>>> + (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
>>>> + return 0;
>>>> +
>>>> + return -EINVAL;
>>>> +}
>>>> +
>>>> /**
>>>> - * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
>>>> - * entries. There should be one entry per PCC client.
>>>> + * parse_pcc_subspaces -- Count PCC subspaces used only in reduced HW systems.
>>>> * @header: Pointer to the ACPI subtable header under the PCCT.
>>>> * @end: End of subtable entry.
>>>> *
>>>> - * Return: 0 for Success, else errno.
>>>> + * Return: If we find a PCC subspace entry that is one of the types used
>>>> + * in reduced hardware systems, return 0. Otherwise, return -EINVAL.
>>>> *
>>>> * This gets called for each entry in the PCC table.
>>>> */
>>>> @@ -393,10 +417,8 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
>>>> if ((pcct_ss->header.type !=
>>>> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE)
>>>> && (pcct_ss->header.type !=
>>>> - ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
>>>> - pr_err("Incorrect PCC Subspace type detected\n");
>>>> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2))
>>>> return -EINVAL;
>>>> - }
>>>> }
>>>>
>>>> return 0;
>>> Can't we combine parse_pcc_subspace and count_pcc_subspaces into a
>>> single function? parse_pcc_subspace can return 0 for supported subspace
>>> types and -EINVAL for others.
>>
>> I did think about that. The issue is that we have subspaces that are only
>> valid in reduced hardware systems, and subspaces that are not. It might make
>> sense to use different names, as in 'count_reduced_hw_subspaces()' and
>> 'count_general_subspaces()' (or something like those) but we do have the two
>> separate classes and hardware belonging to each of those classes.
>>
>> That being said, you raise a good point: this would only be useful if the
>> mailbox code needed to know the classes of subspaces were different; I saw
>> no such code but I could have missed it. If you're aware of any such cases,
>> let me know. Otherwise, I'll combine the two counting routines and test it.
>>
>>> The limitation on number of subspaces(max = 256) applies to all types of PCC
>>> subspaces (see Table 14-351 in ACPI 6.2). The MAX_PCC_SUBSPACES check in
>>> parse_pcc_subspace seems invalid as pcc_mbox_ctrl.num_chans will not be
>>> initialized yet at that moment.
>>
>> Good catch. Thanks. That test was there prior to my patches, but I'll pull
>> it out.
>>
>>> Given above, I think it is probably better to update parse_pcc_subspace to
>>> only check for a valid PCC subspace type. The check to make sure overall count
>>> of subspace is less than 256 is already present following the call to
>>> acpi_table_parse_entries_array().
>>>
>>> --
>>> Thanks,
>>> Prashanth
>>>
>>
>> Thanks, Prashanth.
>>
>> Rafael: do you want me to just re-send this patch or the whole series? Either
>> way works for me; what's easiest for you since the first two have been applied?
>
> Just this patch, please.
>
> I've applied the other two already.
>
> Thanks,
> Rafael
>
An FYI: I tested v4 of this patch with and without patch 2/3 of this
set (which has already been withdrawn); both cases removed the mostly
incorrect error message.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On Thu, May 17, 2018 at 12:01 AM, Al Stone <[email protected]> wrote:
> There have been multiple reports of the following error message:
>
> [ 0.068293] Error parsing PCC subspaces from PCCT
>
> This error message is not correct. In multiple cases examined, the PCCT
> (Platform Communications Channel Table) concerned is actually properly
> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
> is making the assumption that the only valid PCCT is one that contains
> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
> types are counted and as long as there is at least one of the desired
> types, the acpi_pcc_probe() succeeds. When no subtables of these types
> are found, regardless of whether or not any other subtable types are
> present, the error mentioned above is reported.
>
> In the cases reported to me personally, the PCCT contains exactly one
> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
> acpi_pcc_probe() does not count it as a valid subtable, so believes
> there to be no valid subtables, and hence outputs the error message.
>
> An example of the PCCT being reported as erroneous yet perfectly fine
> is the following:
>
> Signature : "PCCT"
> Table Length : 0000006E
> Revision : 05
> Checksum : A9
> Oem ID : "XXXXXX"
> Oem Table ID : "XXXXX "
> Oem Revision : 00002280
> Asl Compiler ID : "XXXX"
> Asl Compiler Revision : 00000002
>
> Flags (decoded below) : 00000001
> Platform : 1
> Reserved : 0000000000000000
>
> Subtable Type : 00 [Generic Communications Subspace]
> Length : 3E
>
> Reserved : 000000000000
> Base Address : 00000000DCE43018
> Address Length : 0000000000001000
>
> Doorbell Register : [Generic Address Structure]
> Space ID : 01 [SystemIO]
> Bit Width : 08
> Bit Offset : 00
> Encoded Access Width : 01 [Byte Access:8]
> Address : 0000000000001842
>
> Preserve Mask : 00000000000000FD
> Write Mask : 0000000000000002
> Command Latency : 00001388
> Maximum Access Rate : 00000000
> Minimum Turnaround Time : 0000
>
> To fix this, we count up all of the possible subtable types for the
> PCCT, and only report an error when there are none (which could mean
> either no subtables, or no valid subtables), or there are too many.
> We also change the logic so that if there is a valid subtable, we
> do try to initialize it per the PCCT subtable contents. This is a
> change in functionality; previously, the probe would have returned
> right after the error message and would not have tried to use any
> other subtable definition.
>
> Tested on my personal laptop which showed the error previously; the
> error message no longer appears and the laptop appears to operate
> normally.
I'd like to know the Prashanth's opinion here.
On 5/17/2018 4:24 AM, Rafael J. Wysocki wrote:
> On Thu, May 17, 2018 at 12:01 AM, Al Stone <[email protected]> wrote:
>> There have been multiple reports of the following error message:
>>
>> [ 0.068293] Error parsing PCC subspaces from PCCT
>>
>> This error message is not correct. In multiple cases examined, the PCCT
>> (Platform Communications Channel Table) concerned is actually properly
>> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
>> is making the assumption that the only valid PCCT is one that contains
>> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
>> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
>> types are counted and as long as there is at least one of the desired
>> types, the acpi_pcc_probe() succeeds. When no subtables of these types
>> are found, regardless of whether or not any other subtable types are
>> present, the error mentioned above is reported.
>>
>> In the cases reported to me personally, the PCCT contains exactly one
>> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
>> acpi_pcc_probe() does not count it as a valid subtable, so believes
>> there to be no valid subtables, and hence outputs the error message.
>>
>> An example of the PCCT being reported as erroneous yet perfectly fine
>> is the following:
>>
>> Signature : "PCCT"
>> Table Length : 0000006E
>> Revision : 05
>> Checksum : A9
>> Oem ID : "XXXXXX"
>> Oem Table ID : "XXXXX "
>> Oem Revision : 00002280
>> Asl Compiler ID : "XXXX"
>> Asl Compiler Revision : 00000002
>>
>> Flags (decoded below) : 00000001
>> Platform : 1
>> Reserved : 0000000000000000
>>
>> Subtable Type : 00 [Generic Communications Subspace]
>> Length : 3E
>>
>> Reserved : 000000000000
>> Base Address : 00000000DCE43018
>> Address Length : 0000000000001000
>>
>> Doorbell Register : [Generic Address Structure]
>> Space ID : 01 [SystemIO]
>> Bit Width : 08
>> Bit Offset : 00
>> Encoded Access Width : 01 [Byte Access:8]
>> Address : 0000000000001842
>>
>> Preserve Mask : 00000000000000FD
>> Write Mask : 0000000000000002
>> Command Latency : 00001388
>> Maximum Access Rate : 00000000
>> Minimum Turnaround Time : 0000
>>
>> To fix this, we count up all of the possible subtable types for the
>> PCCT, and only report an error when there are none (which could mean
>> either no subtables, or no valid subtables), or there are too many.
>> We also change the logic so that if there is a valid subtable, we
>> do try to initialize it per the PCCT subtable contents. This is a
>> change in functionality; previously, the probe would have returned
>> right after the error message and would not have tried to use any
>> other subtable definition.
>>
>> Tested on my personal laptop which showed the error previously; the
>> error message no longer appears and the laptop appears to operate
>> normally.
> I'd like to know the Prashanth's opinion here.
Looks good.
Reviewed-by: Prashanth Prakash <[email protected]>
--
Thanks,
Prashanth
On Thursday, May 17, 2018 9:48:39 PM CEST Prakash, Prashanth wrote:
>
> On 5/17/2018 4:24 AM, Rafael J. Wysocki wrote:
> > On Thu, May 17, 2018 at 12:01 AM, Al Stone <[email protected]> wrote:
> >> There have been multiple reports of the following error message:
> >>
> >> [ 0.068293] Error parsing PCC subspaces from PCCT
> >>
> >> This error message is not correct. In multiple cases examined, the PCCT
> >> (Platform Communications Channel Table) concerned is actually properly
> >> constructed; the problem is that acpi_pcc_probe() which reads the PCCT
> >> is making the assumption that the only valid PCCT is one that contains
> >> subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
> >> ACPI_PCCT_TYPE_HW_REDUCED_TYPE2. The number of subtables of these
> >> types are counted and as long as there is at least one of the desired
> >> types, the acpi_pcc_probe() succeeds. When no subtables of these types
> >> are found, regardless of whether or not any other subtable types are
> >> present, the error mentioned above is reported.
> >>
> >> In the cases reported to me personally, the PCCT contains exactly one
> >> subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE. The function
> >> acpi_pcc_probe() does not count it as a valid subtable, so believes
> >> there to be no valid subtables, and hence outputs the error message.
> >>
> >> An example of the PCCT being reported as erroneous yet perfectly fine
> >> is the following:
> >>
> >> Signature : "PCCT"
> >> Table Length : 0000006E
> >> Revision : 05
> >> Checksum : A9
> >> Oem ID : "XXXXXX"
> >> Oem Table ID : "XXXXX "
> >> Oem Revision : 00002280
> >> Asl Compiler ID : "XXXX"
> >> Asl Compiler Revision : 00000002
> >>
> >> Flags (decoded below) : 00000001
> >> Platform : 1
> >> Reserved : 0000000000000000
> >>
> >> Subtable Type : 00 [Generic Communications Subspace]
> >> Length : 3E
> >>
> >> Reserved : 000000000000
> >> Base Address : 00000000DCE43018
> >> Address Length : 0000000000001000
> >>
> >> Doorbell Register : [Generic Address Structure]
> >> Space ID : 01 [SystemIO]
> >> Bit Width : 08
> >> Bit Offset : 00
> >> Encoded Access Width : 01 [Byte Access:8]
> >> Address : 0000000000001842
> >>
> >> Preserve Mask : 00000000000000FD
> >> Write Mask : 0000000000000002
> >> Command Latency : 00001388
> >> Maximum Access Rate : 00000000
> >> Minimum Turnaround Time : 0000
> >>
> >> To fix this, we count up all of the possible subtable types for the
> >> PCCT, and only report an error when there are none (which could mean
> >> either no subtables, or no valid subtables), or there are too many.
> >> We also change the logic so that if there is a valid subtable, we
> >> do try to initialize it per the PCCT subtable contents. This is a
> >> change in functionality; previously, the probe would have returned
> >> right after the error message and would not have tried to use any
> >> other subtable definition.
> >>
> >> Tested on my personal laptop which showed the error previously; the
> >> error message no longer appears and the laptop appears to operate
> >> normally.
> > I'd like to know the Prashanth's opinion here.
>
> Looks good.
>
> Reviewed-by: Prashanth Prakash <[email protected]>
Applied, thanks!