[PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce acpi_device_fix_up_power_extended() for fixing up power of
a device having an ACPI companion in a manner that takes the device's
children into account and make the MMC code use it in two places
instead of walking the list of the device ACPI companion's children
directly.

This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
 drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
 drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
 include/acpi/acpi_bus.h           |    1 +
 4 files changed, 28 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
+++ linux-pm/drivers/mmc/host/sdhci-acpi.c
@@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
 {
 	struct device *dev = &pdev->dev;
 	const struct sdhci_acpi_slot *slot;
-	struct acpi_device *device, *child;
 	const struct dmi_system_id *id;
+	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
 	slot = sdhci_acpi_get_slot(device);
 
 	/* Power on the SDHCI controller and its children */
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	acpi_device_fix_up_power_extended(device);
 
 	if (sdhci_acpi_byt_defer(dev))
 		return -EPROBE_DEFER;
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
 }
 EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
 
+static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
+{
+	if (adev->status.present && adev->status.enabled)
+		acpi_device_fix_up_power(adev);
+
+	return 0;
+}
+
+/**
+ * acpi_device_fix_up_power_extended - Force device and its children into D0.
+ * @adev: Parent device object whose power state is to be fixed up.
+ *
+ * Call acpi_device_fix_up_power() for @adev and its children so long as they
+ * are reported as present and enabled.
+ */
+void acpi_device_fix_up_power_extended(struct acpi_device *adev)
+{
+	acpi_device_fix_up_power(adev);
+	acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
+
 int acpi_device_update_power(struct acpi_device *device, int *state_p)
 {
 	int state;
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
 int acpi_device_set_power(struct acpi_device *device, int state);
 int acpi_bus_init_power(struct acpi_device *device);
 int acpi_device_fix_up_power(struct acpi_device *device);
+void acpi_device_fix_up_power_extended(struct acpi_device *adev);
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
 int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
+++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
@@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
 #ifdef CONFIG_ACPI
 static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
 {
-	struct acpi_device *device, *child;
+	struct acpi_device *device;
 
 	device = ACPI_COMPANION(&slot->chip->pdev->dev);
-	if (!device)
-		return;
-
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	if (device)
+		acpi_device_fix_up_power_extended(device);
 }
 #else
 static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}

Re: [PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[Adrian Hunter]

On 9/06/22 17:18, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Introduce acpi_device_fix_up_power_extended() for fixing up power of
> a device having an ACPI companion in a manner that takes the device's
> children into account and make the MMC code use it in two places
> instead of walking the list of the device ACPI companion's children
> directly.
> 
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
>  drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
>  drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
>  include/acpi/acpi_bus.h           |    1 +
>  4 files changed, 28 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
> +++ linux-pm/drivers/mmc/host/sdhci-acpi.c
> @@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
>  {
>  	struct device *dev = &pdev->dev;
>  	const struct sdhci_acpi_slot *slot;
> -	struct acpi_device *device, *child;
>  	const struct dmi_system_id *id;
> +	struct acpi_device *device;
>  	struct sdhci_acpi_host *c;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
> @@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
>  	slot = sdhci_acpi_get_slot(device);
>  
>  	/* Power on the SDHCI controller and its children */
> -	acpi_device_fix_up_power(device);
> -	list_for_each_entry(child, &device->children, node)
> -		if (child->status.present && child->status.enabled)
> -			acpi_device_fix_up_power(child);
> +	acpi_device_fix_up_power_extended(device);
>  
>  	if (sdhci_acpi_byt_defer(dev))
>  		return -EPROBE_DEFER;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
>  }
>  EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
>  
> +static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
> +{
> +	if (adev->status.present && adev->status.enabled)
> +		acpi_device_fix_up_power(adev);
> +
> +	return 0;
> +}
> +
> +/**
> + * acpi_device_fix_up_power_extended - Force device and its children into D0.
> + * @adev: Parent device object whose power state is to be fixed up.
> + *
> + * Call acpi_device_fix_up_power() for @adev and its children so long as they
> + * are reported as present and enabled.
> + */
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev)
> +{
> +	acpi_device_fix_up_power(adev);
> +	acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
> +}
> +EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
> +
>  int acpi_device_update_power(struct acpi_device *device, int *state_p)
>  {
>  	int state;
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
>  int acpi_device_set_power(struct acpi_device *device, int state);
>  int acpi_bus_init_power(struct acpi_device *device);
>  int acpi_device_fix_up_power(struct acpi_device *device);
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev);
>  int acpi_bus_update_power(acpi_handle handle, int *state_p);
>  int acpi_device_update_power(struct acpi_device *device, int *state_p);
>  bool acpi_bus_power_manageable(acpi_handle handle);
> Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
> +++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
> @@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
>  #ifdef CONFIG_ACPI
>  static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
>  {
> -	struct acpi_device *device, *child;
> +	struct acpi_device *device;
>  
>  	device = ACPI_COMPANION(&slot->chip->pdev->dev);
> -	if (!device)
> -		return;
> -
> -	acpi_device_fix_up_power(device);
> -	list_for_each_entry(child, &device->children, node)
> -		if (child->status.present && child->status.enabled)
> -			acpi_device_fix_up_power(child);
> +	if (device)
> +		acpi_device_fix_up_power_extended(device);
>  }
>  #else
>  static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}
> 
> 
> 

Re: [PATCH v1 15/16] ACPI / MMC: PM: Unify fixing up device power

[Ulf Hansson]

On Thu, 9 Jun 2022 at 16:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Introduce acpi_device_fix_up_power_extended() for fixing up power of
> a device having an ACPI companion in a manner that takes the device's
> children into account and make the MMC code use it in two places
> instead of walking the list of the device ACPI companion's children
> directly.
>
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rafael, feel free to pick this via your tree.

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
>  drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
>  drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
>  include/acpi/acpi_bus.h           |    1 +
>  4 files changed, 28 insertions(+), 13 deletions(-)
>
> Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
> +++ linux-pm/drivers/mmc/host/sdhci-acpi.c
> @@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
>  {
>         struct device *dev = &pdev->dev;
>         const struct sdhci_acpi_slot *slot;
> -       struct acpi_device *device, *child;
>         const struct dmi_system_id *id;
> +       struct acpi_device *device;
>         struct sdhci_acpi_host *c;
>         struct sdhci_host *host;
>         struct resource *iomem;
> @@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
>         slot = sdhci_acpi_get_slot(device);
>
>         /* Power on the SDHCI controller and its children */
> -       acpi_device_fix_up_power(device);
> -       list_for_each_entry(child, &device->children, node)
> -               if (child->status.present && child->status.enabled)
> -                       acpi_device_fix_up_power(child);
> +       acpi_device_fix_up_power_extended(device);
>
>         if (sdhci_acpi_byt_defer(dev))
>                 return -EPROBE_DEFER;
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
>  }
>  EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
>
> +static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
> +{
> +       if (adev->status.present && adev->status.enabled)
> +               acpi_device_fix_up_power(adev);
> +
> +       return 0;
> +}
> +
> +/**
> + * acpi_device_fix_up_power_extended - Force device and its children into D0.
> + * @adev: Parent device object whose power state is to be fixed up.
> + *
> + * Call acpi_device_fix_up_power() for @adev and its children so long as they
> + * are reported as present and enabled.
> + */
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev)
> +{
> +       acpi_device_fix_up_power(adev);
> +       acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
> +}
> +EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
> +
>  int acpi_device_update_power(struct acpi_device *device, int *state_p)
>  {
>         int state;
> Index: linux-pm/include/acpi/acpi_bus.h
> ===================================================================
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
>  int acpi_device_set_power(struct acpi_device *device, int state);
>  int acpi_bus_init_power(struct acpi_device *device);
>  int acpi_device_fix_up_power(struct acpi_device *device);
> +void acpi_device_fix_up_power_extended(struct acpi_device *adev);
>  int acpi_bus_update_power(acpi_handle handle, int *state_p);
>  int acpi_device_update_power(struct acpi_device *device, int *state_p);
>  bool acpi_bus_power_manageable(acpi_handle handle);
> Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
> +++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
> @@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
>  #ifdef CONFIG_ACPI
>  static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
>  {
> -       struct acpi_device *device, *child;
> +       struct acpi_device *device;
>
>         device = ACPI_COMPANION(&slot->chip->pdev->dev);
> -       if (!device)
> -               return;
> -
> -       acpi_device_fix_up_power(device);
> -       list_for_each_entry(child, &device->children, node)
> -               if (child->status.present && child->status.enabled)
> -                       acpi_device_fix_up_power(child);
> +       if (device)
> +               acpi_device_fix_up_power_extended(device);
>  }
>  #else
>  static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}
>
>
>

