linux-kernel - Re: [PATCH 5/8] PCI: rzg3s-host: Add Initial PCIe Host Driver for Renesas RZ/G3S SoC

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: <20250512202550.GA1126561@bhelgaas>
Date: Mon, 12 May 2025 15:25:50 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
	manivannan.sadhasivam@...aro.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, geert+renesas@...der.be,
	magnus.damm@...il.com, mturquette@...libre.com, sboyd@...nel.org,
	saravanak@...gle.com, p.zabel@...gutronix.de,
	linux-pci@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 5/8] PCI: rzg3s-host: Add Initial PCIe Host Driver for
 Renesas RZ/G3S SoC

On Mon, May 05, 2025 at 02:26:43PM +0300, Claudiu Beznea wrote:
> On 01.05.2025 23:12, Bjorn Helgaas wrote:
> > On Wed, Apr 30, 2025 at 01:32:33PM +0300, Claudiu wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >>
> >> The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express
> >> Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions
> >> only as a root complex, with a single-lane (x1) configuration. The
> >> controller includes Type 1 configuration registers, as well as IP
> >> specific registers (called AXI registers) required for various adjustments.
> >>
> >> Other Renesas RZ SoCs (e.g., RZ/G3E, RZ/V2H) share the same AXI registers
> >> but have both Root Complex and Endpoint capabilities. As a result, the PCIe
> >> host driver can be reused for these variants with minimal adjustments.
> ...

> >> +static void rzg3s_pcie_irqs_init(struct rzg3s_pcie_host *host)
> > 
> > This and many of the following functions have names that don't
> > correspond to anything in other drivers, which makes it harder to
> > transfer knowledge between the drivers.  If you can find a pattern
> > somewhere to follow, it will make it easier for others to read the
> > driver.
> 
> OK, I'll think about it. Do you have a recomentation?

Not really.  Maybe pick a driver with recent activity.

> >> +static int rzg3s_pcie_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	void *devres_group_id;
> >> +	int ret;
> >> +
> >> +	devres_group_id = devres_open_group(dev, NULL, GFP_KERNEL);
> >> +	if (!devres_group_id)
> >> +		return -ENOMEM;
> > 
> > What's the benefit of using devres_open_group()?  No other PCI
> > controller drivers use it.
> 
> This driver uses devm_add_action_or_reset() to keep the error path simpler.
> Some of the action or reset registered handlers access the controller
> registers. Because the driver is attached to the platform bus and the
> dev_pm_domain_detach() is called right after driver remove [1] having devm
> action or reset handlers accessing controller register will later lead to
> hangs when the device_unbind_cleanup() -> devres_release_all() will be
> called on remove path. Other issue described in [2] may arries when doing
> continuous unbind/bind if the driver has runtime PM API (not case for this
> driver at the moment) that access directly controller registers.
> 
> This is because the dev_pm_domain_detach() drops the clocks from PM domain
> and any subsequent pm_runtime_resume() (or similar function) call will lead
> to no runtime resume of the device.
> 
> There is a solution proposed to this here [2] but it slowly progresses.
> Until this will be solved I chosed the appraoch of having the devres group
> opened here. If you agree with it, I had the intention to drop this call if
> there will be an accepted solution for it. If you are OK with going forward
> like this, for the moment, would to prefer me to add a comment about the
> reason the devres_open_group() is used here?
> 
> This is not PCIe specific but platform bus specific. There are other
> affected drivers on this side (e.g. rzg2l-adc [3], rzg3s-thermal [4]).
> 
> A similar solution as [2] is already used by the i2c subsystem.

OK.  Is there something unique about rzg3s that means it needs
devres_open_group(), while other PCI controller drivers do not?  Or
should the other drivers be using it too?  Maybe they have similar
latent defects that should be fixed.

If there's something unique about rzg3s, please add a brief comment
about what it is so we know why it needs devres_open_group().

Bjorn