On 22 July 2016 at 13:38, Robert Richter [off-list ref] wrote:

On 29.06.16 15:56:50, Ard Biesheuvel wrote:

quoted

On 29 June 2016 at 15:34, Christopher Covington [off-list ref] wrote:

quoted

Hi Tomasz,

On 06/29/2016 06:48 AM, Tomasz Nowicki wrote:

quoted

On 28.06.2016 18:12, Duc Dang wrote:

quoted

On Tue, Jun 28, 2016 at 6:04 AM, Christopher Covington
[off-list ref] wrote:

quoted

Hi Tomasz,

quoted

quoted

quoted

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".

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.