Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752663AbdFNRn0 (ORCPT ); Wed, 14 Jun 2017 13:43:26 -0400 Received: from mail-pg0-f53.google.com ([74.125.83.53]:34766 "EHLO mail-pg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752327AbdFNRnZ (ORCPT ); Wed, 14 Jun 2017 13:43:25 -0400 MIME-Version: 1.0 In-Reply-To: <20170614125902.GA29762@red-moon> References: <1493340847-25720-1-git-send-email-kdinh@apm.com> <20170505165119.GA8614@red-moon> <20170608103956.GB8644@red-moon> <20170614125902.GA29762@red-moon> From: Khuong Dinh Date: Wed, 14 Jun 2017 10:43:23 -0700 Message-ID: Subject: Re: [PATCH v2 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1 To: Lorenzo Pieralisi Cc: Marc Zyngier , msalter@redhat.com, Bjorn Helgaas , linux-pci@vger.kernel.org, jcm@redhat.com, patches@apm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, rjw@rjwysocki.net Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v5EHj2M2013941 Content-Length: 12744 Lines: 296 On Wed, Jun 14, 2017 at 5:59 AM, Lorenzo Pieralisi wrote: > On Tue, Jun 13, 2017 at 01:56:44PM -0700, Khuong Dinh wrote: >> Hi Lorenzo, >> >> On Thu, Jun 8, 2017 at 3:39 AM, Lorenzo Pieralisi >> wrote: >> > On Tue, Jun 06, 2017 at 09:44:15AM -0700, Khuong Dinh wrote: >> >> Hi Lorenzo, >> >> >> >> On Tue, May 9, 2017 at 3:55 PM, Khuong Dinh wrote: >> >> > Hi Lorenzo, >> >> > >> >> > On Fri, May 5, 2017 at 9:51 AM, Lorenzo Pieralisi >> >> > wrote: >> >> >> On Thu, May 04, 2017 at 05:36:06PM -0700, Khuong Dinh wrote: >> >> >>> Hi Marc, >> >> >>> There's no explicit dependency between pcie driver and msi >> >> >>> controller. The current solution that we did is relying on the >> >> >>> node ordering in BIOS. ACPI 5.0 introduced _DEP method to assign a >> >> >>> higher priority in start ordering. This method could be applied in >> >> >>> case of msi and pcie are the same level of subsys_init (in ACPI >> >> >>> boot). However, PCIe driver has not supported for this dependency >> >> >>> check yet. How do you think about this solution. >> >> >> >> >> >> First off, you can't post emails with confidentiality notices on >> >> >> mailing lists so remove it from now onwards. >> >> > >> >> > Fixed >> >> > >> >> >> Secondly, I commented on this before, so you know what my opinion is. >> >> > >> >> > I got your opinion. I'll implement as your suggestion. >> >> > >> >> >> >> Regarding to your previous suggestion to add a hook walking the ACPI >> >> namespace before acpi_bus_scan() in acpi_scan_init() to make MSI >> >> controllers must be probed before PCI. I have a concern that the >> >> MSI namespace “ _SB.MSIX” is not a unique name and how can we walk >> >> the ACPI namespace and use this name to make MSI probed before PCI. >> >> May you have little bit more information for this or do you have >> >> another suggestion for this? >> >> >> >> There’s another solution which makes this simpler and I’d like to >> >> ask your opinion about this. >> >> The solution is to make an hierarchy between MSI and PCI nodes as below: >> >> Device(\_SB.MSIX) { >> >> Device(\_SB.PCI0) >> >> Device(\_SB.PCI1) >> >> …… >> >> } >> >> In other word, MSIX node is a parent node of PCI nodes in ACPI >> >> table. In this sense, there’s an explicit dependency between MSI >> >> and PCI, MSI controller must be probed before PCI and it will >> >> guarantee not breaking next kernel releases. How do you think about >> >> this solution. >> > >> > I think that's a plaster as ineffective as reordering nodes, in short >> > it is not a solution and you still rely on kernel link ordering, you >> > can fiddle with ACPI tables as much as you want but that does not change. >> > >> >> I also tried to use _DEP method to describe the dependency of PCIe >> >> nodes, but it looks that it does not work as PCI and MSI are handed >> >> by acpi_bus_scan and still having issue when we re-probe PCI. >> > >> > That's a tad vague to be frank, anyway it is pretty clear to me that we >> > have hit a wall. In ACPI there is no way to describe probe dependencies >> > like the one you have, it is as simple as that, and this MSI issue you >> > are facing is just an example, there are more eg: >> > >> > https://www.spinics.net/lists/arm-kernel/msg585825.html >> > >> > At the end of the day the choice is simple either we accept (and we >> > maintain because that's the problem) these hacks - and I am not willing >> > to do that - or we find a way to solve this from a general perspective not >> > as a point hack. >> > >> > I can have a look at solving the whole issue but it won't happen >> > tomorrow. >> >> Thanks for helping to resolve this general ACPI dependence issue. >> I look forward for a version to test with. >> >> Can we separate the general dependence patch from the X-Gene MSI ACPI >> enable patch. >> This original patch was to enable ACPI support for PCIe MSI. >> The dependence issue can be resolved separately. >> Can you help to pull in the patch. > > Ok, pragmatically the only sane thing you can do is: > > (1) Add an X-gene specific hook in drivers/acpi/scan.c (acpi_scan_init()) > that carries out fwnode registration (ie register the fwnode by > searching the MSI HID object - this does not depend on ACPI device > nodes order - you enforce the order by parsing the namespace before > ACPI core does it, at that point in time ACPI objects are created). > Not sure Rafael will be happy to see this code but that's the only > solution that does not rely on ACPI device nodes ordering and kernel > link ordering. You may achieve the same by extending the ACPI APD > code (drivers/acpi/acpi_apd.c) whether that's the best way forward is > to be seen. Hi Rafael, Do you have any idea about this suggestion? Otherwise, I'll follow this Lorenzo's approach (1). > (2) You can also add an xgene specific scan handler at arch_init_call() > but given that PCI root bridge is managed through a scan handler too you > would rely on ACPI devices nodes ordering in the DSDT. Let me explain > something for the benefit of everyone reading this thread: I do not want > X-gene ACPI tables to force the kernel to scan devices in any order > whatsover because this would mean we will _never_ be able to change the > scan order if we wanted to lest we trigger regressions. So this is > a non-option in my opinion. > > (3) Last option is to register the MSI fwnode either in PCI ECAM quirk > handling or in arm64 before probing the PCI root bus. > > I am not keen on (2) and (3) at all, so _if_ we have to find a solution > to this problem (1) is the only option you have for the time being as > far as I am concerned. Thank you very much for your suggestions, Lorenzo. If there's not any other idea, I'll follow your approach (1) Best regards, Khuong Dinh > Lorenzo > >> >> Best regards, >> Khuong Dinh >> >> > Thanks, >> > Lorenzo >> > >> >> Thanks, >> >> Khuong Dinh >> >> >> >> >> Finally, please execute this command on the vmlinux that "works" >> >> >> for you: >> >> >> >> >> >> nm vmlinux | grep -E '__initcall_(xgene_pcie_msi_init|acpi_init)' >> >> > >> >> > $ nm vmlinux | grep -E '__initcall_(xgene_pcie_msi_init|acpi_init)' >> >> > ffff000008dab2d8 t __initcall_acpi_init4 >> >> > ffff000008dab2c8 t __initcall_xgene_pcie_msi_init4 >> >> > >> >> > Best regards, >> >> > Khuong Dinh >> >> > >> >> >> Even by ordering devices in the ACPI tables (that I abhor) I still do >> >> >> not understand how this works (I mean without relying on kernel link >> >> >> order to ensure that the MSI driver is probed before PCI devices are >> >> >> enumerated in acpi_init()). >> >> >> >> >> >> Thanks, >> >> >> Lorenzo >> >> >> >> >> >>> Best regards, >> >> >>> Khuong >> >> >>> >> >> >>> On Fri, Apr 28, 2017 at 2:27 AM, Marc Zyngier wrote: >> >> >>> > On 28/04/17 01:54, Khuong Dinh wrote: >> >> >>> >> From: Khuong Dinh >> >> >>> >> >> >> >>> >> This patch makes pci-xgene-msi driver ACPI-aware and provides >> >> >>> >> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode. >> >> >>> >> >> >> >>> >> Signed-off-by: Khuong Dinh >> >> >>> >> Signed-off-by: Duc Dang >> >> >>> >> Acked-by: Marc Zyngier >> >> >>> >> --- >> >> >>> >> v2: >> >> >>> >> - Verify with BIOS version 3.06.25 and 3.07.09 >> >> >>> >> v1: >> >> >>> >> - Initial version >> >> >>> >> --- >> >> >>> >> drivers/pci/host/pci-xgene-msi.c | 35 ++++++++++++++++++++++++++++++++--- >> >> >>> >> 1 files changed, 32 insertions(+), 3 deletions(-) >> >> >>> >> >> >> >>> >> diff --git a/drivers/pci/host/pci-xgene-msi.c b/drivers/pci/host/pci-xgene-msi.c >> >> >>> >> index f1b633b..00aaa3d 100644 >> >> >>> >> --- a/drivers/pci/host/pci-xgene-msi.c >> >> >>> >> +++ b/drivers/pci/host/pci-xgene-msi.c >> >> >>> >> @@ -24,6 +24,7 @@ >> >> >>> >> #include >> >> >>> >> #include >> >> >>> >> #include >> >> >>> >> +#include >> >> >>> >> >> >> >>> >> #define MSI_IR0 0x000000 >> >> >>> >> #define MSI_INT0 0x800000 >> >> >>> >> @@ -39,7 +40,7 @@ struct xgene_msi_group { >> >> >>> >> }; >> >> >>> >> >> >> >>> >> struct xgene_msi { >> >> >>> >> - struct device_node *node; >> >> >>> >> + struct fwnode_handle *fwnode; >> >> >>> >> struct irq_domain *inner_domain; >> >> >>> >> struct irq_domain *msi_domain; >> >> >>> >> u64 msi_addr; >> >> >>> >> @@ -249,6 +250,13 @@ static void xgene_irq_domain_free(struct irq_domain *domain, >> >> >>> >> .free = xgene_irq_domain_free, >> >> >>> >> }; >> >> >>> >> >> >> >>> >> +#ifdef CONFIG_ACPI >> >> >>> >> +static struct fwnode_handle *xgene_msi_get_fwnode(struct device *dev) >> >> >>> >> +{ >> >> >>> >> + return xgene_msi_ctrl.fwnode; >> >> >>> >> +} >> >> >>> >> +#endif >> >> >>> >> + >> >> >>> >> static int xgene_allocate_domains(struct xgene_msi *msi) >> >> >>> >> { >> >> >>> >> msi->inner_domain = irq_domain_add_linear(NULL, NR_MSI_VEC, >> >> >>> >> @@ -256,7 +264,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi) >> >> >>> >> if (!msi->inner_domain) >> >> >>> >> return -ENOMEM; >> >> >>> >> >> >> >>> >> - msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->node), >> >> >>> >> + msi->msi_domain = pci_msi_create_irq_domain(msi->fwnode, >> >> >>> >> &xgene_msi_domain_info, >> >> >>> >> msi->inner_domain); >> >> >>> >> >> >> >>> >> @@ -265,6 +273,9 @@ static int xgene_allocate_domains(struct xgene_msi *msi) >> >> >>> >> return -ENOMEM; >> >> >>> >> } >> >> >>> >> >> >> >>> >> +#ifdef CONFIG_ACPI >> >> >>> >> + pci_msi_register_fwnode_provider(&xgene_msi_get_fwnode); >> >> >>> >> +#endif >> >> >>> >> return 0; >> >> >>> >> } >> >> >>> >> >> >> >>> >> @@ -449,6 +460,13 @@ static int xgene_msi_hwirq_free(unsigned int cpu) >> >> >>> >> {}, >> >> >>> >> }; >> >> >>> >> >> >> >>> >> +#ifdef CONFIG_ACPI >> >> >>> >> +static const struct acpi_device_id xgene_msi_acpi_ids[] = { >> >> >>> >> + {"APMC0D0E", 0}, >> >> >>> >> + { }, >> >> >>> >> +}; >> >> >>> >> +#endif >> >> >>> >> + >> >> >>> >> static int xgene_msi_probe(struct platform_device *pdev) >> >> >>> >> { >> >> >>> >> struct resource *res; >> >> >>> >> @@ -469,7 +487,17 @@ static int xgene_msi_probe(struct platform_device *pdev) >> >> >>> >> goto error; >> >> >>> >> } >> >> >>> >> xgene_msi->msi_addr = res->start; >> >> >>> >> - xgene_msi->node = pdev->dev.of_node; >> >> >>> >> + >> >> >>> >> + xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node); >> >> >>> >> + if (!xgene_msi->fwnode) { >> >> >>> >> + xgene_msi->fwnode = irq_domain_alloc_fwnode(NULL); >> >> >>> > >> >> >>> > Please provide something other than NULL, such as the base address if >> >> >>> > the device. That's quite useful for debugging. >> >> >>> > >> >> >>> >> + if (!xgene_msi->fwnode) { >> >> >>> >> + dev_err(&pdev->dev, "Failed to create fwnode\n"); >> >> >>> >> + rc = ENOMEM; >> >> >>> >> + goto error; >> >> >>> >> + } >> >> >>> >> + } >> >> >>> >> + >> >> >>> >> xgene_msi->num_cpus = num_possible_cpus(); >> >> >>> >> >> >> >>> >> rc = xgene_msi_init_allocator(xgene_msi); >> >> >>> >> @@ -540,6 +568,7 @@ static int xgene_msi_probe(struct platform_device *pdev) >> >> >>> >> .driver = { >> >> >>> >> .name = "xgene-msi", >> >> >>> >> .of_match_table = xgene_msi_match_table, >> >> >>> >> + .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids), >> >> >>> >> }, >> >> >>> >> .probe = xgene_msi_probe, >> >> >>> >> .remove = xgene_msi_remove, >> >> >>> >> >> >> >>> > >> >> >>> > The code is trivial, but relies on the MSI controller to probe before >> >> >>> > the PCI bus. What enforces this? How is it making sure that this is not >> >> >>> > going to break in the next kernel release? As far as I can tell, there >> >> >>> > is no explicit dependency between the two, making it the whole thing >> >> >>> > extremely fragile. >> >> >>> > >> >> >>> > Thanks, >> >> >>> > >> >> >>> > M. >> >> >>> > -- >> >> >>> > Jazz is not dead. It just smells funny... >> >> >>> >> >> >>> --