Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks

[Dongdong Liu]

Hi Duc

å 2016/6/14 17:00, Duc Dang åé:
> On Mon, Jun 13, 2016 at 10:51 PM, Dongdong Liu <liudongdong3@xxxxxxxxxx> wrote:
> > Hi Duc
> >
> > å 2016/6/14 4:57, Duc Dang åé:
> > > On Mon, Jun 13, 2016 at 8:47 AM, Christopher Covington
> > > <cov@xxxxxxxxxxxxxx> wrote:
> > > > Hi Dongdong,
> > > >
> > > > On 06/13/2016 09:02 AM, Dongdong Liu wrote:
> > > > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > > > > index d3c3e85..49612b3 100644
> > > > > --- a/drivers/acpi/pci_mcfg.c
> > > > > +++ b/drivers/acpi/pci_mcfg.c
> > > > > @@ -22,6 +22,10 @@
> > > > >    #include <linux/kernel.h>
> > > > >    #include <linux/pci.h>
> > > > >    #include <linux/pci-acpi.h>
> > > > > +#include <linux/pci-ecam.h>
> > > > > +
> > > > > +/* Root pointer to the mapped MCFG table */
> > > > > +static struct acpi_table_mcfg *mcfg_table;
> > > > >
> > > > >    /* Structure to hold entries from the MCFG table */
> > > > >    struct mcfg_entry {
> > > > > @@ -35,6 +39,38 @@ struct mcfg_entry {
> > > > >    /* List to save mcfg entries */
> > > > >    static LIST_HEAD(pci_mcfg_list);
> > > > >
> > > > > +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
> > > > > +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
> > > > > +
> > > > > +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> > > > > +{
> > > > > +     int bus_num = root->secondary.start;
> > > > > +     int domain = root->segment;
> > > > > +     struct pci_cfg_fixup *f;
> > > > > +
> > > > > +     if (!mcfg_table)
> > > > > +             return &pci_generic_ecam_ops;
> > > > > +
> > > > > +     /*
> > > > > +      * Match against platform specific quirks and return corresponding
> > > > > +      * CAM ops.
> > > > > +      *
> > > > > +      * First match against PCI topology <domain:bus> then use OEM ID
> > > > > and
> > > > > +      * OEM revision from MCFG table standard header.
> > > > > +      */
> > > > > +     for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups;
> > > > > f++) {
> > > > > +             if ((f->domain == domain || f->domain ==
> > > > > PCI_MCFG_DOMAIN_ANY) &&
> > > > > +                 (f->bus_num == bus_num || f->bus_num ==
> > > > > PCI_MCFG_BUS_ANY) &&
> > > > > +                 (!strncmp(f->oem_id, mcfg_table->header.oem_id,
> > > > > +                           ACPI_OEM_ID_SIZE)) &&
> > > > > +                 (!strncmp(f->oem_table_id,
> > > > > mcfg_table->header.oem_table_id,
> > > > > +                           ACPI_OEM_TABLE_ID_SIZE)))
> > > >
> > > >
> > > > This would just be a small convenience, but if the character count used
> > > > here were
> > > >
> > > > min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
> > > >
> > > > then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substrings
> > > > and
> > > > wouldn't need to be padded out to the full length.
> > > >
> > > > > +                     return f->ops;
> > > > > +     }
> > > > > +     /* No quirks, use ECAM */
> > > > > +     return &pci_generic_ecam_ops;
> > > > > +}
> > > >
> > > >
> > > > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > > > > index 7d63a66..088a1da 100644
> > > > > --- a/include/linux/pci-acpi.h
> > > > > +++ b/include/linux/pci-acpi.h
> > > > > @@ -25,6 +25,7 @@ static inline acpi_status
> > > > > pci_acpi_remove_pm_notifier(struct acpi_device *dev)
> > > > >    extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
> > > > >
> > > > >    extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource
> > > > > *bus_res);
> > > > > +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root
> > > > > *root);
> > > > >
> > > > >    static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev
> > > > > *pdev)
> > > > >    {
> > > > > @@ -72,6 +73,25 @@ struct acpi_pci_root_ops {
> > > > >         int (*prepare_resources)(struct acpi_pci_root_info *info);
> > > > >    };
> > > > >
> > > > > +struct pci_cfg_fixup {
> > > > > +     struct pci_ecam_ops *ops;
> > > > > +     char *oem_id;
> > > > > +     char *oem_table_id;
> > > > > +     int domain;
> > > > > +     int bus_num;
> > > > > +};
> > > > > +
> > > > > +#define PCI_MCFG_DOMAIN_ANY  -1
> > > > > +#define PCI_MCFG_BUS_ANY     -1
> > > > > +
> > > > > +/* Designate a routine to fix up buggy MCFG */
> > > > > +#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, dom, bus) \
> > > > > +     static const struct pci_cfg_fixup                               \
> > > > > +     __mcfg_fixup_##oem_id##oem_table_id##dom##bus                   \
> > > >
> > > >
> > > > I'm not entirely sure that this is the right fix--I'm pretty blindly
> > > > following a GCC documentation suggestion [1]--but removing the first two
> > > > preprocessor concatenation operators "##" solved the following build
> > > > error
> > > > for me.
> > > >
> > > > include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and
> > > > ""QCOM"" does not give a valid preprocessing token
> > > >     __mcfg_fixup_##oem_id##oem_table_id##dom##bus   \
> > >
> > >
> > > I think the problem is gcc is not happy with quoted string when
> > > processing these tokens
> > > (""QCOM"", the extra "" are added by gcc). So should we not concat
> > > string tokens and
> > > use the fixup definition in v1 of this RFC:
> > > /* Designate a routine to fix up buggy MCFG */
> > > #define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, rev, dom, bus)             \
> > >           static const struct pci_cfg_fixup
> > > __mcfg_fixup_##system##dom##bus\
> > >            __used __attribute__((__section__(".acpi_fixup_mcfg"),         \
> > >                                   aligned((sizeof(void *))))) =           \
> > >           { ops, oem_id, rev, dom, bus };
> >
> >
> > V1 fixup exist the redefinition error when compiling mutiple
> > DECLARE_ACPI_MCFG_FIXUP
> > with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
> >
> > #define EFI_ACPI_HISI_OEM_ID "HISI"
> > #define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
> > #define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
> >
> > DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
> >         EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
> > PCI_MCFG_BUS_ANY);
> >
> > DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
> >         EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
> > PCI_MCFG_BUS_ANY);
> >
> > In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
> > include/linux/pci-acpi.h:98:43: error: redefinition of
> > '__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
> >           static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
> >                                             ^
> > drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro
> > 'DECLARE_ACPI_MCFG_FIXUP'
> >   DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
> >   ^
> > include/linux/pci-acpi.h:98:43: note: previous definition of
> > '__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
> >           static const struct pci_cfg_fixup __mcfg_fixup_##system##dom##bus\
> >                                             ^
> > drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro
> > 'DECLARE_ACPI_MCFG_FIXUP'
> >
> >
> > V2 fixup can resolve the redefinition error, but need to use macro.
> > We can see that the name of macro is not replace with it's value in
> > "__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
> >
> > Any good idea is appreciated.
> Hmmm.
>
> I was testing # op and using min_t to get the min-len when doing
> strncmp similar to Chris' suggestion (using min_t avoids type
> warnings)
>
> /* Designate a routine to fix up buggy MCFG */
> #define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus) \
>          static const struct pci_cfg_fixup
> __mcfg_fixup##oem_id##oem_table_id##rev##dom##bus\
>           __used __attribute__((__section__(".acpi_fixup_mcfg"),         \
>                                  aligned((sizeof(void *))))) =           \

