linux-kernel - Re: [PATCH v3] PCI: j721e: Fix programming sequence of "strap" settings

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: <oztilfun77apxtjxumx4tydkcd2gsalsak4m2rvsry2oooqjna@2yvcx6cnuemm>
Date: Sun, 31 Aug 2025 18:15:13 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org, 
	bhelgaas@...gle.com, helgaas@...nel.org, kishon@...nel.org, vigneshr@...com, 
	stable@...r.kernel.org, linux-pci@...r.kernel.org, linux-omap@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, srk@...com
Subject: Re: [PATCH v3] PCI: j721e: Fix programming sequence of "strap"
 settings

On Fri, Aug 29, 2025 at 02:46:28PM GMT, Siddharth Vadapalli wrote:
> The Cadence PCIe Controller integrated in the TI K3 SoCs supports both
> Root-Complex and Endpoint modes of operation. The Glue Layer allows
> "strapping" the Mode of operation of the Controller, the Link Speed
> and the Link Width. This is enabled by programming the "PCIEn_CTRL"
> register (n corresponds to the PCIe instance) within the CTRL_MMR
> memory-mapped register space. The "reset-values" of the registers are
> also different depending on the mode of operation.
> 
> Since the PCIe Controller latches onto the "reset-values" immediately
> after being powered on, if the Glue Layer configuration is not done while
> the PCIe Controller is off, it will result in the PCIe Controller latching
> onto the wrong "reset-values". In practice, this will show up as a wrong
> representation of the PCIe Controller's capability structures in the PCIe
> Configuration Space. Some such capabilities which are supported by the PCIe
> Controller in the Root-Complex mode but are incorrectly latched onto as
> being unsupported are:
> - Link Bandwidth Notification
> - Alternate Routing ID (ARI) Forwarding Support
> - Next capability offset within Advanced Error Reporting (AER) capability
> 
> Fix this by powering off the PCIe Controller before programming the "strap"
> settings and powering it on after that.
> 
> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
> ---
> 
> Hello,
> 
> This patch is based on commit
> 07d9df80082b Merge tag 'perf-tools-fixes-for-v6.17-2025-08-27' of git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools
> of Mainline Linux.
> 
> v2 of this patch is at:
> https://lore.kernel.org/r/20250819101336.292013-1-s-vadapalli@ti.com/
> Changes since v2:
> - Based on Bjorn's feedback at:
>   https://lore.kernel.org/r/20250819221748.GA598958@bhelgaas/
>   1) Commit message has been rephrased to summarize the issue and the
>   fix without elaborating too much on the details.
>   2) Description of the issue's symptoms noticeable by a user has been
>   added to the commit message.
>   3) Comment has been wrapped to fit within 80 columns.
>   4) The implementation has been simplified by moving the Controller
>   Power OFF and Power ON sequence into j721e_pcie_ctrl_init() as a
>   result of which the code reordering as well as function parameter
>   changes are no longer required.
> - Based on offline feedback from Vignesh, Runtime PM APIs are used
>   instead of PM DOMAIN APIs to power off and power on the PCIe
>   Controller.
> - Rebased patch on latest Mainline Linux.
> 
> Test Logs on J7200 EVM without the current patch applied show that the
> ARI Forwarding Capability incorrectly shows up as not being supported:
> https://gist.github.com/Siddharth-Vadapalli-at-TI/768bca36025ed630c4e69bcc3d94501a
> 
> Test Logs on J7200 EVM with the current patch applied show that the
> ARI Forwarding Capability correctly shows up as being supported:
> https://gist.github.com/Siddharth-Vadapalli-at-TI/fc1752d17140646c8fa57209eccd86ce
> 
> As explained in the commit message, this discrepancy is solely due to
> the PCIe Controller latching onto the incorrect reset-values which
> occurs when the strap settings are programmed after the PCIe Controller
> is powered on, at which point, the reset-values don't toggle anymore.
> 
> Regards,
> Siddharth.
> 
>  drivers/pci/controller/cadence/pci-j721e.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 6c93f39d0288..c178b117215a 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -284,6 +284,22 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>  	if (!ret)
>  		offset = args.args[0];
>  
> +	/*
> +	 * The PCIe Controller's registers have different "reset-values"
> +	 * depending on the "strap" settings programmed into the PCIEn_CTRL
> +	 * register within the CTRL_MMR memory-mapped register space.
> +	 * The registers latch onto a "reset-value" based on the "strap"
> +	 * settings sampled after the PCIe Controller is powered on.
> +	 * To ensure that the "reset-values" are sampled accurately, power
> +	 * off the PCIe Controller before programming the "strap" settings
> +	 * and power it on after that.
> +	 */
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to power off PCIe Controller\n");
> +		return ret;
> +	}

How does the controller gets powered off after pm_runtime_put_sync() since you
do not have runtime PM callbacks? I believe the parent is turning off the power
domain?

- Mani

-- 
மணிவண்ணன் சதாசிவம்