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

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

[Bjorn Helgaas]

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

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

[John Garry]

On 21/06/2019 14:56, Bjorn Helgaas wrote:
>>
>> > +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)

Hi Bjorn,

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

It's no coincidence that i2c and spi are the only other two non-ACPI 
core callers. For getting ACPI support for the hisi-lpc driver, we 
modeled the driver to have the same ACPI enumeration method as i2c and 
spi hosts. That is, allow the host driver to enumerate the child devices.

You can check drivers/acpi/scan.c::acpi_device_enumeration_by_parent() 
for where we make the check on the host and how it is used.

Thanks,
John

>
>> > +}
>> > +
>> >  /*
>> >   * 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;