From mboxrd@z Thu Jan 1 00:00:00 1970 From: Duc Dang Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks Date: Tue, 14 Jun 2016 02:00:23 -0700 Message-ID: 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> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <575F9B4D.8080501@huawei.com> Sender: linux-pci-owner@vger.kernel.org To: Dongdong Liu Cc: Christopher Covington , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , Rafael Wysocki , Hanjun Guo , Lorenzo Pieralisi , okaya@codeaurora.org, Jayachandran C , Tomasz Nowicki , 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 List-Id: linux-acpi@vger.kernel.org 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 *root) >>>> +{ >>>> + 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 corresp= onding >>>> + * CAM ops. >>>> + * >>>> + * First match against PCI topology then use OE= M ID >>>> and >>>> + * OEM revision from MCFG table standard header. >>>> + */ >>>> + for (f =3D __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fix= ups; >>>> 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 count = used >>> here were >>> >>> min(strlen(f->oem_id), ACPI_OEM_ID_SIZE) >>> >>> then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be subst= rings >>> 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 handl= e); >>>> >>>> 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 pc= i_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, b= us) \ >>>> + 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 blindl= y >>> following a GCC documentation suggestion [1]--but removing the firs= t two >>> preprocessor concatenation operators "##" solved the following buil= d >>> 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 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_M= CFG_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, bu= s) \ 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 }; 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 =3D=3D domain || f->domain =3D=3D PCI_MCF= G_DOMAIN_ANY) && (f->bus_num =3D=3D bus_num || f->bus_num =3D=3D 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 =3D=3D 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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> 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 ""QDF24= 32"" >>> 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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> __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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> __mcfg_fixup_##oem_id##oem_table_id##dom##bus \ >>> ^ >>> arch/arm64/kernel/pci.c:225:44: error: expected =E2=80=98=3D=E2=80=99= , =E2=80=98,=E2=80=99, =E2=80=98;=E2=80=99, =E2=80=98asm=E2=80=99 or >>> =E2=80=98__attribute__=E2=80=99 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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> __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#Concatenat= ion >>> >>>> + __used __attribute__((__section__(".acpi_fixup_mcfg"), = \ >>>> + aligned((sizeof(void *))))) =3D = \ >>>> + { ops, oem_id, oem_table_id, dom, bus }; >>>> + >>>> extern int acpi_pci_probe_root_resources(struct acpi_pci_root_in= fo >>>> *info); >>>> extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root >>>> *root, >>>> struct acpi_pci_root_op= s >>>> *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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752074AbcFNJA6 (ORCPT ); Tue, 14 Jun 2016 05:00:58 -0400 Received: from mail-vk0-f42.google.com ([209.85.213.42]:34805 "EHLO mail-vk0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751702AbcFNJAx convert rfc822-to-8bit (ORCPT ); Tue, 14 Jun 2016 05:00:53 -0400 MIME-Version: 1.0 In-Reply-To: <575F9B4D.8080501@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> From: Duc Dang Date: Tue, 14 Jun 2016 02:00:23 -0700 Message-ID: Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks To: Dongdong Liu Cc: Christopher Covington , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , Rafael Wysocki , Hanjun Guo , Lorenzo Pieralisi , okaya@codeaurora.org, Jayachandran C , Tomasz Nowicki , 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 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 }; 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: MIME-Version: 1.0 In-Reply-To: <575F9B4D.8080501@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> From: Duc Dang Date: Tue, 14 Jun 2016 02:00:23 -0700 Message-ID: Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks To: Dongdong Liu Cc: Christopher Covington , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , Rafael Wysocki , Hanjun Guo , Lorenzo Pieralisi , okaya@codeaurora.org, Jayachandran C , Tomasz Nowicki , 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 Content-Type: text/plain; charset=UTF-8 List-ID: On Mon, Jun 13, 2016 at 10:51 PM, Dongdong Liu wr= ote: > 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 *root) >>>> +{ >>>> + 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 correspondi= ng >>>> + * 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_fixups; >>>> 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 count used >>> here were >>> >>> min(strlen(f->oem_id), ACPI_OEM_ID_SIZE) >>> >>> then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be substring= s >>> 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_de= v >>>> *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 tw= o >>> 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 *))))) =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##bu= s\ > ^ > 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##bu= s\ > ^ > 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 *))))) =3D \ { 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 =3D=3D domain || f->domain =3D=3D PCI_MCFG_DO= MAIN_ANY) && (f->bus_num =3D=3D bus_num || f->bus_num =3D=3D 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 =3D=3D 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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> 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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> __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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> __mcfg_fixup_##oem_id##oem_table_id##dom##bus \ >>> ^ >>> arch/arm64/kernel/pci.c:225:44: error: expected =E2=80=98=3D=E2=80=99, = =E2=80=98,=E2=80=99, =E2=80=98;=E2=80=99, =E2=80=98asm=E2=80=99 or >>> =E2=80=98__attribute__=E2=80=99 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 >>> =E2=80=98DECLARE_ACPI_MCFG_FIXUP=E2=80=99 >>> __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 *))))) =3D = \ >>>> + { 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: dhdang@apm.com (Duc Dang) Date: Tue, 05 Jul 2016 04:36:11 -0000 Subject: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks In-Reply-To: <575F9B4D.8080501@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> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 }; 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.