linux-kernel - Re: [PATCH v3 2/4] PCI: dwc: Add ECAM support with iATU configuration

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] [day] [month] [year] [list]

Message-ID: <b074baa0-07c5-450d-8531-e2b1d706df0b@oss.qualcomm.com>
Date: Tue, 28 Jan 2025 13:53:21 +0100
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Manivannan Sadhasivam <mani@...nel.org>,
        Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.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>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>, Jingoo Han <jingoohan1@...il.com>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        quic_vbadigan@...cinc.com, quic_vpernami@...cinc.com,
        quic_mrana@...cinc.com, mmareddy@...cinc.com,
        Krishna chaitanya chundru <quic_krichai@...cinc.com>
Subject: Re: [PATCH v3 2/4] PCI: dwc: Add ECAM support with iATU configuration

On 24.01.2025 7:12 AM, Manivannan Sadhasivam wrote:
> On Tue, Jan 21, 2025 at 02:32:20PM +0530, Krishna Chaitanya Chundru wrote:
>> The current implementation requires iATU for every configuration
>> space access which increases latency & cpu utilization.
>>
>> Designware databook 5.20a, section 3.10.10.3 says about CFG Shift Feature,
>> which shifts/maps the BDF (bits [31:16] of the third header DWORD, which
>> would be matched against the Base and Limit addresses) of the incoming
>> CfgRd0/CfgWr0 down to bits[27:12]of the translated address.
>>
>> Configuring iATU in config shift feature enables ECAM feature to access the
>> config space, which avoids iATU configuration for every config access.
>>
>> Add "ctrl2" into struct dw_pcie_ob_atu_cfg  to enable config shift feature.
>>
>> As DBI comes under config space, this avoids remapping of DBI space
>> separately. Instead, it uses the mapped config space address returned from
>> ECAM initialization. Change the order of dw_pcie_get_resources() execution
>> to achieve this.
>>
>> Enable the ECAM feature if the config space size is equal to size required
>> to represent number of buses in the bus range property, add a function
>> which checks this. The DWC glue drivers uses this function and decide to
>> enable ECAM mode or not.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> 
> Some minor comments inline. Overall, the patch LGTM!
> 
>> ---

[...]

>> +bool dw_pcie_ecam_supported(struct dw_pcie_rp *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct platform_device *pdev = to_platform_device(pci->dev);
>> +	struct resource *config_res, *bus_range;
>> +	u64 bus_config_space_count;
>> +
>> +	bus_range = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS)->res;
>> +	if (!bus_range)
>> +		return false;
>> +
>> +	config_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
>> +	if (!config_res)
>> +		return false;
>> +
>> +	bus_config_space_count = resource_size(config_res) >> PCIE_ECAM_BUS_SHIFT;
>> +
>> +	return bus_config_space_count >= resource_size(bus_range);
> 
> return !!(bus_config_space_count >= resource_size(bus_range));

You made me think that there's some weird interaction here, but C99 says

"Each of the operators < (less than), > (greater than), <= (less than or equal to),
and >= (greater than or equal to) shall yield 1 if the specified relation is true and
0 if it is false.89) The result has type int"

so I'm not sure this is necessary

Konrad