Re: [PATCH 4/5] bus: hisi_lpc: Add .remove method to avoid driver unbind crash

[Bjorn Helgaas <helgaas@kernel.org>]

[+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
>