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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <gzw3rcuwuu7yswljzde2zszqlzkfsilozdfv2ebrcxjpvngpkk@hvzqb5wbjalb>
Date: Wed, 30 Apr 2025 13:00:56 +0530
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 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.

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

Also in any case, you need to call this before devm_pm_runtime_enable().
Otherwise, PM core will suspend the parent and enable it during
pm_runtime_get_noresume(), which is redundant.

- Mani

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