linux-kernel - Re: (subset) [PATCH v12 0/9] Add STM32MP25 PCIe drivers

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] [day] [month] [year] [list]
Message-ID: <bybl3ss5z6jgbhjqny5qwh25a5khhcvixoknrzqsnmennzcfdv@46j4grd35mx5>
Date: Wed, 25 Jun 2025 07:09:49 -0600
From: Manivannan Sadhasivam <mani@...nel.org>
To: Christian Bruel <christian.bruel@...s.st.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.org>, lpieralisi@...nel.org, kwilczynski@...nel.org, 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, 
	johan+linaro@...nel.org, cassel@...nel.org, shradha.t@...sung.com, 
	thippeswamy.havalige@....com, quic_schintav@...cinc.com, 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: (subset) [PATCH v12 0/9] Add STM32MP25 PCIe drivers

On Wed, Jun 25, 2025 at 12:18:16PM +0200, Christian Bruel wrote:
> 
> 
> On 6/25/25 06:00, Manivannan Sadhasivam wrote:
> > On Tue, Jun 24, 2025 at 05:22:06PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jun 23, 2025 at 06:13:07AM -0600, Manivannan Sadhasivam wrote:
> > > > On Tue, 10 Jun 2025 11:07:05 +0200, Christian Bruel wrote:
> > > > > Changes in v12;
> > > > >     Fix warning reported by kernel test robot <lkp@...el.com>
> > > > > 
> > > > > Changes in v11;
> > > > >     Address comments from Manivanna:
> > > > >     - RC driver: Do not call pm_runtime_get_noresume in probe
> > > > >                  More uses of dev_err_probe
> > > > >     - EP driver: Use level triggered PERST# irq
> > > > > 
> > > > > [...]
> > > > 
> > > > Applied, thanks!
> > > > 
> > > > [1/9] dt-bindings: PCI: Add STM32MP25 PCIe Root Complex bindings
> > > >        commit: 41d5cfbdda7a61c5d646a54035b697205cff1cf0
> > > > [2/9] PCI: stm32: Add PCIe host support for STM32MP25
> > > >        commit: f6111bc2d8fe6ffc741661126a2174523124dc11
> > > > [3/9] dt-bindings: PCI: Add STM32MP25 PCIe Endpoint bindings
> > > >        commit: 203cfc4a23506ffb9c48d1300348c290dbf9368e
> > > > [4/9] PCI: stm32: Add PCIe Endpoint support for STM32MP25
> > > >        commit: 8869fb36a107a9ff18dab8c224de6afff1e81dec
> > > > [5/9] MAINTAINERS: add entry for ST STM32MP25 PCIe drivers
> > > >        commit: 003902ed7778d62083120253cd282a9112674986
> > > 
> > > This doesn't build for me with the attached config:
> > > 
> > >    $ make drivers/pci/controller/dwc/pcie-stm32.o
> > >      CALL    scripts/checksyscalls.sh
> > >      DESCEND objtool
> > >      INSTALL libsubcmd_headers
> > >      CC      drivers/pci/controller/dwc/pcie-stm32.o
> > >    drivers/pci/controller/dwc/pcie-stm32.c: In function ‘stm32_pcie_suspend_noirq’:
> > >    drivers/pci/controller/dwc/pcie-stm32.c:83:16: error: implicit declaration of function ‘pinctrl_pm_select_sleep_state’ [-Werror=implicit-function-declaration]
> > >       83 |         return pinctrl_pm_select_sleep_state(dev);
> > > 	|                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >    drivers/pci/controller/dwc/pcie-stm32.c: In function ‘stm32_pcie_resume_noirq’:
> > >    drivers/pci/controller/dwc/pcie-stm32.c:96:24: error: ‘structdevice’ has no member named ‘pins’
> > >       96 |         if (!IS_ERR(dev->pins->init_state))
> > > 	|                        ^~
> > >    drivers/pci/controller/dwc/pcie-stm32.c:97:23: error: implicit declaration of function ‘pinctrl_select_state’ [-Werror=implicit-function-declaration]
> > >       97 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
> > > 	|                       ^~~~~~~~~~~~~~~~~~~~
> > >    drivers/pci/controller/dwc/pcie-stm32.c:97:47: error: ‘structdevice’ has no member named ‘pins’
> > >       97 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
> > > 	|                                               ^~
> > >    drivers/pci/controller/dwc/pcie-stm32.c:97:61: error: ‘structdevice’ has no member named ‘pins’
> > >       97 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
> > > 	|                                                             ^~
> > >    drivers/pci/controller/dwc/pcie-stm32.c:99:23: error: implicit declaration of function ‘pinctrl_pm_select_default_state’ [-Werror=implicit-function-declaration]
> > >       99 |                 ret = pinctrl_pm_select_default_state(dev);
> > > 	|                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > 
> > Hmm... I see two issues here. First is, wrong pinctrl header used. The correct
> > one is:
> > 
> > #include <linux/pinctrl/consumer.h>
> 
> ah yes, the missing pinctrl_pm_select_default_state() should indeed be fixed
> by using the correct header.
> 

I've fixed it in the branch.

> > 
> > Second issue is the driver accessing "struct device::pins" directly. The "pins"
> > member won't be available if CONFIG_PINCTRL is not set (which is what your
> > .config has). So either the member should not be accessed directly or the
> > driver has to depend on CONFIG_PINCTRL. The latter one is not acceptable.It
> > also looks weird that only this driver is accessing the "pins" member directly
> > apart from the pinctrl core. So I think this part needs a revisit.
> > 
> > Christian?
> The pinctrl "init" and "default" configurations are managed effectively by
> the probing code. The same approach is required in
> stm32_pcie_resume_noirq().
> 
> In this case, would introducing a new helper function,
> pinctrl_pm_select_init_state(), be preferable, even if we are the only
> consumer?
> 

Sounds reasonable. If you end up creating an API, get it acked by the pinctrl
maintainer so that I can merge it (and the use of it in this driver) through
the PCI tree to avoid cross tree dependency.

- Mani

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