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

[Marek Vasut]

On 10/19/26 5:01 AM, Koichiro Den wrote:

[...]

> > > @@ -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.

Mixing guard and goto makes the code difficult to read, which would most 
likely lead to bugs, right. I think scoped_guard() and goto is OKish, as 
long as there is no goto inside the scoped guard (that would be horror).

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

Please see above, as long as there is no "goto out" inside the 
scoped_guard, I think it is fine to have scoped_guard in function with goto.

> Thanks for the review!
Thank you for your help !

[...]

--
Best regards,
Marek Vasut