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

[Koichiro Den]

On Sat, Oct 17, 2026 at 02:46:32PM +0200, Marek Vasut wrote:
> On 10/15/26 5:30 AM, Koichiro Den wrote:
> 
> [...]
> 
> > +static void rcar_gen4_pcie_app_irq_sync(struct rcar_gen4_pcie *rcar)
> > +{
> > +	guard(raw_spinlock_irqsave)(&rcar->app_lock);
> > +
> > +	rcar_gen4_pcie_app_irq_sync_locked(rcar);
> > +}
> > +
> >   static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
> >   {
> >   	struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
> >   	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > +	unsigned long flags;
> >   	u32 val;
> >   	int ret;
> > @@ -543,12 +589,7 @@ static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
> >   	}
> >   	/* Configure MSI interrupt signal */
> > -	val = readl(rcar->base + PCIEINTSTS0EN);
> > -	if (pci_msi_enabled())
> > -		val |= MSI_CTRL_INT;
> > -	else
> > -		val &= ~MSI_CTRL_INT;
> > -	writel(val, rcar->base + PCIEINTSTS0EN);
> > +	rcar_gen4_pcie_app_irq_sync(rcar);
> >   	return 0;
> > @@ -563,9 +604,11 @@ static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
> >   	writel(0, rcar->base + AXIINTCCONT);
> >   	/* Deconfigure MSI interrupt signal */
> > +	raw_spin_lock_irqsave(&rcar->app_lock, flags);
> 
> This is only a small nitpick. I wonder if you could use the same
> scoped_guard() {} call here instead of the raw_spin_lock_irqsave() and
> raw_spin_unlock_irqrestore() pair?
> 
> scoped_guard(raw_spinlock_irqsave, &rcar->app_lock) {
> 
> >   	val = readl(rcar->base + PCIEINTSTS0EN);
> >   	val &= ~MSI_CTRL_INT;
> >   	writel(val, rcar->base + PCIEINTSTS0EN);
> 
> }

Yeah, actually I've been always wondering whether to ask you and the other
maintainers about the guard/scoped_guard policy in this driver, or PCI
contreoller drivers more broadly. I'm not aware of any documented PCI-specific
policy, while netdev as an example, has some guidance on this.

In v2, Sashiko flagged the mix of goto-based cleanup and guard():
https://lore.kernel.org/r/20260928170317.B0B331F000FF@xxxxxxxxxxxxxxx/
so I switched that part to explicit locking in v3/v4.

Would you consider an explicit scoped_guard() block (instead of guard()) at the
end of the function OK, even though the function still uses goto? Or, maybe I
should have just ignored the feedback from Sashiko, and guard() would have been
fine here too?

Since I need to respin anyway:
https://lore.kernel.org/r/bi3rvyqpdu4iq6pfyrqvx7eplyim6gmg2ijal67mpateobmtlq@gec7zewfm76r/
I'm thinking of replacing the lock/unlock pair with scoped_guard() as you suggest.

Thanks for the review!

Best regards,
Koichiro Den

> 
> > +	raw_spin_unlock_irqrestore(&rcar->app_lock, flags);
> >   	return ret;
> >   }
> 
> Either way, this patch is
> 
> Reviewed-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
> 
> Thank you !