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

[Marek Vasut <marek.vasut@mailbox.org>]

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@smtp.kernel.org/
> 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