From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Nowicki Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks Date: Tue, 14 Jun 2016 13:52:17 +0200 Message-ID: <575FEFF1.9020105@semihalf.com> References: <1465822923-33599-1-git-send-email-liudongdong3@huawei.com> <1465822923-33599-2-git-send-email-liudongdong3@huawei.com> <575ED57F.6050503@codeaurora.org> <575F9B4D.8080501@huawei.com> <575FD253.3070508@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-wm0-f52.google.com ([74.125.82.52]:38837 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751194AbcFNLwZ (ORCPT ); Tue, 14 Jun 2016 07:52:25 -0400 Received: by mail-wm0-f52.google.com with SMTP id m124so119305048wme.1 for ; Tue, 14 Jun 2016 04:52:24 -0700 (PDT) In-Reply-To: <575FD253.3070508@huawei.com> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Dongdong Liu , Duc Dang Cc: Christopher Covington , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , Rafael Wysocki , Hanjun Guo , Lorenzo Pieralisi , okaya@codeaurora.org, Jayachandran C , robert.richter@caviumnetworks.com, Marcin Wojtas , Liviu Dudau , David Daney , Yijing Wang , Suravee Suthikulpanit , Mark Salter , linux-pci@vger.kernel.org, linux-arm , linux-acpi@vger.kernel.org, Linux Kernel Mailing List , linaro-acpi@lists.linaro.org, Jon Masters On 14.06.2016 11:45, Dongdong Liu wrote: > Hi Duc > > =E5=9C=A8 2016/6/14 17:00, Duc Dang =E5=86=99=E9=81=93: >> On Mon, Jun 13, 2016 at 10:51 PM, Dongdong Liu >> wrote: >>> Hi Duc >>> >>> =E5=9C=A8 2016/6/14 4:57, Duc Dang =E5=86=99=E9=81=93: >>>> >>>> On Mon, Jun 13, 2016 at 8:47 AM, Christopher Covington >>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> + >>>>>> +/* 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 *roo= t) >>>>>> +{ >>>>>> + int bus_num =3D root->secondary.start; >>>>>> + int domain =3D 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 then use >>>>>> OEM ID >>>>>> and >>>>>> + * OEM revision from MCFG table standard header. >>>>>> + */ >>>>>> + for (f =3D __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_f= ixups; >>>>>> f++) { >>>>>> + if ((f->domain =3D=3D domain || f->domain =3D=3D >>>>>> PCI_MCFG_DOMAIN_ANY) && >>>>>> + (f->bus_num =3D=3D bus_num || f->bus_num =3D=3D >>>>>> 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 coun= t >>>>> 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 resourc= e >>>>>> *bus_res); >>>>>> +extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_ro= ot >>>>>> *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 blin= dly >>>>> following a GCC documentation suggestion [1]--but removing the >>>>> first two >>>>> preprocessor concatenation operators "##" solved the following bu= ild >>>>> 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 *))))) >>>> =3D \ >>>> { 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 macr= o >>> '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 macr= o >>> '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 *))))) >> =3D \ > >> { ops, #oem_id, #oem_table_id, rev, dom, bus }; > > > This should change to { ops, oem_id, oem_table_id, rev, dom, bus }; > =E2=80=98#=E2=80=99 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, bu= s)\ static const struct pci_cfg_fixup \ __mcfg_fixup_##oem_id##oem_table_id##dom##bus \ __used __attribute__((__section__(".acpi_fixup_mcfg"), \ aligned((sizeof(void *))))) =3D \ { 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,=20 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= =20 string w/o quotation marks: #define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bu= s)\ static const struct pci_cfg_fixup \ __mcfg_fixup_##oem_id##oem_table_id##dom##bus \ __used __attribute__((__section__(".acpi_fixup_mcfg"), \ aligned((sizeof(void *))))) =3D \ { ops, #oem_id, #oem_table_id, rev, dom, bus }; DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, XXXXXX, YYYYYYYY, 1,=20 PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY); Personally I think that (2) is better, no need for macro definitions. Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751719AbcFNLw1 (ORCPT ); Tue, 14 Jun 2016 07:52:27 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:37687 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751150AbcFNLwY (ORCPT ); Tue, 14 Jun 2016 07:52:24 -0400 Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks To: Dongdong Liu , Duc Dang References: <1465822923-33599-1-git-send-email-liudongdong3@huawei.com> <1465822923-33599-2-git-send-email-liudongdong3@huawei.com> <575ED57F.6050503@codeaurora.org> <575F9B4D.8080501@huawei.com> <575FD253.3070508@huawei.com> Cc: Christopher Covington , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , Rafael Wysocki , Hanjun Guo , Lorenzo Pieralisi , okaya@codeaurora.org, Jayachandran C , robert.richter@caviumnetworks.com, Marcin Wojtas , Liviu Dudau , David Daney , Yijing Wang , Suravee Suthikulpanit , Mark Salter , linux-pci@vger.kernel.org, linux-arm , linux-acpi@vger.kernel.org, Linux Kernel Mailing List , linaro-acpi@lists.linaro.org, Jon Masters , Andrea Gallo , jeremy.linton@arm.com, Gabriele Paoloni , "Chenxin (Charles)" , Linuxarm , "liudong-1989@163.com" From: Tomasz Nowicki Message-ID: <575FEFF1.9020105@semihalf.com> Date: Tue, 14 Jun 2016 13:52:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <575FD253.3070508@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> wrote: >>> Hi Duc >>> >>> 在 2016/6/14 4:57, Duc Dang 写道: >>>> >>>> On Mon, Jun 13, 2016 at 8:47 AM, Christopher Covington >>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> + >>>>>> +/* 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 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); Personally I think that (2) is better, no need for macro definitions. Tomasz From mboxrd@z Thu Jan 1 00:00:00 1970 From: tn@semihalf.com (Tomasz Nowicki) Date: Tue, 14 Jun 2016 13:52:17 +0200 Subject: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks In-Reply-To: <575FD253.3070508@huawei.com> References: <1465822923-33599-1-git-send-email-liudongdong3@huawei.com> <1465822923-33599-2-git-send-email-liudongdong3@huawei.com> <575ED57F.6050503@codeaurora.org> <575F9B4D.8080501@huawei.com> <575FD253.3070508@huawei.com> Message-ID: <575FEFF1.9020105@semihalf.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> wrote: >>> Hi Duc >>> >>> ? 2016/6/14 4:57, Duc Dang ??: >>>> >>>> On Mon, Jun 13, 2016 at 8:47 AM, Christopher Covington >>>> 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 >>>>>> #include >>>>>> #include >>>>>> +#include >>>>>> + >>>>>> +/* 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 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); Personally I think that (2) is better, no need for macro definitions. Tomasz