On Thu, Mar 09, 2017 at 06:46:15PM -0800, Brian Norris wrote:
> Currently, if we try to unbind the platform device, the remove will
> succeed, but the removal won't undo most of the registration, leaving
> partially-configured PCI devices in the system.
>
> This allows, for example, a simple 'lspci' to crash the system, as it
> will try to touch the freed (via devm_*) driver structures.
>
> So let's implement device remove().
How exactly do you reproduce this problem?
There are several other drivers that are superficially similar, e.g.,
they define a struct platform_driver without a .remove method. Do
they all have this problem? Some of them do set .suppress_bind_attrs
= true; is that relevant to this scenario?
In fact, the only other callers of pci_remove_root_bus() are
iproc_pcie_remove(), hv_pci_remove(), and vmd_remove().
These don't have .remove:
imx6_pcie_driver
ls_pcie_driver
armada8k_pcie_driver
artpec6_pcie_driver
dw_plat_pcie_driver
hisi_pcie_driver
hisi_pcie_almost_ecam_driver
spear13xx_pcie_driver
gen_pci_driver
These don't have .remove but do set .suppress_bind_attrs = true:
dra7xx_pcie_driver
qcom_pcie_driver
advk_pcie_driver
mvebu_pcie_driver
rcar_pci_driver
rcar_pcie_driver
tegra_pcie_driver
altera_pcie_driver
nwl_pcie_driver
xilinx_pcie_driver
> Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> ---
> v2:
> * unmap IO space with pci_unmap_iospace()
> * remove IRQ domain
> ---
> drivers/pci/host/pcie-rockchip.c | 36 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 5d7b27b1e941..d2e5078ae331 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -223,9 +223,11 @@ struct rockchip_pcie {
> int link_gen;
> struct device *dev;
> struct irq_domain *irq_domain;
> - u32 io_size;
> int offset;
> + struct pci_bus *root_bus;
> + struct resource *io;
> phys_addr_t io_bus_addr;
> + u32 io_size;
> void __iomem *msg_region;
> u32 mem_size;
> phys_addr_t msg_bus_addr;
> @@ -1360,6 +1362,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> err, io);
> continue;
> }
> + rockchip->io = io;
> break;
> case IORESOURCE_MEM:
> mem = win->res;
> @@ -1391,6 +1394,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> err = -ENOMEM;
> goto err_free_res;
> }
> + rockchip->root_bus = bus;
>
> pci_bus_size_bridges(bus);
> pci_bus_assign_resources(bus);
> @@ -1421,6 +1425,34 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> return err;
> }
>
> +static int rockchip_pcie_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> + pci_stop_root_bus(rockchip->root_bus);
> + pci_remove_root_bus(rockchip->root_bus);
> + pci_unmap_iospace(rockchip->io);
> + irq_domain_remove(rockchip->irq_domain);
> +
> + phy_power_off(rockchip->phy);
> + phy_exit(rockchip->phy);
> +
> + clk_disable_unprepare(rockchip->clk_pcie_pm);
> + clk_disable_unprepare(rockchip->hclk_pcie);
> + clk_disable_unprepare(rockchip->aclk_perf_pcie);
> + clk_disable_unprepare(rockchip->aclk_pcie);
> +
> + if (!IS_ERR(rockchip->vpcie3v3))
> + regulator_disable(rockchip->vpcie3v3);
> + if (!IS_ERR(rockchip->vpcie1v8))
> + regulator_disable(rockchip->vpcie1v8);
> + if (!IS_ERR(rockchip->vpcie0v9))
> + regulator_disable(rockchip->vpcie0v9);
> +
> + return 0;
> +}
> +
> static const struct dev_pm_ops rockchip_pcie_pm_ops = {
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
> rockchip_pcie_resume_noirq)
> @@ -1438,6 +1470,6 @@ static struct platform_driver rockchip_pcie_driver = {
> .pm = &rockchip_pcie_pm_ops,
> },
> .probe = rockchip_pcie_probe,
> -
> + .remove = rockchip_pcie_remove,
> };
> builtin_platform_driver(rockchip_pcie_driver);
> --
> 2.12.0.246.ga2ecc84866-goog
>
