| 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
| ||
|
Message-ID: <48e9d897-2cd3-48ef-b46a-635ae75f5ac6@ti.com>
Date: Mon, 1 Sep 2025 11:51:33 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Manivannan Sadhasivam <mani@...nel.org>
CC: Siddharth Vadapalli <s-vadapalli@...com>, <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 Mon, Sep 01, 2025 at 11:18:23AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Sep 01, 2025 at 10:27:51AM GMT, Siddharth Vadapalli wrote:
> > On Sun, Aug 31, 2025 at 06:15:13PM +0530, Manivannan Sadhasivam wrote:
> >
> > Hello Mani,
> >
> > > On Fri, Aug 29, 2025 at 02:46:28PM GMT, Siddharth Vadapalli wrote:
[...]
> > > > + /*
> > > > + * 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?
> >
> > By invoking 'pm_runtime_put_sync(dev)', the ref-count is being
> > decremented and it results in the device being powered off. I have
> > verified it by printing the power domain index within the functions for
> > powering off and powering on devices on the J7200 SoC. Logs:
> >
> > root@...00-evm:~# modprobe pci_j721e
> > [ 25.231675] [Powering On]: 240
> > [ 25.234848] j721e-pcie 2910000.pcie: host bridge /bus@...000/pcie@...0000 ranges:
> > [ 25.242378] j721e-pcie 2910000.pcie: IO 0x4100001000..0x4100100fff -> 0x0000001000
> > [ 25.250496] j721e-pcie 2910000.pcie: MEM 0x4100101000..0x41ffffffff -> 0x0000101000
> > [ 25.258588] j721e-pcie 2910000.pcie: IB MEM 0x0000000000..0xffffffffffff -> 0x0000000000
> > [ 25.267098] [Powering Off]: 240
> > [ 25.270318] [Powering On]: 240
> > [ 25.273511] [Powering On]: 187
> > [ 26.372361] j721e-pcie 2910000.pcie: PCI host bridge to bus 0000:00
> > [ 26.378666] pci_bus 0000:00: root bus resource [bus 00-ff]
> > [ 26.384156] pci_bus 0000:00: root bus resource [io 0x0000-0xfffff] (bus address [0x1000-0x100fff])
> > [ 26.393197] pci_bus 0000:00: root bus resource [mem 0x4100101000-0x41ffffffff] (bus address [0x00101000-0xffffffff])
> > [ 26.403728] pci 0000:00:00.0: [104c:b00f] type 01 class 0x060400 PCIe Root Port
> > [ 26.411044] pci 0000:00:00.0: PCI bridge to [bus 00]
> > [ 26.416009] pci 0000:00:00.0: bridge window [io 0x0000-0x0fff]
> > [ 26.422091] pci 0000:00:00.0: bridge window [mem 0x00000000-0x000fffff]
> > [ 26.428874] pci 0000:00:00.0: bridge window [mem 0x00000000-0x000fffff 64bit pref]
> > [ 26.436676] pci 0000:00:00.0: supports D1
> > [ 26.440699] pci 0000:00:00.0: PME# supported from D0 D1 D3hot
> > [ 26.448064] pci 0000:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> > [ 26.456274] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
> > [ 26.462923] pci 0000:00:00.0: PCI bridge to [bus 01]
> > [ 26.467933] pci_bus 0000:00: resource 4 [io 0x0000-0xfffff]
> > [ 26.473595] pci_bus 0000:00: resource 5 [mem 0x4100101000-0x41ffffffff]
> > [ 26.480337] pcieport 0000:00:00.0: of_irq_parse_pci: failed with rc=-22
> > [ 26.487479] pcieport 0000:00:00.0: PME: Signaling with IRQ 701
> > [ 26.493909] pcieport 0000:00:00.0: AER: enabled with IRQ 701
> >
> > In the above logs, '240' is the Power Domain Index for the PCIe
> > Controller on J7200 SoC. It is powered on initially before the driver is
> > probed.
>
> In that case, the driver should not call pm_runtime_get_sync() in its probe.
> What it should do is:
>
> pm_runtime_set_active()
> pm_runtime_enable()
If I understand correctly, are you suggesting the following?
j721e_pcie_probe()
pm_runtime_set_active()
pm_runtime_enable()
ret = j721e_pcie_ctrl_init(pcie);
/*
* PCIe Controller should be powered off here, but is there
* a way to ensure that it has been powered off?
*/
=> Program the strap settings and return to
j721e_pcie_probe()
/* Power on the PCIe Controller now */
ret = pm_runtime_get_sync(dev);
>
> But the driver is supporting several SoC variants. Does the bootloader enable
> PCIe controller for all of them?
By 'bootloader', I assume that you are referring to Firmware that is
responsible for powering on or off the Controller on the basis of Power
Management APIs from Linux. If so, yes, all the SoC variants are Powered
on prior to the probe function being invoked via the
'dev_pm_domain_attach()' API called in drivers/base/platform.c.
>
> > During driver probe, we see the logs corresponding to
> > "devm_pci_alloc_host_bridge()" from the timestamp of '25.234848' which
> > is prior to the invocation of 'j721e_pcie_ctrl_init()'. Some time around
> > the '25.267098' timestamp, the 'j721e_pcie_ctrl_init()' function is
> > invoked which then decrements the ref-count via 'pm_runtime_put_sync(dev)'
> > leading to the PCIe Controller being powered off. This seems to be
> > consistent across boot unlike the usage of 'dev_pm_domain_detach' which
> > handles the device power off via a workqueue as a result of which it may
> > not be powered off yet when 'j721e_pcie_ctrl_init()' is programming the
> > strap settings. Hence, I switched from 'dev_pm_domain_detach()' to
> > 'pm_runtime_put_sync()' in the v3 patch.
> >
>
> No using dev_pm_domain_detach() is a wrong approach.
>
> > Please let me know if you have any suggestions for alternative means to
> > power off the device in a reliable manner without deferring it to a
> > workqueue as done by the 'dev_pm_domain_detach()' API.
> >
>
> Using pm_runtime_put_sync() is the correct way, but the comment and patch
> description needs to be improved. In the comment, you are claiming that
> pm_runtime_put_sync() will power off the controller, even though it is true, it
> is not clear who is responsible for doing that. So reword it to reflect the fact
> that the power domain (genpd?) will turn off the controller.
Thank you for the feedback. Yes, it is indeed genpd that powers off the
PCIe Controller via corresponding requests to the Firmware that manages the
Power states of all devices in the SoC. I will update the commit message
to indicate this.
Regards,
Siddharth.
Powered by blists - more mailing lists