* Re: [PATCH 4/5] bus: hisi_lpc: Add .remove method to avoid driver unbind crash
[not found] ` <1561026716-140537-5-git-send-email-john.garry@huawei.com>
@ 2019-06-21 13:56 ` Bjorn Helgaas
2019-06-21 14:33 ` John Garry
0 siblings, 1 reply; 2+ messages in thread
From: Bjorn Helgaas @ 2019-06-21 13:56 UTC (permalink / raw)
To: John Garry
Cc: xuwei5, linuxarm, arm, linux-kernel, linux-pci, joe, linux-acpi,
Rafael J. Wysocki
[+cc Rafael, linux-acpi]
On Thu, Jun 20, 2019 at 06:31:55PM +0800, John Garry wrote:
> The original driver author seemed to be under the impression that a driver
> cannot be removed if it does not have a .remove method. Or maybe if it is
> a built-in platform driver.
>
> This is not true. This crash can be created:
>
> root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# echo HISI0191\:00 > unbind
> root@ubuntu:/sys/bus/platform/drivers/hisi-lpc# ipmitool raw 6 1
> Unable to handle kernel paging request at virtual address ffff000010035010
> ...
> The problem here is that the host goes away but the associated logical PIO
> region remains registered, as do the child devices.
>
> Fix by adding a .remove method to tidy-up by removing the child devices
> and unregistering the logical PIO region.
>
> Fixes: adf38bb0b595 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> drivers/bus/hisi_lpc.c | 34 ++++++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 6d301aafcad2..0e9f1f141c93 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -456,6 +456,17 @@ struct hisi_lpc_acpi_cell {
> size_t pdata_size;
> };
>
> +static void hisi_lpc_acpi_remove(struct device *hostdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(hostdev);
> + struct acpi_device *child;
> +
> + device_for_each_child(hostdev, NULL, hisi_lpc_acpi_remove_subdev);
> +
> + list_for_each_entry(child, &adev->children, node)
> + acpi_device_clear_enumerated(child);
There are only two other non-ACPI core callers of
acpi_device_clear_enumerated() (i2c and spi). That always makes me
wonder if we're getting too deep in ACPI internals.
> +}
> +
> /*
> * hisi_lpc_acpi_probe - probe children for ACPI FW
> * @hostdev: LPC host device pointer
> @@ -555,8 +566,7 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
> return 0;
>
> fail:
> - device_for_each_child(hostdev, NULL,
> - hisi_lpc_acpi_remove_subdev);
> + hisi_lpc_acpi_remove(hostdev);
> return ret;
> }
>
> @@ -626,6 +636,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> return ret;
> }
>
> + dev_set_drvdata(dev, lpcdev);
> +
> io_end = lpcdev->io_host->io_start + lpcdev->io_host->size;
> dev_info(dev, "registered range [%pa - %pa]\n",
> &lpcdev->io_host->io_start, &io_end);
> @@ -633,6 +645,23 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> return ret;
> }
>
> +static int hisi_lpc_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct acpi_device *acpi_device = ACPI_COMPANION(dev);
> + struct hisi_lpc_dev *lpcdev = dev_get_drvdata(dev);
> + struct logic_pio_hwaddr *range = lpcdev->io_host;
> +
> + if (acpi_device)
> + hisi_lpc_acpi_remove(dev);
> + else
> + of_platform_depopulate(dev);
> +
> + logic_pio_unregister_range(range);
> +
> + return 0;
> +}
> +
> static const struct of_device_id hisi_lpc_of_match[] = {
> { .compatible = "hisilicon,hip06-lpc", },
> { .compatible = "hisilicon,hip07-lpc", },
> @@ -646,5 +675,6 @@ static struct platform_driver hisi_lpc_driver = {
> .acpi_match_table = ACPI_PTR(hisi_lpc_acpi_match),
> },
> .probe = hisi_lpc_probe,
> + .remove = hisi_lpc_remove,
> };
> builtin_platform_driver(hisi_lpc_driver);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread