Patchwork [v1] ACPI / platform: Unbind, trim and unregister staled devices

login
register
mail settings
Submitter Andy Shevchenko
Date March 13, 2019, 4:59 p.m.
Message ID <20190313165933.6961-1-andriy.shevchenko@linux.intel.com>
Download mbox | patch
Permalink /patch/748311/
State New
Headers show

Comments

Andy Shevchenko - March 13, 2019, 4:59 p.m.
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@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/acpi_platform.c | 47 ++++++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c          |  7 +++---
 include/linux/acpi.h         |  2 ++
 3 files changed, 53 insertions(+), 3 deletions(-)
Rafael J. Wysocki - March 19, 2019, 9:47 p.m.
On Wed, Mar 13, 2019 at 5:59 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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
>

Patch

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