Re: [RFC PATCH v4 3/5] PCI: Check platform specific ECAM quirks

[Ard Biesheuvel]

On 22 July 2016 at 13:38, Robert Richter <rric@xxxxxxxxxx> wrote:
> On 29.06.16 15:56:50, Ard Biesheuvel wrote:
>> On 29 June 2016 at 15:34, Christopher Covington <cov@xxxxxxxxxxxxxx> wrote:
>> > Hi Tomasz,
>> >
>> > On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:
>> >> On 28.06.2016 18:12, Duc Dang wrote:
>> >>> On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
>> >>> <cov@xxxxxxxxxxxxxx> wrote:
>> >>>> Hi Tomasz,
>> >
>> >>>> Ard's comments on v3 included:
>> >>>>
>> >>>> "... exact OEM table/rev id matches ..."
>> >>>> "... substring match ... out of the question ..."
>> >
>> > Digging through the archives I see Jon Master commented earlier to "be
>> > careful with substring match".
>> >
>> >>> I think having OEM Table ID as "PLAT " and then "PLAT2 " (the the
>> >>> next version of the SoC) is common. So yes, matching full string is
>> >>> better as we can use "PLAT2 " in MCFG table and not worry about the
>> >>> "PLAT" sub-string match causes the quirk to be applied
>> >>> unintentionally.
>> >
>> >> Note that platforms already shipped where OEM string has no padding will
>> >
>> > I'm confused by this statement. OEMID is defined as 6 bytes long and OEM
>> > Table ID as 8 bytes long in the ACPI specification. As far as I can
>> > tell, if your string isn't exactly that long, padding up to that length
>> > is required.
>> >
>> >> have change the firmware or add 0 padding to our quirk array IDs.
>> >
>> > The fixed 6 or 8 character string compare, as used v2 of this patchset,
>> > will be compatible with existing firmware as best I can tell. Adding
>> > padding to the quirk array IDs is exactly what I'm suggesting, although
>> > all the strings I've seen are space padded rather than null padded.
>> >
>>
>> I don't think any interpretation of the 6 or 8 byte wide OEM fields is
>> necessary to be able to match it against a list of known values as
>> used by the quirky platforms. We need an exact match against whatever
>> we know is in the table of an affected system, and whether a space
>> qualifies as padding or as a character is irrelevant.
>>
>> > Matches:
>> > {"APM   ", "XGENE   ", 1}
>> > {"CAVIUM", "THUNDERX", 1}
>> > {"HISI  ", "HISI-D02", 1}
>> > {"HISI  ", "HISI-D03", 1}
>> > {"QCOM  ", "QDF2432 ", 1}
>> >
>>
>> I would not mind listing these as
>>
>> { { 'A','P','M',' ',' ',' ',' '}, {'X','G','E','N','E',' ',' ',' '}, 1}
>> ...
>>
>> just to stress that we are not dealing with C strings (and to avoid
>> having to deal with the implicit NUL terminator).
>> That also means memcmp() with a fixed length is the most appropriate
>> to perform the comparison
>
> I still would go with memcmp but have the char arrays null terminated
> in addition. This first makes string handling easier, and fixes some
> unterminated %s printfs bugs in the code.
>
> Thus, I would prefer to go with:
>
> struct pci_cfg_fixup {
>         char oem_id[ACPI_OEM_ID_SIZE + 1];
>         char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>         ...
>
> static struct pci_cfg_quirks mcfg_qurks[] __initconst = {
> /*      { OEM_ID, OEM_TABLE_ID, REV, DOMAIN, BUS_RANGE, pci_ops, init_hook }, */
> #ifdef CONFIG_PCI_HOST_THUNDER_PEM
>         /* Pass2.0 */
>         { "CAVIUM", "THUNDERX", 1, ...
>
> This is also no "pain in the eyes". :)
>
> If there are zero bytes in then just use \0, e.g.:
>
>  { "foo\0\0\0", "foobar\0\0", ... }
>
> For comparisation still use memcmp accordingly:
>
>  memcmp(..., ACPI_OEM_ID_SIZE);
>  memcmp(..., ACPI_OEM_TABLE_ID_SIZE);
>
> The following would be fixed too as strings are now null terminated:
>
>         pr_info("Handling %s %s r%d PCI MCFG quirks\n",
>                 f->oem_id, f->oem_table_id, f->oem_revision);
>

This looks like a clear improvement to me.

> Btw, use dev_info(&root->device->dev, ...) here for pr_info() and
> modify message text, e.g.:
>
>  acpi PNP0A08:04: Applying PCI MCFG quirks for CAVIUM THUNDERX rev: 1
>
> And, we should support some sort of MCFG_OEM_REVISION_ANY to move the
> rev handling optional to pci_cfg_fixup::init().
>

xxx_ANY implies 'wildcard', which we don't want in this code. The set
of quirky hardware we intend to support is known, and wildcard
matching makes it easier to circumvent our policy that from here on,
i.e., that all ACPI/arm64 supported hardware shall adhere to the spec.

-- 
Ard.