Re: [PATCH v2] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Runtime PM is disabled asynchronously and prematurely during driver removal, leading to an incorrect power state and violating PM guidelines.
- [High] IRQ domains are destroyed during host deinit while their associated interrupts are still mapped and active.
--

--- Patch [1]: [PATCH v2] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe ---
commit e9cae95ac8bfa924ebc45ae6433e2c2961f70890
Author: Ali Tariq <alitariq45892@gmail.com>

    PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

    pm_runtime_get_sync() is called in starfive_pcie_probe() without
    checking its return value. If runtime resume fails, the driver
    proceeds to configure PCIe hardware through regmap_update_bits(),
    enable clocks and resets, and power on the PHY, even though the
    device may not actually be powered.

> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
> index 298036c3e7f94..e9a472985b8ac 100644
> --- a/drivers/pci/controller/plda/pcie-starfive.c
> +++ b/drivers/pci/controller/plda/pcie-starfive.c
> @@ -410,7 +410,11 @@ static int starfive_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0) {
> +		pm_runtime_disable(&pdev->dev);
> +		return dev_err_probe(dev, ret, "failed to resume device\n");
> +	}
>  
>  	plda->host_ops = &sf_host_ops;
>  	plda->num_events = PLDA_MAX_EVENT_NUM;

While reviewing this patch, I noticed a couple of pre-existing issues in
related code:

> static void starfive_pcie_remove(struct platform_device *pdev)
> {
> 	struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev);
> 
> 	pm_runtime_put(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> 	plda_pcie_host_deinit(&pcie->plda);
> 	platform_set_drvdata(pdev, NULL);
> }

[Severity: High]
This isn't a bug introduced by this patch, but should this use 
pm_runtime_put_sync() instead?

In starfive_pcie_remove(), pm_runtime_put() queues an asynchronous idle
request, but pm_runtime_disable() is called immediately after. This cancels
the pending request, leaving the device in an active power state indefinitely.

Additionally, this teardown happens before plda_pcie_host_deinit(). If
pm_runtime_put_sync() were used, the hardware would drop power immediately,
causing plda_pcie_host_deinit() to access unpowered registers and potentially
crash the system.

Should the host bridge be torn down first, and only then synchronously power
off the device?


Also in drivers/pci/controller/plda/pcie-plda-host.c:

> static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie)
> {
> 	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> 	irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
> 	irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL);
> 
> 	irq_domain_remove(pcie->msi.dev_domain);
> 
> 	irq_domain_remove(pcie->intx_domain);
> 	irq_domain_remove(pcie->event_domain);
> }

[Severity: High]
This is a pre-existing issue, but are we destroying the IRQ domains while
their associated interrupts are still mapped and active?

During driver removal, starfive_pcie_remove() calls plda_pcie_host_deinit(),
which synchronously removes the event and INTx IRQ domains via
plda_pcie_irq_domain_deinit().

However, the interrupts mapped from these domains are never unmapped via
irq_dispose_mapping(). The actual interrupt handlers requested via
devm_request_irq() in plda_init_interrupts() haven't been freed yet, because
devm teardown occurs strictly after the driver remove() callback returns.

Is it safe to destroy these domains with active mappings and bound interrupts,
or could this lead to issues during subsequent devm teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718153352.661930-1-alitariq45892@gmail.com?part=1