[PATCH v2 15/16] ACPI / MMC: PM: Unify fixing up device power

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Introduce acpi_device_fix_up_power_extended() for fixing up power of
a device having an ACPI companion in a manner that takes the device's
children into account and make the MMC code use it in two places
instead of walking the list of the device ACPI companion's children
directly.

This will help to eliminate the children list head from struct
acpi_device as it is redundant and it is used in questionable ways
in some places (in particular, locking is needed for walking the
list pointed to it safely, but it is often missing).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---

v1 -> v2:
   * Add R-by from Andy and ACKs from Adrian and Ulf.

---
 drivers/acpi/device_pm.c          |   22 ++++++++++++++++++++++
 drivers/mmc/host/sdhci-acpi.c     |    7 ++-----
 drivers/mmc/host/sdhci-pci-core.c |   11 +++--------
 include/acpi/acpi_bus.h           |    1 +
 4 files changed, 28 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/mmc/host/sdhci-acpi.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-acpi.c
+++ linux-pm/drivers/mmc/host/sdhci-acpi.c
@@ -775,8 +775,8 @@ static int sdhci_acpi_probe(struct platf
 {
 	struct device *dev = &pdev->dev;
 	const struct sdhci_acpi_slot *slot;
-	struct acpi_device *device, *child;
 	const struct dmi_system_id *id;
+	struct acpi_device *device;
 	struct sdhci_acpi_host *c;
 	struct sdhci_host *host;
 	struct resource *iomem;
@@ -796,10 +796,7 @@ static int sdhci_acpi_probe(struct platf
 	slot = sdhci_acpi_get_slot(device);
 
 	/* Power on the SDHCI controller and its children */
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	acpi_device_fix_up_power_extended(device);
 
 	if (sdhci_acpi_byt_defer(dev))
 		return -EPROBE_DEFER;
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -369,6 +369,28 @@ int acpi_device_fix_up_power(struct acpi
 }
 EXPORT_SYMBOL_GPL(acpi_device_fix_up_power);
 
+static int fix_up_power_if_applicable(struct acpi_device *adev, void *not_used)
+{
+	if (adev->status.present && adev->status.enabled)
+		acpi_device_fix_up_power(adev);
+
+	return 0;
+}
+
+/**
+ * acpi_device_fix_up_power_extended - Force device and its children into D0.
+ * @adev: Parent device object whose power state is to be fixed up.
+ *
+ * Call acpi_device_fix_up_power() for @adev and its children so long as they
+ * are reported as present and enabled.
+ */
+void acpi_device_fix_up_power_extended(struct acpi_device *adev)
+{
+	acpi_device_fix_up_power(adev);
+	acpi_dev_for_each_child(adev, fix_up_power_if_applicable, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_device_fix_up_power_extended);
+
 int acpi_device_update_power(struct acpi_device *device, int *state_p)
 {
 	int state;
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -524,6 +524,7 @@ const char *acpi_power_state_string(int
 int acpi_device_set_power(struct acpi_device *device, int state);
 int acpi_bus_init_power(struct acpi_device *device);
 int acpi_device_fix_up_power(struct acpi_device *device);
+void acpi_device_fix_up_power_extended(struct acpi_device *adev);
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
 int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
Index: linux-pm/drivers/mmc/host/sdhci-pci-core.c
===================================================================
--- linux-pm.orig/drivers/mmc/host/sdhci-pci-core.c
+++ linux-pm/drivers/mmc/host/sdhci-pci-core.c
@@ -1240,16 +1240,11 @@ static const struct sdhci_pci_fixes sdhc
 #ifdef CONFIG_ACPI
 static void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot)
 {
-	struct acpi_device *device, *child;
+	struct acpi_device *device;
 
 	device = ACPI_COMPANION(&slot->chip->pdev->dev);
-	if (!device)
-		return;
-
-	acpi_device_fix_up_power(device);
-	list_for_each_entry(child, &device->children, node)
-		if (child->status.present && child->status.enabled)
-			acpi_device_fix_up_power(child);
+	if (device)
+		acpi_device_fix_up_power_extended(device);
 }
 #else
 static inline void intel_mrfld_mmc_fix_up_power_slot(struct sdhci_pci_slot *slot) {}