Re: [PATCH 06/11] PCI: rcar-gen4: Add Root Port reset support

[Marek Vasut]

On 9/28/26 5:50 AM, Koichiro Den wrote:

Hello Den-san,

> > > > > +static void rcar_gen4_pcie_app_irq_sync_locked(struct rcar_gen4_pcie *rcar)
> > > > > +{
> > > > > +	bool armed = !rcar_gen4_pcie_irqs_blocked(rcar);
> > > > > +	u32 val;
> > > > > +
> > > > > +	lockdep_assert_held(&rcar->app_lock);
> > > > > +
> > > > > +	val = readl(rcar->base + PCIEINTSTS0EN);
> > > > > +	val &= ~MSI_CTRL_INT;
> > > > > +	if (armed && pci_msi_enabled())
> > > > > +		val |= MSI_CTRL_INT;
> > > >
> > > > Should this function cache the state of pci_msi_enabled() in struct
> > > > rcar_gen4_pcie , so that in case rcar_gen4_pcie_irqs_blocked() reports MSIs
> > > > as blocked at this point ...
> > > >
> > > > > +	writel(val, rcar->base + PCIEINTSTS0EN);
> > > > > +}
> > > > > +
> > > > > +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);
> > > >
> > > > ... this function can enable MSIs once it is called and MSIs are no longer
> > > > blocked ?
> > >
> > > Even leaving aside rcar_gen4_pcie_resume_irqs(), which this patch introduces,
> > > rcar_gen4_pcie_app_irq_sync() re-evaluates the blocked state and
> > > pci_msi_enabled() on each call, so I would expect it to enable MSIs once
> > > unblocked, provided pci_msi_enabled() is true.
> > >
> > > Am I missing a case where a cached value would be needed?
> > >
> > > For example, even if there were a race with quirk_disable_all_msi(), I'm not
> > > sure caching the value would help.
> >
> > I think there might be a possible issue, please consider this:
> >
> > - The code enters rcar_gen4_pcie_reset_root_port() with MSI disabled
> >    (register PCIEINTSTS0EN bit MSI_CTRL_INT is unset)
> > - rcar_gen4_pcie_quiesce_irqs() is called, updates rcar->state
> > - rcar_gen4_pcie_resume_irqs() is called on exit from
> >    rcar_gen4_pcie_reset_root_port(), updates rcar->state again,
> >    and calls rcar_gen4_pcie_app_irq_sync_locked()
> > - At this point, I think MSI might be enabled instead of disabled ?
> >    (register PCIEINTSTS0EN bit MSI_CTRL_INT is SET)
> >
> > Is that correct ?
>
> I understand, but I'm still unsure when MSI_CTRL_INT should remain clear after
> a successful reset, with pci_msi_enabled() still true. And if there is such a
> case, wouldn't we need to preserve the intended enable state of MSI_CTRL_INT,
> rather than cache pci_msi_enabled()?

I only have a weak argument here -- I can think of a case where someone 
cleared MSI_CTRL_INT bit intentionally to prevent triggering an IRQ 
handler, with the intention of restoring that bit once their work was 
done. But I do agree this is a weak concern.

I will review V2 in the next couple of days.

Thank you for your help !

--
Best regards,
Marek Vasut