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

[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 };

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.

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