Message ID | 20180712023938.19398-1-dann.frazier@canonical.com |
---|---|
State | New |
Headers | show |
Series | [SRU,Bionic] mailbox: PCC: erroneous error message when parsing ACPI PCCT | expand |
On Wed, Jul 11, 2018 at 08:39:38PM -0600, dann frazier wrote: > From: Al Stone <ahs3@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1528684 > > 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> > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > (cherry picked from commit 8f8027c5f935bf02bdc8806c109ddbb0e402283c) > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > 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.18.0 Simple cherry-pick, tested by the OR. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
On 12.07.2018 04:39, dann frazier wrote: > From: Al Stone <ahs3@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1528684 > > 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> > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > (cherry picked from commit 8f8027c5f935bf02bdc8806c109ddbb0e402283c) > Signed-off-by: dann frazier <dann.frazier@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Clean cherry pick and successful testing. > 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); > >
Applied to B On 2018-07-11 20:39:38 , dann frazier wrote: > From: Al Stone <ahs3@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1528684 > > 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> > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > (cherry picked from commit 8f8027c5f935bf02bdc8806c109ddbb0e402283c) > Signed-off-by: dann frazier <dann.frazier@canonical.com> > --- > 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.18.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Wed, Jul 11, 2018 at 08:39:38PM -0600, dann frazier wrote: > From: Al Stone <ahs3@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1528684 > > 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> > Reviewed-by: Prashanth Prakash <pprakash@codeaurora.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > (cherry picked from commit 8f8027c5f935bf02bdc8806c109ddbb0e402283c) > Signed-off-by: dann frazier <dann.frazier@canonical.com> Applied to cosmic/master-next, thanks!
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);