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

[rric@kernel.org (Robert Richter)]

On 29.06.16 15:56:50, Ard Biesheuvel wrote:
> On 29 June 2016 at 15:34, Christopher Covington <cov@codeaurora.org> 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@codeaurora.org> 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);

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().

Plus one spelling fix: mcfg_qurks -> mcfg_quirks 

Thanks,

-Robert

> 
> > Given the above tuples, won't accidentally match:
> > (guessing at possible future ids)
> > {"APM   ", "XGENEi  ", 1}
> > {"CAVIUM", "THUNDERX", i} i != 1
> > {"CAVIUM", "THUNDERi", 1}
> > {"CAVIUM", "THUNDRXi", 1}
> > {"HISI  ", "HISI-D0i", 1} i != 2 && i != 3
> > {"QCOM  ", "QDF24ij ", 1} i != 3 && j != 2
> >
> > References for APM, HiSilicon IDs:
> > https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007108.html
> > https://lists.linaro.org/pipermail/linaro-acpi/2016-June/007043.html
> >
> > Thanks,
> > Cov
> >
> > --
> > Qualcomm Innovation Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project