Message-ID: <8eb33906-070b-4fb0-1f1e-00c40507c457@redhat.com>
Date:   Wed, 16 May 2018 16:03:38 -0600
From:   Al Stone <ahs3@...hat.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Prakash, Prashanth" <pprakash@...eaurora.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>
Subject: Re: [PATCH v3 3/3] mailbox: ACPI: erroneous error message when
 parsing the ACPI PCCT

On 05/15/2018 02:00 AM, Rafael J. Wysocki wrote:
> On Tue, May 15, 2018 at 12:49 AM, Al Stone <ahs3@...hat.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@...hat.com>
>>>> Cc: Jassi Brar <jassisinghbrar@...il.com>
>>>> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
>>>> Cc: Len Brown <lenb@...nel.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.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@...hat.com
-----------------------------------

Powered by blists - more mailing lists