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

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