| 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: <q4rbaadr7amsrtwaeickdjmcst77onuopir5rzpvixa7ow7udk@txwsmidjs3im>
Date: Fri, 16 May 2025 15:52:00 +0100
From: Manivannan Sadhasivam <mani@...nel.org>
To: Christian Bruel <christian.bruel@...s.st.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org, bhelgaas@...gle.com,
krzk+dt@...nel.org, conor+dt@...nel.org, mcoquelin.stm32@...il.com,
alexandre.torgue@...s.st.com, p.zabel@...gutronix.de, thippeswamy.havalige@....com,
shradha.t@...sung.com, quic_schintav@...cinc.com, cassel@...nel.org,
johan+linaro@...nel.org, linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/9] PCI: stm32: Add PCIe host support for STM32MP25
On Fri, May 16, 2025 at 10:37:16AM +0200, Christian Bruel wrote:
>
>
> On 5/15/25 13:29, Manivannan Sadhasivam wrote:
> > On Mon, May 12, 2025 at 05:08:13PM +0200, Christian Bruel wrote:
> > > Hi Manivannan,
> > >
> > > On 4/30/25 09:30, Manivannan Sadhasivam wrote:
> > > > On Wed, Apr 23, 2025 at 11:01:12AM +0200, Christian Bruel wrote:
> > > > > Add driver for the STM32MP25 SoC PCIe Gen1 2.5 GT/s and Gen2 5GT/s
> > > > > controller based on the DesignWare PCIe core.
> > > > >
> > > > > Supports MSI via GICv2m, Single Virtual Channel, Single Function
> > > > >
> > > > > Supports WAKE# GPIO.
> > > > >
> > > >
> > > > Mostly looks good. Just a couple of comments below.
> > > >
> > > > > Signed-off-by: Christian Bruel <christian.bruel@...s.st.com>
> > > > > ---
> > > > > drivers/pci/controller/dwc/Kconfig | 12 +
> > > > > drivers/pci/controller/dwc/Makefile | 1 +
> > > > > drivers/pci/controller/dwc/pcie-stm32.c | 368 ++++++++++++++++++++++++
> > > > > drivers/pci/controller/dwc/pcie-stm32.h | 15 +
> > > > > 4 files changed, 396 insertions(+)
> > > > > create mode 100644 drivers/pci/controller/dwc/pcie-stm32.c
> > > > > create mode 100644 drivers/pci/controller/dwc/pcie-stm32.h
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > +static int stm32_pcie_probe(struct platform_device *pdev)
> > > > > +{
> > > > > + struct stm32_pcie *stm32_pcie;
> > > > > + struct device *dev = &pdev->dev;
> > > > > + int ret;
> > > > > +
> > > > > + stm32_pcie = devm_kzalloc(dev, sizeof(*stm32_pcie), GFP_KERNEL);
> > > > > + if (!stm32_pcie)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + stm32_pcie->pci.dev = dev;
> > > > > + stm32_pcie->pci.ops = &dw_pcie_ops;
> > > > > + stm32_pcie->pci.pp.ops = &stm32_pcie_host_ops;
> > > > > +
> > > > > + stm32_pcie->regmap = syscon_regmap_lookup_by_compatible("st,stm32mp25-syscfg");
> > > > > + if (IS_ERR(stm32_pcie->regmap))
> > > > > + return dev_err_probe(dev, PTR_ERR(stm32_pcie->regmap),
> > > > > + "No syscfg specified\n");
> > > > > +
> > > > > + stm32_pcie->clk = devm_clk_get(dev, NULL);
> > > > > + if (IS_ERR(stm32_pcie->clk))
> > > > > + return dev_err_probe(dev, PTR_ERR(stm32_pcie->clk),
> > > > > + "Failed to get PCIe clock source\n");
> > > > > +
> > > > > + stm32_pcie->rst = devm_reset_control_get_exclusive(dev, NULL);
> > > > > + if (IS_ERR(stm32_pcie->rst))
> > > > > + return dev_err_probe(dev, PTR_ERR(stm32_pcie->rst),
> > > > > + "Failed to get PCIe reset\n");
> > > > > +
> > > > > + ret = stm32_pcie_parse_port(stm32_pcie);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + platform_set_drvdata(pdev, stm32_pcie);
> > > > > +
> > > > > + ret = pm_runtime_set_active(dev);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "Failed to activate runtime PM %d\n", ret);
> > > >
> > > > Please use dev_err_probe() here and below.
> > >
> > > OK, will report this in the EP driver also.
> > >
> > > >
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + ret = devm_pm_runtime_enable(dev);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "Failed to enable runtime PM %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + pm_runtime_get_noresume(dev);
> > > > > +
> > > >
> > > > I know that a lot of the controller drivers do this for no obvious reason. But
> > > > in this case, I believe you want to enable power domain or genpd before
> > > > registering the host bridge. Is that right?
> > >
> > > We call pm_runtime_enable() before stm32_add_pcie_port() and
> > > dw_pcie_host_init(). This ensures that PCIe is active during the PERST#
> > > sequence and before accessing the DWC registers.
> > >
> >
> > What do you mean by 'PCIe is active'? Who is activating it other than this
> > driver?
>
> "PCIe is active" in the sense of pm_runtime_active() and PM runtime_enabled.
>
> A better call point would be just before dw_host_init(), after the PCIe
> controller is reset :
>
> stm32_add_pcie_port()
> clk_prepare_enable()
also...
pm_runtime_set_active()
pm_runtime_no_callbacks()
> devm_pm_runtime_enable()
> dw_pcie_host_init()
>
> with this sequence, the stm32_pcie_suspend_noirq() is well balanced. does
> that sound better ?
>
Yeah.
> >
> > > > And the fact that you are not
> > > > decrementing the runtime usage count means, you want to keep it ON all the time?
> > > > Beware that your system suspend/resume calls would never get executed.
> > >
> > > We do not support PM runtime autosuspend, so we must notify PM runtime that
> > > PCIe is always active. Without invoking pm_runtime_get_noresume(), PCIe
> > > would mistakenly be marked as suspended.
> >
> > This cannot happen unless the child devices are also suspended? Or if there are
> > no child devices connected.
>
> If no device is connected or if one is active, without
> pm_runtime_get_noresume(), pm_genpd_summary says "PCIe suspended" despite
> being clocked and having accessible configuration space
>
This mostly means the hierarchy is not properly modelled. So the PM core doesn't
know that the child devices are active. You need to check the genpd summary for
PCI bridge and endpoint devices.
If there are no devices, then it is OK to be marked as suspended. But I would
assume that even if an user hotplugs an endpoint, it would just work since there
are no runtime PM callbacks populated. Only thing to worry is that, the genpd
core may turn off the power domain if all the devices in the hierarchy are
suspended. But we do have some workarounds for that, if power domains need to be
kept on. So let me know.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists