Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp2659008imm; Mon, 24 Sep 2018 07:59:07 -0700 (PDT) X-Google-Smtp-Source: ACcGV62L6oIBSuhdh2kyJoL65Dkd7qVHmfVWjoU/2tz2J/4uuwLoxw/iByJ50+MxKyTEhxuRceBp X-Received: by 2002:a63:f414:: with SMTP id g20-v6mr9868975pgi.407.1537801146973; Mon, 24 Sep 2018 07:59:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1537801146; cv=none; d=google.com; s=arc-20160816; b=q7aFy15AIVvTM+Z1a91FFxHNkM5K5pMhBaN33Sj2zTMnGfuPsYfR3+1BdYOZgeFhK/ XErtwqbB44Vg/Gum5IGYZSQVJqX4W+mA+Y0fwdihqMMXjf+jhF16FCH+pRRflcWcruCI XGcBepdr9Boqb9W03hK4b5E0ACYEbsSjpRtHXQqJjBE35V10qK6s90ZpWQ0CuJ45jnc3 R3IKN6CB/PN9jCHWJ5NTiTvsuYSRm5PBkn5c2GH0f06Wpu9Upnj0EsIjPNgnPOOR9WYA fbLzEIq1ewAxipkUFPL/1URZ8RBS+3DfKBfBRHtZ7w663GbY4Qjs8y28QHV2Z64biMuO m0JQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=VqESojIUdQMXa8G5oE7fx0wTI/9rxUJ/7QE6Nf3xgmY=; b=HHO+x8E4XcjzG8wDSxp1fKVe1us4AfS0Ouhqg4J5hemZSuPK3MzQan8zL8nYclWUsD s8+RZ1tTuHUhwupcS4bwhHtuHi60j7IU9Sgjrg4+hvOpQY3GYCDA0A5owqC+D09Lhy7u ZdXIQiL917Sk+WySn87+1h9ogEr+SziX5Zxj/eqESVLgP5JU76QnbEAX8+1WxpT1e3T+ M260L3nOM+ILHjLLr3KJkkVS6dnMTUeexz20wP9KIWFw1XiLAE+nwjppouXHRnQKfPhK yOxnj94c+eEKHXZ++ZU26K/aSbc37Qe+rt3xeJ/LVHilpzieCusdvqltdKRoOUrHxRax XhFA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id z11-v6si31622154pgv.138.2018.09.24.07.58.50; Mon, 24 Sep 2018 07:59:06 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728362AbeIXURk (ORCPT + 99 others); Mon, 24 Sep 2018 16:17:40 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:36286 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725982AbeIXURk (ORCPT ); Mon, 24 Sep 2018 16:17:40 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4940080D; Mon, 24 Sep 2018 07:15:19 -0700 (PDT) Received: from e107981-ln.cambridge.arm.com (e107981-ln.Emea.Arm.com [10.4.13.117]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A1E5A3F5B7; Mon, 24 Sep 2018 07:15:17 -0700 (PDT) Date: Mon, 24 Sep 2018 15:15:12 +0100 From: Lorenzo Pieralisi To: Thomas Petazzoni Cc: Russell King - ARM Linux , Jan Kundr??t , Baruch Siach , Jason Cooper , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Bjorn Helgaas , linux-arm-kernel@lists.infradead.org Subject: Re: [BISECTED] Regression: Solidrun Clearfog Base won't boot since "PCI: mvebu: Only remap I/O space if configured" Message-ID: <20180924141512.GA11875@e107981-ln.cambridge.arm.com> References: <20180912231050.GX30658@n2100.armlinux.org.uk> <20180913094515.351967bb@windsurf> <5ad46fec-a71a-477a-b23f-d20aacfb481d@cesnet.cz> <20180913104241.65db8243@windsurf> <20180924101213.GO30658@n2100.armlinux.org.uk> <20180924122614.70738b5c@windsurf> <20180924111341.GP30658@n2100.armlinux.org.uk> <20180924141203.3df9707a@windsurf> <20180924124620.GA10322@e107981-ln.cambridge.arm.com> <20180924151040.5e57462b@windsurf> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180924151040.5e57462b@windsurf> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 24, 2018 at 03:10:40PM +0200, Thomas Petazzoni wrote: > Hello, > > On Mon, 24 Sep 2018 13:46:29 +0100, Lorenzo Pieralisi wrote: > > > What I think you can do short term, given that AFAICS MVEBU is not > > removable, instead of using pci_host_probe() you move part of its code > > into the driver and make sure that you remap IO as last operation before > > probe completion (ie after scanning the host bridge) so that you do not > > need to unmap it on failure; write a commit log summarising/linking this > > thread please and when v4.20 lands we will give this a more thorough > > look as Russell requested. > > > > How does that sound ? > > The only thing that can fail in pci_host_probe() is: > > ret = pci_scan_root_bus_bridge(bridge); > if (ret < 0) { > dev_err(bridge->dev.parent, "Scanning root bridge > failed"); return ret; > } > > In the pci-mvebu driver prior to the conversion to pci_host_probe(), > the code flow at the end of ->probe() was: > > mvebu_pcie_enable() > pci_common_init_dev() > pcibios_init_hw() > > and pcibios_init_hw() calls pci_scan_root_bus_bridge(), without doing > much about the return value other than issuing a warning: > > ret = pci_scan_root_bus_bridge(bridge); > } > > if (WARN(ret < 0, "PCI: unable to scan bus!")) { > pci_free_host_bridge(bridge); > break; > } > > I.e, even before the conversion to pci_host_probe(), in case of > failure in pci_scan_root_bus_bridge(), we would have the I/O mapping in > place, but the PCI controller not registered. > > We could keep the same (not great) behavior by doing: > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c > index 50eb0729385b..487492f0c5f7 100644 > --- a/drivers/pci/controller/pci-mvebu.c > +++ b/drivers/pci/controller/pci-mvebu.c > @@ -1179,9 +1179,6 @@ static int mvebu_pcie_parse_request_resources(struct mvebu_pcie *pcie) > resource_size(&pcie->io) - 1); > pcie->realio.name = "PCI I/O"; > > - for (i = 0; i < resource_size(&pcie->realio); i += SZ_64K) > - pci_ioremap_io(i, pcie->io.start + i); > - > pci_add_resource(&pcie->resources, &pcie->realio); > } > > @@ -1197,7 +1194,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > struct device_node *child; > int num, i, ret; > > - bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct mvebu_pcie)); > + bridge = pci_alloc_host_bridge(sizeof(struct mvebu_pcie)); > if (!bridge) > return -ENOMEM; > > @@ -1212,8 +1209,10 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > num = of_get_available_child_count(np); > > pcie->ports = devm_kcalloc(dev, num, sizeof(*pcie->ports), GFP_KERNEL); > - if (!pcie->ports) > - return -ENOMEM; > + if (!pcie->ports) { > + ret = -ENOMEM; > + goto free_host_bridge; > + } > > i = 0; > for_each_available_child_of_node(np, child) { > @@ -1222,7 +1221,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > ret = mvebu_pcie_parse_port(pcie, port, child); > if (ret < 0) { > of_node_put(child); > - return ret; > + goto free_host_bridge; > } else if (ret == 0) { > continue; > } > @@ -1268,7 +1267,21 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > bridge->align_resource = mvebu_pcie_align_resource; > bridge->msi = pcie->msi; > > - return pci_host_probe(bridge); > + if (resource_size(&pcie->io) != 0) { > + for (i = 0; i < resource_size(&pcie->realio); i += SZ_64K) > + pci_ioremap_io(i, pcie->io.start + i); > + } > + > + ret = pci_host_probe(bridge); > + if (ret) > + pci_free_host_bridge(bridge); > + > + /* Yes, when pci_host_probe() returns a failure, we don't care */ > + return 0; > + > +free_host_bridge: > + pci_free_host_bridge(bridge); > + return ret; > } > > static const struct of_device_id mvebu_pcie_of_match_table[] = { > > I.e, we simply ignore the failure of pci_host_probe(). > > To be honest, I really prefer the option of introducing pci_unmap_io(). I understand that, I wanted to make sure we come up with a fix asap and what I put forward would cover everything discussed in this thread, at least temporarily, giving us time to check ISA related issues while unmapping IO space. Lorenzo