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

[Duc Dang <dhdang@apm.com>]

On Tue, Jun 14, 2016 at 4:52 AM, Tomasz Nowicki <tn@semihalf.com> wrote:
> On 14.06.2016 11:45, Dongdong Liu wrote:
>>
>> Hi Duc
>>
>> 在 2016/6/14 17:00, Duc Dang 写道:
>>>
>>> On Mon, Jun 13, 2016 at 10:51 PM, Dongdong Liu
>>> <liudongdong3@huawei.com> wrote:
>>>>
>>>> Hi Duc
>>>>
>>>> 在 2016/6/14 4:57, Duc Dang 写道:
>>>>>
>>>>>
>>>>> On Mon, Jun 13, 2016 at 8:47 AM, Christopher Covington
>>>>> <cov@codeaurora.org> 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.
>
>
> Both solutions are OK.
>
> 1. This works when we use macros as OEM ID and OEM table ID:
>
> #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##dom##bus           \
>          __used __attribute__((__section__(".acpi_fixup_mcfg"),         \
>                                 aligned((sizeof(void *))))) =           \
>         { ops, oem_id, oem_table_id, rev, dom, bus };
>
> #define OEM_ID "XXXXXX"
> #define OEM_TABLE_ID "YYYYYYYY"
>
> DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, OEM_ID, OEM_TABLE_ID, 1,
> PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
>
> 2. This works w/o macro which means we need to define OEM ID and OEM as
> string w/o quotation marks:
>
> #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##dom##bus \
> __used __attribute__((__section__(".acpi_fixup_mcfg"), \
> aligned((sizeof(void *))))) = \
> { ops, #oem_id, #oem_table_id, rev, dom, bus };
>
> DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, XXXXXX, YYYYYYYY, 1,
> PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);

In case of oem_id and oem_table_id have special characters ("HISI-D02"
as an example), #2 will have problem unless we use macro definitions
for them.

>
> Personally I think that (2) is better, no need for macro definitions.
> Tomasz
Regards,
Duc Dang