Re: [PATCH v2 09/15] PCI: rcar-gen4: Add Root Port reset support

[Koichiro Den]

On Sun, Oct 04, 2026 at 02:11:37AM +0200, Marek Vasut wrote:
> On 9/28/26 6:52 PM, Koichiro Den wrote:
> > Implement the host bridge reset_root_port() callback so PCI error
> > recovery can reset and reinitialize the R-Car controller. This also
> > provides the reset operation for the link-down handling added later.
> > 
> > Call .reinit() with clocks and PHY initialization retained, restore
> > the Root Port registers and restart link training.
> > 
> > Rather than tracking which APP interrupt enables survive the power
> > reset, derive them from software state through a single helper. A flag
> > keeps the sources masked from the start of a reset until one succeeds,
> > so a failed reinitialization does not re-enable them against an
> > uninitialized controller.
> > 
> > Serialize the reset with a mutex, as not all callers hold the Root
> > Port's device lock: pci_try_reset_function() on a downstream device only
> > locks that device before falling back to a parent bus reset.
> 
> Please pardon my ignorance, but is this maybe something that could be fixed
> in the core code ?

Yes. pci_reset_function() has locked the upstream bridge since 7e89efc6e9e4
("PCI: Lock upstream bridge for pci_reset_function()"), but
pci_try_reset_function() and pci_reset_function_locked() still do not. I am not
sure whether this is intentional, but in either case, that is a core matter and
separate from this series.

However: more to the point, your question made me look at this again, and I now
believe the mutex (rcar->reset_lock) was not needed in the first place. Every
path to reset_root_port() already holds a device lock that excludes the others:
- error recovery and bus resets go through pci_bus_lock(), which locks the Root
  Port and every device below it, and
- pci_try_reset_function() (even without the upstream bridge locked, as I noted
  above) holds the lock of the downstream device, which pci_parent_bus_reset()
  only accepts when it is the only device on the bus.

So I will drop the mutex in v3. Thanks for making me look at this again, it
really helps!

> 
> [...]
> 
> > +/*
> > + * R-Car Gen4 controllers have a single Root Port per instance, so the
> 
> I have two nitpicks here.
> 
> First, this is also applicable to R-Car Gen5 SoC PCIe4 controller, so please
> rephrase as:
> 
> -R-Car Gen4 controllers ...
> +R-Car Gen4 SoC PCIe controllers and R-Car Gen5 SoC PCIe4 controller ...
> 
> Second, in another review thread, Bjorn mentioned it would be good to be
> more explicit about what is SoC generation and what is PCIe generation:
> 
> https://lore.kernel.org/all/20260928222442.GA2266778@bhelgaas/
> 
> That is also why I used such a lengthy sentence above, that is
> 
>                               Controllers are here
>                                         |
>                        _________________^__________________
>                       |                                    |
>                vvvvvvvvvvvvvvvv                    vvvvvvvvvvvvvvvv
> R-Car Gen4 SoC PCIe controllers and R-Car Gen5 SoC PCIe4 controller ...
>       ^^^^^^^^                            ^^^^^^^^
>           |                                   |
>           '----------------- -----------------'
>                             V
>                             |
>                    SoC generation is here

That all makes sense. Thanks for the advice and for pointing me to the context.
I'll use your wording in the comment and all relevant commit messages in v3.

Best regards,
Koichiro Den

> 
> > + * 'pci_dev' is ignored and the whole controller is reset.
> > + */
> > +static int rcar_gen4_pcie_reset_root_port(struct pci_host_bridge *bridge,
> > +					  struct pci_dev *pdev)
> > +{
> > +	struct rcar_gen4_pcie *rcar = dev_get_drvdata(bridge->dev.parent);
> > +	struct dw_pcie *dw = &rcar->dw;
> > +	struct dw_pcie_rp *pp = &dw->pp;
> > +	struct device *dev = dw->dev;
> > +	int ret;
> The rest looks good, thank you !