On Wed, Mar 13, 2019 at 5:59 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> When the commit 68bdb6773289
>
> ("ACPI: add support for ACPI reconfiguration notifiers")
>
> introduced reconfiguration notifiers it misses the point that the ACPI table,
> which may be loaded and then unloded via ConfigFS, can contain devices that are
> not enumerated by their parents.
>
> In such case the stale platform device is dangling in the system while the rest
> of the devices from the same table are already gone.
>
> Introduce acpi_trim_stale_platform_devices() function that iterates over staled
> platform devices, unbinds and trims its ACPI companions and then unregisters
> the physical device itself.
>
> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> drivers/acpi/acpi_platform.c | 47 ++++++++++++++++++++++++++++++++++++
> drivers/acpi/scan.c | 7 +++---
> include/linux/acpi.h | 2 ++
> 3 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 1f32caa87686..529ef9621e4a 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -133,3 +133,50 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
> return pdev;
> }
> EXPORT_SYMBOL_GPL(acpi_create_platform_device);
> +
> +static int trim_stale_platform_device(struct device *dev, void *context)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + int ret;
> +
> + /* Skip devices enumerated by parent */
> + if (!adev || adev->flags.enumeration_by_parent)
> + return 0;
> +
> + /* Skip devices that are sharing ACPI companion except primary one */
> + if (acpi_get_first_physical_node(adev) != dev)
> + return 0;
> +
> + /* Skip alive devices */
> + ret = acpi_bus_get_status(adev);
> + if (!ret)
> + return 0;
> +
> + /*
> + * We have to call acpi_unbind_one() explicitly to avoid asynchronous
> + * call in device_platform_notify() against staled data.
> + */
I'm not sure if I understand the comment. What's the exact scenario
you are concerned about?
Also, wouldn't platform device unregistration before calling
acpi_bus_trim() work?
> + acpi_unbind_one(dev);
> + acpi_bus_trim(adev);
> +
> + dev_dbg(&adev->dev, "trimming platform device %s\n", dev_name(dev));
> + platform_device_unregister(to_platform_device(dev));
> + return 0;
> +}
> +
> +/**
> + * acpi_trim_stale_platform_devices - remove platform devices that are gone
> + * @pdev: platform device to start walking the hierarchy from
> + *
> + * Iterate over platform devices that have ACPI companion and represent
> + * physical node and check if the device is gone. In such case unbind its
> + * ACPI companion and trim, then unregister physical device itself.
> + */
> +int acpi_trim_stale_platform_devices(struct platform_device *pdev)
> +{
> + struct device *dev = pdev ? &pdev->dev : NULL;
> + struct bus_type *bus = &platform_bus_type;
> +
> + return bus_for_each_dev(bus, dev, NULL, trim_stale_platform_device);
> +}
> +EXPORT_SYMBOL_GPL(acpi_trim_stale_platform_devices);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5efd4219f112..4151181676dd 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2291,6 +2291,10 @@ static void acpi_table_events_fn(struct work_struct *work)
> acpi_scan_lock_acquire();
> acpi_bus_scan(ACPI_ROOT_OBJECT);
> acpi_scan_lock_release();
> + } else if (tew->event == ACPI_TABLE_EVENT_UNLOAD) {
> + acpi_scan_lock_acquire();
> + acpi_trim_stale_platform_devices(NULL);
> + acpi_scan_lock_release();
> }
The lock can be acquired around the entire if () statement. Also, it
might be slightly cleaner to use a switch () statement here.
>
> kfree(tew);
> @@ -2303,9 +2307,6 @@ void acpi_scan_table_handler(u32 event, void *table, void *context)
> if (!acpi_scan_initialized)
> return;
>
> - if (event != ACPI_TABLE_EVENT_LOAD)
> - return;
> -
> tew = kmalloc(sizeof(*tew), GFP_KERNEL);
> if (!tew)
> return;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 87715f20b69a..98a02f770d49 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -608,6 +608,8 @@ void acpi_walk_dep_device_list(acpi_handle handle);
>
> struct platform_device *acpi_create_platform_device(struct acpi_device *,
> struct property_entry *);
> +int acpi_trim_stale_platform_devices(struct platform_device *pdev);
> +
> #define ACPI_PTR(_ptr) (_ptr)
>
> static inline void acpi_device_set_enumerated(struct acpi_device *adev)
> --
> 2.20.1
>
