Re: [PATCH 2/2] PCI: meson: Add missing remove callback

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, May 18, 2026 at 10:44:18PM +0545, Shuvam Pandey wrote:
> meson_pcie_probe() powers on the PHY and registers the DesignWare host
> bridge with dw_pcie_host_init(), but the driver has no remove callback.
> On driver unbind or module unload, the driver core therefore proceeds to
> devres cleanup without first unregistering the host bridge or powering off
> the PHY.
> 
> Add a remove callback that deinitializes the DesignWare host bridge and
> powers off the PHY while device-managed resources are still valid.
> 
> Fixes: 9c0ef6d34fdb ("PCI: amlogic: Add the Amlogic Meson PCIe controller driver")
> Signed-off-by: Shuvam Pandey <shuvampandey1@gmail.com>
> ---
>  drivers/pci/controller/dwc/pci-meson.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 0694084f6..c96e2244a 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -451,6 +451,14 @@ static int meson_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void meson_pcie_remove(struct platform_device *pdev)
> +{
> +	struct meson_pcie *mp = platform_get_drvdata(pdev);
> +
> +	dw_pcie_host_deinit(&mp->pci.pp);
> +	meson_pcie_power_off(mp);

FYI, from internal Sashiko review while backporting this commit
(4b0dc84b2939 ("PCI: meson: Add missing remove callback")):

[Severity: Medium]
Will this manual call to meson_pcie_power_off() execute too early,
before devres tears down the clocks?

During probe, the initialization order powers on the PHY before
enabling theclocks. LIFO order requires disabling the clocks before
powering off the PHY.Because meson_pcie_probe_clocks() uses
devm_add_action_or_reset() to manage the clocks, they will be disabled
after meson_pcie_remove() returns.

Could this cause a hardware hang or undefined behavior during driver
unbind due to the clock and power sequencing violation?

[Severity: High]
This is a pre-existing issue, but does this remove function also need
to call reset_control_assert() to avoid leaking the shared reset
deassert count?

During probe, meson_pcie_get_resets() obtains a shared reset and
manually increments its deassert count:

  mrst->apb = meson_pcie_get_reset(mp, "apb", PCIE_SHARED_RESET);
  if (IS_ERR(mrst->apb))
    return PTR_ERR(mrst->apb);
  reset_control_deassert(mrst->apb);

Because the devm reset getters used here do not automatically assert
on cleanup, omitting reset_control_assert() in meson_pcie_remove()
means the apb shared reset will permanently have a non-zero
deassert_count. This prevents it from ever being asserted again, even
if all consumers unbind.

> +}
> +
>  static const struct of_device_id meson_pcie_of_match[] = {
>  	{
>  		.compatible = "amlogic,axg-pcie",
> @@ -464,6 +472,7 @@ MODULE_DEVICE_TABLE(of, meson_pcie_of_match);
>  
>  static struct platform_driver meson_pcie_driver = {
>  	.probe = meson_pcie_probe,
> +	.remove = meson_pcie_remove,
>  	.driver = {
>  		.name = "meson-pcie",
>  		.of_match_table = meson_pcie_of_match,
> -- 
> 2.50.1 (Apple Git-155)
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic