linux-kernel - Re: [PATCH v8 2/9] PCI: stm32: Add PCIe host support for STM32MP25

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: <ec33uuugief45swij7eu3mbx7htfxov6qa5miucqsrdp36z7qe@svpbhliveks4>
Date: Thu, 15 May 2025 12:29:21 +0100
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Christian Bruel <christian.bruel@...s.st.com>
Cc: 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 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?

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

- Mani

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