linux-kernel - Re: [PATCH 3/3] PCI: qcom: Enable ECAM feature based on config size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241204224049.GA3023706@bhelgaas>
Date: Wed, 4 Dec 2024 16:40:49 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
Cc: cros-qcom-dts-watchers@...omium.org,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jingoo Han <jingoohan1@...il.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, quic_vbadigan@...cinc.com,
	quic_ramkri@...cinc.com, quic_nitegupt@...cinc.com,
	quic_skananth@...cinc.com, quic_vpernami@...cinc.com,
	quic_mrana@...cinc.com, mmareddy@...cinc.com,
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH 3/3] PCI: qcom: Enable ECAM feature based on config size

On Wed, Dec 04, 2024 at 07:56:54AM +0530, Krishna Chaitanya Chundru wrote:
> On 12/4/2024 12:29 AM, Bjorn Helgaas wrote:
> > On Sun, Nov 17, 2024 at 03:30:20AM +0530, Krishna chaitanya chundru wrote:
> > > Enable the ECAM feature if the config space size is equal to size required
> > > to represent number of buses in the bus range property.
> > > 
> > > The ELBI registers falls after the DBI space, so use the cfg win returned
> > > from the ecam init to map these regions instead of doing the ioremap again.
> > > ELBI starts at offset 0xf20 from dbi.
> > > 
> > > On bus 0, we have only the root complex. Any access other than that should
> > > not go out of the link and should return all F's. Since the IATU is
> > > configured for bus 1 onwards, block the transactions for bus 0:0:1 to
> > > 0:31:7 (i.e., from dbi_base + 4KB to dbi_base + 1MB) from going outside the
> > > link through ecam blocker through parf registers.

> > > +static bool qcom_pcie_check_ecam_support(struct device *dev)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(dev);
> > > +	struct resource bus_range, *config_res;
> > > +	u64 bus_config_space_count;
> > > +	int ret;
> > > +
> > > +	/* If bus range is not present, keep the bus range as maximum value */
> > > +	ret = of_pci_parse_bus_range(dev->of_node, &bus_range);
> > > +	if (ret) {
> > > +		bus_range.start = 0x0;
> > > +		bus_range.end = 0xff;
> > > +	}
> > 
> > I would have thought the generic OF parsing would already default to
> > [bus 00-ff]?
> > 
> if there is no bus-range of_pci_parse_bus_range is not updating it[1],
> the bus ranges is being updated to default value in
> devm_of_pci_get_host_bridge_resources()[2]

Understood.  But qcom uses dw_pcie_host_init(), which calls
devm_pci_alloc_host_bridge(), which ultimately calls
of_pci_parse_bus_range() and defaults to [bus 00-ff] if there's no
bus-range in DT:

  qcom_pcie_probe
    dw_pcie_host_init
      devm_pci_alloc_host_bridge
        devm_of_pci_bridge_init
          pci_parse_request_of_pci_ranges
            devm_of_pci_get_host_bridge_resources(0, 0xff)
              of_pci_parse_bus_range

So the question is why you need to do that again here.

I see that qcom_pcie_probe() calls qcom_pcie_check_ecam_support()
*before* it calls dw_pcie_host_init(), so I guess that's the immediate
answer.

But this is another reason why I think qcom_pcie_check_ecam_support()
is kind of a sub-optimal solution here.

I wonder if we should factor the devm_pci_alloc_host_bridge() call out
of dw_pcie_host_init() so drivers can take advantage of the DT parsing
it does.  It looks like mobiveil does it that way:

  ls_g4_pcie_probe
    devm_pci_alloc_host_bridge
    mobiveil_pcie_host_probe

  mobiveil_pcie_probe
    devm_pci_alloc_host_bridge
    mobiveil_pcie_host_probe

> [1]https://elixir.bootlin.com/linux/v6.12.1/source/drivers/pci/of.c#L193
> [2]https://elixir.bootlin.com/linux/v6.12.1/source/drivers/pci/of.c#L347