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: <437ad4b9-64d1-6095-8b9d-fc40760b8248@quicinc.com>
Date: Mon, 9 Dec 2024 10:09:52 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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 12/5/2024 4:10 AM, Bjorn Helgaas wrote:
> 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:
It makes sense to use this way in the next patch in the qcom driver will
call devm_pci_alloc_host_bridge() before calling dw_pcie_host_init() and
in dw_pcie_host_init() if the bridge is allocated dwc driver will skip
allocating the bridge so that other drivers will not be affected.

- Krishna Chaitanya.

> 
>    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