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

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

On 9/23/26 6:12 PM, Koichiro Den wrote:

Hello Den-san,

>>> @@ -90,12 +91,22 @@ struct rcar_gen4_pcie_drvdata {
>>>    	enum dw_pcie_device_mode mode;
>>>    };
>>> +enum rcar_gen4_pcie_state {
>>> +	/* The controller is being reset and reinitialized */
>>> +	RCAR_PCIE_RESETTING,
>>> +};
>>> +
>>>    struct rcar_gen4_pcie {
>>>    	struct dw_pcie dw;
>>>    	void __iomem *base;
>>>    	void __iomem *phy_base;
>>>    	struct platform_device *pdev;
>>>    	const struct rcar_gen4_pcie_drvdata *drvdata;
>>> +	unsigned long state;
>>
>> Can we simply use boolean flags here in struct rcar_gen4_pcie, instead of
>> the enum rcar_gen4_pcie_state ?
> 
> Sure! let me rework this for v2.

Thank you.

>>> +	/* Protects APP interrupt enable registers and their software state. */
>>> +	raw_spinlock_t app_lock;
>>> +	/* Serializes Root Port hardware reinitialization. */
>>> +	struct mutex reset_lock;
>>>    };
>>>    #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
>>> @@ -344,6 +355,33 @@ static int rcar_gen4_pcie_host_msi_addr(struct dw_pcie_rp *pp, u32 *msi_addr)
>>>    	return 0;
>>>    }
>>> +/* Whether the APP interrupt sources may currently be enabled. */
>>> +static bool rcar_gen4_pcie_irqs_blocked(struct rcar_gen4_pcie *rcar)
>>> +{
>>> +	return !!rcar->state;
>>> +}
>>> +
>>> +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 ?

Please correct me if I am wrong.

-- 
Best regards,
Marek Vasut