From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752171AbcFNJrb (ORCPT ); Tue, 14 Jun 2016 05:47:31 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:14369 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750703AbcFNJr1 (ORCPT ); Tue, 14 Jun 2016 05:47:27 -0400 Subject: Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks To: 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> CC: Christopher Covington , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , "Rafael Wysocki" , Hanjun Guo , "Lorenzo Pieralisi" , , Jayachandran C , Tomasz Nowicki , , Marcin Wojtas , "Liviu Dudau" , David Daney , "Yijing Wang" , Suravee Suthikulpanit , Mark Salter , , linux-arm , , "Linux Kernel Mailing List" , , Jon Masters , Andrea Gallo , , Gabriele Paoloni , "Chenxin (Charles)" , Linuxarm , "liudong-1989@163.com" From: Dongdong Liu Message-ID: <575FD253.3070508@huawei.com> Date: Tue, 14 Jun 2016 17:45:55 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.61.21.156] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.575FD265.00AE,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 3a3b8085b8cad909e73e6b1441cbb467 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > 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. In my case, I have tested and it worked ok. Could you show the detail error information that you met? Thanks Dongdong > >> >> 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. > > . >