>          { ops, #oem_id, #oem_table_id, rev, dom, bus };


This should change to { ops, oem_id, oem_table_id, rev, dom, bus };
â#â is not need.

> My fixup definition:
> DECLARE_ACPI_MCFG_FIXUP(&xgene_pcie_ecam_ops, APM, XGENE, 2,
>                                                 PCI_MCFG_DOMAIN_ANY,
> PCI_MCFG_BUS_ANY);
> My match condition is:
>                 if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
>                     (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
>                     (!strncmp(f->oem_id, mcfg->header.oem_id,
>                               min_t(size_t, strlen(f->oem_id),
>                                     ACPI_OEM_ID_SIZE))) &&
>                     (!strncmp(f->oem_table_id, mcfg->header.oem_table_id,
>                               min_t(size_t, strlen(f->oem_table_id),
>                                     ACPI_OEM_TABLE_ID_SIZE))) &&
>                     (f->oem_revision == mcfg->header.oem_revision)) {
>                         return f->ops;
>                 }
>
> But this still does not work for your case as having HISI-D02 as
> oem_table_id will cause error.

In my case, I have tested  and it worked ok.
Could you show the detail error information that you met?

Thanks
Dongdong
> > Thanks
> > Dongdong
> >
> > > Regards,
> > > Duc Dang.
> > >
> > >
> > > >     ^
> > > > arch/arm64/kernel/pci.c:225:1: note: in expansion of macro
> > > > âDECLARE_ACPI_MCFG_FIXUPâ
> > > >    DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
> > > > PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
> > > >    ^
> > > > arch/arm64/kernel/pci.c:225:44: error: pasting ""QCOM"" and ""QDF2432""
> > > > does not give a valid preprocessing token
> > > >    DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
> > > > PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
> > > >                                               ^
> > > > include/linux/pci-acpi.h:90:17: note: in definition of macro
> > > > âDECLARE_ACPI_MCFG_FIXUPâ
> > > >     __mcfg_fixup_##oem_id##oem_table_id##dom##bus   \
> > > >                    ^
> > > > arch/arm64/kernel/pci.c:225:52: error: pasting ""QDF2432"" and
> > > > "PCI_MCFG_DOMAIN_ANY" does not give a valid preprocessi
> > > > ng token
> > > >    DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
> > > > PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
> > > >                                                       ^
> > > > include/linux/pci-acpi.h:90:25: note: in definition of macro
> > > > âDECLARE_ACPI_MCFG_FIXUPâ
> > > >     __mcfg_fixup_##oem_id##oem_table_id##dom##bus   \
> > > >                            ^
> > > > arch/arm64/kernel/pci.c:225:44: error: expected â=â, â,â, â;â, âasmâ or
> > > > â__attribute__â before string constant
> > > >    DECLARE_ACPI_MCFG_FIXUP(&pci_32b_ecam_ops, "QCOM", "QDF2432",
> > > > PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
> > > >                                               ^
> > > > include/linux/pci-acpi.h:90:17: note: in definition of macro
> > > > âDECLARE_ACPI_MCFG_FIXUPâ
> > > >     __mcfg_fixup_##oem_id##oem_table_id##dom##bus   \
> > > >                    ^
> > > > make[1]: *** [arch/arm64/kernel/pci.o] Error 1
> > > > make: *** [arch/arm64/kernel] Error 2
> > > >
> > > > 1. https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html#Concatenation
> > > >
> > > > > +     __used  __attribute__((__section__(".acpi_fixup_mcfg"),         \
> > > > > +                             aligned((sizeof(void *))))) =           \
> > > > > +     { ops, oem_id, oem_table_id, dom, bus };
> > > > > +
> > > > >    extern int acpi_pci_probe_root_resources(struct acpi_pci_root_info
> > > > > *info);
> > > > >    extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root
> > > > > *root,
> > > > >                                             struct acpi_pci_root_ops
> > > > > *ops,
> > > >
> > > > Thanks,
> > > > Cov
> > > >
> > > > --
> > > > Qualcomm Innovation Center, Inc.
> > > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > > > a Linux Foundation Collaborative Project
> > >
> > >
> > > .
> Regards
> Duc Dang.
>
> .