| Message ID | 20180501003907.4322-4-ahs3@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tue, May 1, 2018 at 2:39 AM, Al Stone <ahs3@redhat.com> 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 <ahs3@redhat.com> > Cc: Jassi Brar <jassisinghbrar@gmail.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
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 <ahs3@redhat.com> > Cc: Jassi Brar <jassisinghbrar@gmail.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > --- > 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 -- 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
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 <ahs3@redhat.com> >> Cc: Jassi Brar <jassisinghbrar@gmail.com> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Len Brown <lenb@kernel.org> >> --- >> 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?
On Tue, May 15, 2018 at 12:49 AM, Al Stone <ahs3@redhat.com> 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 <ahs3@redhat.com> >>> Cc: Jassi Brar <jassisinghbrar@gmail.com> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Len Brown <lenb@kernel.org> >>> --- >>> 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 -- 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
On 05/15/2018 02:00 AM, Rafael J. Wysocki wrote: > On Tue, May 15, 2018 at 12:49 AM, Al Stone <ahs3@redhat.com> 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 <ahs3@redhat.com> >>>> Cc: Jassi Brar <jassisinghbrar@gmail.com> >>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>> Cc: Len Brown <lenb@kernel.org> >>>> --- >>>> 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.
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);
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 <ahs3@redhat.com> Cc: Jassi Brar <jassisinghbrar@gmail.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> --- drivers/mailbox/pcc.c | 96 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 33 deletions(-)