[PATCH v2 0/7] PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code

[PATCH v2 0/7] PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code

[Rafael J. Wysocki]

Hi All,

As explained in the changelog of patch [2/7], using struct pci_platform_pm_ops
for ACPI is not particularly beneficial, so it is better to get rid of it and
call the functions pointed to by it directly from the PCI core.

However, struct pci_platform_pm_ops is also used by the Intel MID support code,
but it is actually better to call the MID PM function directly from the PCI
core either, which is done in patch [1/7].

After these changes, patch [3/7] removes struct pci_platform_pm_ops and the
rest is just cleanups and some code consolidation on top of that.

Thanks!

[PATCH v2 1/7] PCI: PM: Do not use pci_platform_pm_ops for Intel MID PM

[Rafael J. Wysocki]

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

There are only two users of struct pci_platform_pm_ops in the tree,
one of which is Intel MID PM and the other one is ACPI.  They are
mutually exclusive and the MID PM should take precedence when they
both are enabled, but whether or not this really is the case hinges
on the specific ordering of arch_initcall() calls made by them.

The struct pci_platform_pm_ops abstraction is not really necessary
for just these two users, but it adds complexity and overhead because
of retoplines involved in using all of the function pointers in there.
It also makes following the code a bit more difficult than it would
be otherwise.

Moreover, Intel MID PCI PM doesn't even implement the majority of the
function pointers in struct pci_platform_pm_ops in a meaningful way,
so switch over the PCI core to calling the relevant MID PM routines,
mid_pci_set_power_state() and mid_pci_set_power_state(), directly as
needed and drop mid_pci_platform_pm.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Instead of dropping the MID PCI PM completely, make the PCI core
     call a few functions from there directly instead of using struct
     pci_platform_pm_ops (Andy and Ferry).

---
 drivers/pci/pci-mid.c |   37 ++++++++-----------------------------
 drivers/pci/pci.c     |   23 ++++++++++++++++++++++-
 drivers/pci/pci.h     |   19 +++++++++++++++++++
 3 files changed, 49 insertions(+), 30 deletions(-)

Index: linux-pm/drivers/pci/pci-mid.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-mid.c
+++ linux-pm/drivers/pci/pci-mid.c
@@ -16,45 +16,23 @@
 
 #include "pci.h"
 
-static bool mid_pci_power_manageable(struct pci_dev *dev)
+static bool pci_mid_pm_enabled __read_mostly;
+
+bool pci_use_mid_pm(void)
 {
-	return true;
+	return pci_mid_pm_enabled;
 }
 
-static int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
+int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
 {
 	return intel_mid_pci_set_power_state(pdev, state);
 }
 
-static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
+pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 {
 	return intel_mid_pci_get_power_state(pdev);
 }
 
-static pci_power_t mid_pci_choose_state(struct pci_dev *pdev)
-{
-	return PCI_D3hot;
-}
-
-static int mid_pci_wakeup(struct pci_dev *dev, bool enable)
-{
-	return 0;
-}
-
-static bool mid_pci_need_resume(struct pci_dev *dev)
-{
-	return false;
-}
-
-static const struct pci_platform_pm_ops mid_pci_platform_pm = {
-	.is_manageable	= mid_pci_power_manageable,
-	.set_state	= mid_pci_set_power_state,
-	.get_state	= mid_pci_get_power_state,
-	.choose_state	= mid_pci_choose_state,
-	.set_wakeup	= mid_pci_wakeup,
-	.need_resume	= mid_pci_need_resume,
-};
-
 /*
  * This table should be in sync with the one in
  * arch/x86/platform/intel-mid/pwr.c.
@@ -71,7 +49,8 @@ static int __init mid_pci_init(void)
 
 	id = x86_match_cpu(lpss_cpu_ids);
 	if (id)
-		pci_set_platform_pm(&mid_pci_platform_pm);
+		pci_mid_pm_enabled = true;
+
 	return 0;
 }
 arch_initcall(mid_pci_init);
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -744,4 +744,23 @@ extern const struct attribute_group aspm
 
 extern const struct attribute_group pci_dev_reset_method_attr_group;
 
+#ifdef CONFIG_X86_INTEL_MID
+bool pci_use_mid_pm(void);
+int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state);
+pci_power_t mid_pci_get_power_state(struct pci_dev *pdev);
+#else
+static inline bool pci_use_mid_pm(void)
+{
+	return false;
+}
+static inline int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
+{
+	return -ENODEV;
+}
+static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
+{
+	return PCI_UNKNOWN;
+}
+#endif
+
 #endif /* DRIVERS_PCI_H */
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -985,45 +985,66 @@ int pci_set_platform_pm(const struct pci
 
 static inline bool platform_pci_power_manageable(struct pci_dev *dev)
 {
+	if (pci_use_mid_pm())
+		return true;
+
 	return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false;
 }
 
 static inline int platform_pci_set_power_state(struct pci_dev *dev,
 					       pci_power_t t)
 {
+	if (pci_use_mid_pm())
+		return mid_pci_set_power_state(dev, t);
+
 	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
 }
 
 static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
 {
+	if (pci_use_mid_pm())
+		return mid_pci_get_power_state(dev);
+
 	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
 }
 
 static inline void platform_pci_refresh_power_state(struct pci_dev *dev)
 {
-	if (pci_platform_pm && pci_platform_pm->refresh_state)
+	if (!pci_use_mid_pm() && pci_platform_pm && pci_platform_pm->refresh_state)
 		pci_platform_pm->refresh_state(dev);
 }
 
 static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
 {
+	if (pci_use_mid_pm())
+		return PCI_POWER_ERROR;
+
 	return pci_platform_pm ?
 			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
 }
 
 static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
 {
+	if (pci_use_mid_pm())
+		return PCI_POWER_ERROR;
+
 	return pci_platform_pm ?
 			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
 }
 
 static inline bool platform_pci_need_resume(struct pci_dev *dev)
 {
+	if (pci_use_mid_pm())
+		return false;
+
 	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
 }
 
 static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
 {
+	if (pci_use_mid_pm())
+		return false;
+
 	if (pci_platform_pm && pci_platform_pm->bridge_d3)
 		return pci_platform_pm->bridge_d3(dev);
 	return false;

[PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

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

Using struct pci_platform_pm_ops for ACPI adds unnecessary
indirection to the interactions between the PCI core and ACPI PM,
which is also subject to retpolines.

Moreover, it is not particularly clear from the current code that,
as far as PCI PM is concerned, "platform" really means just ACPI
except for the special casess when Intel MID PCI PM is used or when
ACPI support is disabled (through the kernel config or command line,
or because there are no usable ACPI tables on the system).

To address the above, rework the PCI PM code to invoke ACPI PM
functions directly as needed and drop the acpi_pci_platform_pm
object that is not necessary any more.

Accordingly, update some of the ACPI PM functions in question to do
extra checks in case the ACPI support is disabled (which previously
was taken care of by avoiding to set the pci_platform_ops pointer
in those cases).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
    * Rebase on top of the new [1/7] and move dropping struct
      pci_platform_pm_ops to a separate patch.

---
 drivers/pci/pci-acpi.c |   42 ++++++++++++++++++++----------------------
 drivers/pci/pci.c      |   22 +++++++++-------------
 drivers/pci/pci.h      |   38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 66 insertions(+), 36 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -906,10 +906,13 @@ acpi_status pci_acpi_add_pm_notifier(str
  *	choose highest power _SxD or any lower power
  */
 
-static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
+pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 {
 	int acpi_state, d_max;
 
+	if (acpi_pci_disabled)
+		return PCI_POWER_ERROR;
+
 	if (pdev->no_d3cold)
 		d_max = ACPI_STATE_D3_HOT;
 	else
@@ -965,7 +968,7 @@ int pci_dev_acpi_reset(struct pci_dev *d
 	return 0;
 }
 
-static bool acpi_pci_power_manageable(struct pci_dev *dev)
+bool acpi_pci_power_manageable(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 
@@ -974,13 +977,13 @@ static bool acpi_pci_power_manageable(st
 	return acpi_device_power_manageable(adev);
 }
 
-static bool acpi_pci_bridge_d3(struct pci_dev *dev)
+bool acpi_pci_bridge_d3(struct pci_dev *dev)
 {
 	const union acpi_object *obj;
 	struct acpi_device *adev;
 	struct pci_dev *rpdev;
 
-	if (!dev->is_hotplug_bridge)
+	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
 		return false;
 
 	/* Assume D3 support if the bridge is power-manageable by ACPI. */
@@ -1008,7 +1011,7 @@ static bool acpi_pci_bridge_d3(struct pc
 	return obj->integer.value == 1;
 }
 
-static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
+int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 	static const u8 state_conv[] = {
@@ -1046,7 +1049,7 @@ static int acpi_pci_set_power_state(stru
 	return error;
 }
 
-static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
+pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 	static const pci_power_t state_conv[] = {
@@ -1068,7 +1071,7 @@ static pci_power_t acpi_pci_get_power_st
 	return state_conv[state];
 }
 
-static void acpi_pci_refresh_power_state(struct pci_dev *dev)
+void acpi_pci_refresh_power_state(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 
@@ -1093,17 +1096,23 @@ static int acpi_pci_propagate_wakeup(str
 	return 0;
 }
 
-static int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
+int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
 {
+	if (acpi_pci_disabled)
+		return 0;
+
 	if (acpi_pm_device_can_wakeup(&dev->dev))
 		return acpi_pm_set_device_wakeup(&dev->dev, enable);
 
 	return acpi_pci_propagate_wakeup(dev->bus, enable);
 }
 
-static bool acpi_pci_need_resume(struct pci_dev *dev)
+bool acpi_pci_need_resume(struct pci_dev *dev)
 {
-	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
+	struct acpi_device *adev;
+
+	if (acpi_pci_disabled)
+		return false;
 
 	/*
 	 * In some cases (eg. Samsung 305V4A) leaving a bridge in suspend over
@@ -1118,6 +1127,7 @@ static bool acpi_pci_need_resume(struct
 	if (!adev || !acpi_device_power_manageable(adev))
 		return false;
 
+	adev = ACPI_COMPANION(&dev->dev);
 	if (adev->wakeup.flags.valid &&
 	    device_may_wakeup(&dev->dev) != !!adev->wakeup.prepare_count)
 		return true;
@@ -1128,17 +1138,6 @@ static bool acpi_pci_need_resume(struct
 	return !!adev->power.flags.dsw_present;
 }
 
-static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
-	.bridge_d3 = acpi_pci_bridge_d3,
-	.is_manageable = acpi_pci_power_manageable,
-	.set_state = acpi_pci_set_power_state,
-	.get_state = acpi_pci_get_power_state,
-	.refresh_state = acpi_pci_refresh_power_state,
-	.choose_state = acpi_pci_choose_state,
-	.set_wakeup = acpi_pci_wakeup,
-	.need_resume = acpi_pci_need_resume,
-};
-
 void acpi_pci_add_bus(struct pci_bus *bus)
 {
 	union acpi_object *obj;
@@ -1448,7 +1447,6 @@ static int __init acpi_pci_init(void)
 	if (acpi_pci_disabled)
 		return 0;
 
-	pci_set_platform_pm(&acpi_pci_platform_pm);
 	acpi_pci_slot_init();
 	acpiphp_init();
 
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -725,17 +725,53 @@ int pci_acpi_program_hp_params(struct pc
 extern const struct attribute_group pci_dev_acpi_attr_group;
 void pci_set_acpi_fwnode(struct pci_dev *dev);
 int pci_dev_acpi_reset(struct pci_dev *dev, bool probe);
+bool acpi_pci_power_manageable(struct pci_dev *dev);
+bool acpi_pci_bridge_d3(struct pci_dev *dev);
+int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state);
+pci_power_t acpi_pci_get_power_state(struct pci_dev *dev);
+void acpi_pci_refresh_power_state(struct pci_dev *dev);
+int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
+bool acpi_pci_need_resume(struct pci_dev *dev);
+pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
 #else
 static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
 {
 	return -ENOTTY;
 }
-
 static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {}
 static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
 {
 	return -ENODEV;
 }
+static inline bool acpi_pci_power_manageable(struct pci_dev *dev)
+{
+	return false;
+}
+static inline bool acpi_pci_bridge_d3(struct pci_dev *dev)
+{
+	return false;
+}
+static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
+{
+	return -ENODEV;
+}
+static inline pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
+{
+	return PCI_UNKNOWN;
+}
+static inline void acpi_pci_refresh_power_state(struct pci_dev *dev) {}
+static inline int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
+{
+	return -ENODEV;
+}
+static inline bool acpi_pci_need_resume(struct pci_dev *dev)
+{
+	return false;
+}
+static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
+{
+	return PCI_POWER_ERROR;
+}
 #endif
 
 #ifdef CONFIG_PCIEASPM
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -988,7 +988,7 @@ static inline bool platform_pci_power_ma
 	if (pci_use_mid_pm())
 		return true;
 
-	return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false;
+	return acpi_pci_power_manageable(dev);
 }
 
 static inline int platform_pci_set_power_state(struct pci_dev *dev,
@@ -997,7 +997,7 @@ static inline int platform_pci_set_power
 	if (pci_use_mid_pm())
 		return mid_pci_set_power_state(dev, t);
 
-	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
+	return acpi_pci_set_power_state(dev, t);
 }
 
 static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
@@ -1005,13 +1005,13 @@ static inline pci_power_t platform_pci_g
 	if (pci_use_mid_pm())
 		return mid_pci_get_power_state(dev);
 
-	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
+	return acpi_pci_get_power_state(dev);
 }
 
 static inline void platform_pci_refresh_power_state(struct pci_dev *dev)
 {
-	if (!pci_use_mid_pm() && pci_platform_pm && pci_platform_pm->refresh_state)
-		pci_platform_pm->refresh_state(dev);
+	if (!pci_use_mid_pm())
+		acpi_pci_refresh_power_state(dev);
 }
 
 static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
@@ -1019,8 +1019,7 @@ static inline pci_power_t platform_pci_c
 	if (pci_use_mid_pm())
 		return PCI_POWER_ERROR;
 
-	return pci_platform_pm ?
-			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
+	return acpi_pci_choose_state(dev);
 }
 
 static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
@@ -1028,8 +1027,7 @@ static inline int platform_pci_set_wakeu
 	if (pci_use_mid_pm())
 		return PCI_POWER_ERROR;
 
-	return pci_platform_pm ?
-			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
+	return acpi_pci_wakeup(dev, enable);
 }
 
 static inline bool platform_pci_need_resume(struct pci_dev *dev)
@@ -1037,7 +1035,7 @@ static inline bool platform_pci_need_res
 	if (pci_use_mid_pm())
 		return false;
 
-	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
+	return acpi_pci_need_resume(dev);
 }
 
 static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
@@ -1045,9 +1043,7 @@ static inline bool platform_pci_bridge_d
 	if (pci_use_mid_pm())
 		return false;
 
-	if (pci_platform_pm && pci_platform_pm->bridge_d3)
-		return pci_platform_pm->bridge_d3(dev);
-	return false;
+	return acpi_pci_bridge_d3(dev);
 }
 
 /**

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Ferry Toth]

Hi,
Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Using struct pci_platform_pm_ops for ACPI adds unnecessary
> indirection to the interactions between the PCI core and ACPI PM,
> which is also subject to retpolines.
> 
> Moreover, it is not particularly clear from the current code that,
> as far as PCI PM is concerned, "platform" really means just ACPI
> except for the special casess when Intel MID PCI PM is used or when
> ACPI support is disabled (through the kernel config or command line,
> or because there are no usable ACPI tables on the system).
> 
> To address the above, rework the PCI PM code to invoke ACPI PM
> functions directly as needed and drop the acpi_pci_platform_pm
> object that is not necessary any more.
> 
> Accordingly, update some of the ACPI PM functions in question to do
> extra checks in case the ACPI support is disabled (which previously
> was taken care of by avoiding to set the pci_platform_ops pointer
> in those cases).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2:
>      * Rebase on top of the new [1/7] and move dropping struct
>        pci_platform_pm_ops to a separate patch.

I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't 
apply (after 1/7 applied). Should I apply this on another tree?

> ---
>   drivers/pci/pci-acpi.c |   42 ++++++++++++++++++++----------------------
>   drivers/pci/pci.c      |   22 +++++++++-------------
>   drivers/pci/pci.h      |   38 +++++++++++++++++++++++++++++++++++++-
>   3 files changed, 66 insertions(+), 36 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -906,10 +906,13 @@ acpi_status pci_acpi_add_pm_notifier(str
>    *	choose highest power _SxD or any lower power
>    */
>   
> -static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> +pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>   {
>   	int acpi_state, d_max;
>   
> +	if (acpi_pci_disabled)
> +		return PCI_POWER_ERROR;
> +
>   	if (pdev->no_d3cold)
>   		d_max = ACPI_STATE_D3_HOT;
>   	else
> @@ -965,7 +968,7 @@ int pci_dev_acpi_reset(struct pci_dev *d
>   	return 0;
>   }
>   
> -static bool acpi_pci_power_manageable(struct pci_dev *dev)
> +bool acpi_pci_power_manageable(struct pci_dev *dev)
>   {
>   	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
>   
> @@ -974,13 +977,13 @@ static bool acpi_pci_power_manageable(st
>   	return acpi_device_power_manageable(adev);
>   }
>   
> -static bool acpi_pci_bridge_d3(struct pci_dev *dev)
> +bool acpi_pci_bridge_d3(struct pci_dev *dev)
>   {
>   	const union acpi_object *obj;
>   	struct acpi_device *adev;
>   	struct pci_dev *rpdev;
>   
> -	if (!dev->is_hotplug_bridge)
> +	if (acpi_pci_disabled || !dev->is_hotplug_bridge)
>   		return false;
>   
>   	/* Assume D3 support if the bridge is power-manageable by ACPI. */
> @@ -1008,7 +1011,7 @@ static bool acpi_pci_bridge_d3(struct pc
>   	return obj->integer.value == 1;
>   }
>   
> -static int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> +int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>   {
>   	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
>   	static const u8 state_conv[] = {
> @@ -1046,7 +1049,7 @@ static int acpi_pci_set_power_state(stru
>   	return error;
>   }
>   
> -static pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
> +pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
>   {
>   	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
>   	static const pci_power_t state_conv[] = {
> @@ -1068,7 +1071,7 @@ static pci_power_t acpi_pci_get_power_st
>   	return state_conv[state];
>   }
>   
> -static void acpi_pci_refresh_power_state(struct pci_dev *dev)
> +void acpi_pci_refresh_power_state(struct pci_dev *dev)
>   {
>   	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
>   
> @@ -1093,17 +1096,23 @@ static int acpi_pci_propagate_wakeup(str
>   	return 0;
>   }
>   
> -static int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
> +int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
>   {
> +	if (acpi_pci_disabled)
> +		return 0;
> +
>   	if (acpi_pm_device_can_wakeup(&dev->dev))
>   		return acpi_pm_set_device_wakeup(&dev->dev, enable);
>   
>   	return acpi_pci_propagate_wakeup(dev->bus, enable);
>   }
>   
> -static bool acpi_pci_need_resume(struct pci_dev *dev)
> +bool acpi_pci_need_resume(struct pci_dev *dev)
>   {
> -	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> +	struct acpi_device *adev;
> +
> +	if (acpi_pci_disabled)
> +		return false;
>   
>   	/*
>   	 * In some cases (eg. Samsung 305V4A) leaving a bridge in suspend over
> @@ -1118,6 +1127,7 @@ static bool acpi_pci_need_resume(struct
>   	if (!adev || !acpi_device_power_manageable(adev))
>   		return false;
>   
> +	adev = ACPI_COMPANION(&dev->dev);
>   	if (adev->wakeup.flags.valid &&
>   	    device_may_wakeup(&dev->dev) != !!adev->wakeup.prepare_count)
>   		return true;
> @@ -1128,17 +1138,6 @@ static bool acpi_pci_need_resume(struct
>   	return !!adev->power.flags.dsw_present;
>   }
>   
> -static const struct pci_platform_pm_ops acpi_pci_platform_pm = {
> -	.bridge_d3 = acpi_pci_bridge_d3,
> -	.is_manageable = acpi_pci_power_manageable,
> -	.set_state = acpi_pci_set_power_state,
> -	.get_state = acpi_pci_get_power_state,
> -	.refresh_state = acpi_pci_refresh_power_state,
> -	.choose_state = acpi_pci_choose_state,
> -	.set_wakeup = acpi_pci_wakeup,
> -	.need_resume = acpi_pci_need_resume,
> -};
> -
>   void acpi_pci_add_bus(struct pci_bus *bus)
>   {
>   	union acpi_object *obj;
> @@ -1448,7 +1447,6 @@ static int __init acpi_pci_init(void)
>   	if (acpi_pci_disabled)
>   		return 0;
>   
> -	pci_set_platform_pm(&acpi_pci_platform_pm);
>   	acpi_pci_slot_init();
>   	acpiphp_init();
>   
> Index: linux-pm/drivers/pci/pci.h
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.h
> +++ linux-pm/drivers/pci/pci.h
> @@ -725,17 +725,53 @@ int pci_acpi_program_hp_params(struct pc
>   extern const struct attribute_group pci_dev_acpi_attr_group;
>   void pci_set_acpi_fwnode(struct pci_dev *dev);
>   int pci_dev_acpi_reset(struct pci_dev *dev, bool probe);
> +bool acpi_pci_power_manageable(struct pci_dev *dev);
> +bool acpi_pci_bridge_d3(struct pci_dev *dev);
> +int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state);
> +pci_power_t acpi_pci_get_power_state(struct pci_dev *dev);
> +void acpi_pci_refresh_power_state(struct pci_dev *dev);
> +int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
> +bool acpi_pci_need_resume(struct pci_dev *dev);
> +pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
>   #else
>   static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
>   {
>   	return -ENOTTY;
>   }
> -
>   static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {}
>   static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
>   {
>   	return -ENODEV;
>   }
> +static inline bool acpi_pci_power_manageable(struct pci_dev *dev)
> +{
> +	return false;
> +}
> +static inline bool acpi_pci_bridge_d3(struct pci_dev *dev)
> +{
> +	return false;
> +}
> +static inline int acpi_pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> +{
> +	return -ENODEV;
> +}
> +static inline pci_power_t acpi_pci_get_power_state(struct pci_dev *dev)
> +{
> +	return PCI_UNKNOWN;
> +}
> +static inline void acpi_pci_refresh_power_state(struct pci_dev *dev) {}
> +static inline int acpi_pci_wakeup(struct pci_dev *dev, bool enable)
> +{
> +	return -ENODEV;
> +}
> +static inline bool acpi_pci_need_resume(struct pci_dev *dev)
> +{
> +	return false;
> +}
> +static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
> +{
> +	return PCI_POWER_ERROR;
> +}
>   #endif
>   
>   #ifdef CONFIG_PCIEASPM
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -988,7 +988,7 @@ static inline bool platform_pci_power_ma
>   	if (pci_use_mid_pm())
>   		return true;
>   
> -	return pci_platform_pm ? pci_platform_pm->is_manageable(dev) : false;
> +	return acpi_pci_power_manageable(dev);
>   }
>   
>   static inline int platform_pci_set_power_state(struct pci_dev *dev,
> @@ -997,7 +997,7 @@ static inline int platform_pci_set_power
>   	if (pci_use_mid_pm())
>   		return mid_pci_set_power_state(dev, t);
>   
> -	return pci_platform_pm ? pci_platform_pm->set_state(dev, t) : -ENOSYS;
> +	return acpi_pci_set_power_state(dev, t);
>   }
>   
>   static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
> @@ -1005,13 +1005,13 @@ static inline pci_power_t platform_pci_g
>   	if (pci_use_mid_pm())
>   		return mid_pci_get_power_state(dev);
>   
> -	return pci_platform_pm ? pci_platform_pm->get_state(dev) : PCI_UNKNOWN;
> +	return acpi_pci_get_power_state(dev);
>   }
>   
>   static inline void platform_pci_refresh_power_state(struct pci_dev *dev)
>   {
> -	if (!pci_use_mid_pm() && pci_platform_pm && pci_platform_pm->refresh_state)
> -		pci_platform_pm->refresh_state(dev);
> +	if (!pci_use_mid_pm())
> +		acpi_pci_refresh_power_state(dev);
>   }
>   
>   static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
> @@ -1019,8 +1019,7 @@ static inline pci_power_t platform_pci_c
>   	if (pci_use_mid_pm())
>   		return PCI_POWER_ERROR;
>   
> -	return pci_platform_pm ?
> -			pci_platform_pm->choose_state(dev) : PCI_POWER_ERROR;
> +	return acpi_pci_choose_state(dev);
>   }
>   
>   static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
> @@ -1028,8 +1027,7 @@ static inline int platform_pci_set_wakeu
>   	if (pci_use_mid_pm())
>   		return PCI_POWER_ERROR;
>   
> -	return pci_platform_pm ?
> -			pci_platform_pm->set_wakeup(dev, enable) : -ENODEV;
> +	return acpi_pci_wakeup(dev, enable);
>   }
>   
>   static inline bool platform_pci_need_resume(struct pci_dev *dev)
> @@ -1037,7 +1035,7 @@ static inline bool platform_pci_need_res
>   	if (pci_use_mid_pm())
>   		return false;
>   
> -	return pci_platform_pm ? pci_platform_pm->need_resume(dev) : false;
> +	return acpi_pci_need_resume(dev);
>   }
>   
>   static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> @@ -1045,9 +1043,7 @@ static inline bool platform_pci_bridge_d
>   	if (pci_use_mid_pm())
>   		return false;
>   
> -	if (pci_platform_pm && pci_platform_pm->bridge_d3)
> -		return pci_platform_pm->bridge_d3(dev);
> -	return false;
> +	return acpi_pci_bridge_d3(dev);
>   }
>   
>   /**
> 
> 
> 

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth <fntoth@gmail.com> wrote:
>
> Hi,
> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Using struct pci_platform_pm_ops for ACPI adds unnecessary
> > indirection to the interactions between the PCI core and ACPI PM,
> > which is also subject to retpolines.
> >
> > Moreover, it is not particularly clear from the current code that,
> > as far as PCI PM is concerned, "platform" really means just ACPI
> > except for the special casess when Intel MID PCI PM is used or when
> > ACPI support is disabled (through the kernel config or command line,
> > or because there are no usable ACPI tables on the system).
> >
> > To address the above, rework the PCI PM code to invoke ACPI PM
> > functions directly as needed and drop the acpi_pci_platform_pm
> > object that is not necessary any more.
> >
> > Accordingly, update some of the ACPI PM functions in question to do
> > extra checks in case the ACPI support is disabled (which previously
> > was taken care of by avoiding to set the pci_platform_ops pointer
> > in those cases).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > v1 -> v2:
> >      * Rebase on top of the new [1/7] and move dropping struct
> >        pci_platform_pm_ops to a separate patch.
>
> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
> apply (after 1/7 applied). Should I apply this on another tree?

This is on top of
https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
which is not yet in any tree.

Sorry for the confusion.

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

On Thu, Sep 23, 2021 at 3:26 PM Ferry Toth <fntoth@gmail.com> wrote:
>
> Hi,
>
> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
>
> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth <fntoth@gmail.com> wrote:
>
> Hi,
> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Using struct pci_platform_pm_ops for ACPI adds unnecessary
> indirection to the interactions between the PCI core and ACPI PM,
> which is also subject to retpolines.
>
> Moreover, it is not particularly clear from the current code that,
> as far as PCI PM is concerned, "platform" really means just ACPI
> except for the special casess when Intel MID PCI PM is used or when
> ACPI support is disabled (through the kernel config or command line,
> or because there are no usable ACPI tables on the system).
>
> To address the above, rework the PCI PM code to invoke ACPI PM
> functions directly as needed and drop the acpi_pci_platform_pm
> object that is not necessary any more.
>
> Accordingly, update some of the ACPI PM functions in question to do
> extra checks in case the ACPI support is disabled (which previously
> was taken care of by avoiding to set the pci_platform_ops pointer
> in those cases).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> v1 -> v2:
>      * Rebase on top of the new [1/7] and move dropping struct
>        pci_platform_pm_ops to a separate patch.
>
> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
> apply (after 1/7 applied). Should I apply this on another tree?
>
> This is on top of
> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
> which is not yet in any tree.
>
> Sorry for the confusion.
>
> No problem at all. If I can I will try to report back tonight. Else, will be delayed 2 due to a short break.

Thank you!

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Ferry Toth]

Repost (with formatting removed, sorry for the noise)
Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com>  wrote:
>> Hi,
>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>
>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary
>>> indirection to the interactions between the PCI core and ACPI PM,
>>> which is also subject to retpolines.
>>>
>>> Moreover, it is not particularly clear from the current code that,
>>> as far as PCI PM is concerned, "platform" really means just ACPI
>>> except for the special casess when Intel MID PCI PM is used or when
>>> ACPI support is disabled (through the kernel config or command line,
>>> or because there are no usable ACPI tables on the system).
>>>
>>> To address the above, rework the PCI PM code to invoke ACPI PM
>>> functions directly as needed and drop the acpi_pci_platform_pm
>>> object that is not necessary any more.
>>>
>>> Accordingly, update some of the ACPI PM functions in question to do
>>> extra checks in case the ACPI support is disabled (which previously
>>> was taken care of by avoiding to set the pci_platform_ops pointer
>>> in those cases).
>>>
>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> v1 -> v2:
>>>       * Rebase on top of the new [1/7] and move dropping struct
>>>         pci_platform_pm_ops to a separate patch.
>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
>> apply (after 1/7 applied). Should I apply this on another tree?
> This is on top of
> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
> which is not yet in any tree.
>
> Sorry for the confusion.
No problem at all. If I can I will try to report back tonight. Else, 
will be delayed 2 due to a short break.

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Ferry Toth]

Hi

Op 23-09-2021 om 15:51 schreef Ferry Toth:
> Repost (with formatting removed, sorry for the noise)
> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com>  wrote:
>>> Hi,
>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>>
>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary
>>>> indirection to the interactions between the PCI core and ACPI PM,
>>>> which is also subject to retpolines.
>>>>
>>>> Moreover, it is not particularly clear from the current code that,
>>>> as far as PCI PM is concerned, "platform" really means just ACPI
>>>> except for the special casess when Intel MID PCI PM is used or when
>>>> ACPI support is disabled (through the kernel config or command line,
>>>> or because there are no usable ACPI tables on the system).
>>>>
>>>> To address the above, rework the PCI PM code to invoke ACPI PM
>>>> functions directly as needed and drop the acpi_pci_platform_pm
>>>> object that is not necessary any more.
>>>>
>>>> Accordingly, update some of the ACPI PM functions in question to do
>>>> extra checks in case the ACPI support is disabled (which previously
>>>> was taken care of by avoiding to set the pci_platform_ops pointer
>>>> in those cases).
>>>>
>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>       * Rebase on top of the new [1/7] and move dropping struct
>>>>         pci_platform_pm_ops to a separate patch.
>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
>>> apply (after 1/7 applied). Should I apply this on another tree?
>> This is on top of
>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ 
>>
>> which is not yet in any tree.
>>
>> Sorry for the confusion.
> No problem at all. If I can I will try to report back tonight. Else, 
> will be delayed 2 due to a short break.

With those 3 extra patches followed by 7 from this series it builds. But 
on boot I get:
dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core
Then after this it reboots. Nothing in the logs. Nothing else on 
console, I guess something goes wrong early.

I tried both in host / device mode - no change.

Normally in host mode I have:
usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, 
bcdDevice= 5.14
Only ref to dwc3 is:
tusb1210 dwc3.0.auto.ulpi: using ACPI for GPIO lookup / using lookup 
tables for GPIO lookup / No GPIO consumer cs found

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote:
>
> Hi
>
> Op 23-09-2021 om 15:51 schreef Ferry Toth:
> > Repost (with formatting removed, sorry for the noise)
> > Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
> >> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com>  wrote:
> >>> Hi,
> >>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
> >>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
> >>>>
> >>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary
> >>>> indirection to the interactions between the PCI core and ACPI PM,
> >>>> which is also subject to retpolines.
> >>>>
> >>>> Moreover, it is not particularly clear from the current code that,
> >>>> as far as PCI PM is concerned, "platform" really means just ACPI
> >>>> except for the special casess when Intel MID PCI PM is used or when
> >>>> ACPI support is disabled (through the kernel config or command line,
> >>>> or because there are no usable ACPI tables on the system).
> >>>>
> >>>> To address the above, rework the PCI PM code to invoke ACPI PM
> >>>> functions directly as needed and drop the acpi_pci_platform_pm
> >>>> object that is not necessary any more.
> >>>>
> >>>> Accordingly, update some of the ACPI PM functions in question to do
> >>>> extra checks in case the ACPI support is disabled (which previously
> >>>> was taken care of by avoiding to set the pci_platform_ops pointer
> >>>> in those cases).
> >>>>
> >>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
> >>>> ---
> >>>>
> >>>> v1 -> v2:
> >>>>       * Rebase on top of the new [1/7] and move dropping struct
> >>>>         pci_platform_pm_ops to a separate patch.
> >>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
> >>> apply (after 1/7 applied). Should I apply this on another tree?
> >> This is on top of
> >> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
> >>
> >> which is not yet in any tree.
> >>
> >> Sorry for the confusion.
> > No problem at all. If I can I will try to report back tonight. Else,
> > will be delayed 2 due to a short break.
>
> With those 3 extra patches followed by 7 from this series it builds. But
> on boot I get:
> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core
> Then after this it reboots. Nothing in the logs. Nothing else on
> console, I guess something goes wrong early.

It appears so.

Can you please try just the 3 extra patches this series is on top of?
The problem is more likely to be located in one of them.

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Ferry Toth]

Hi,

Op 24-09-2021 om 14:02 schreef Rafael J. Wysocki:
> On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote:
>> Hi
>>
>> Op 23-09-2021 om 15:51 schreef Ferry Toth:
>>> Repost (with formatting removed, sorry for the noise)
>>> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
>>>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com>  wrote:
>>>>> Hi,
>>>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
>>>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>>>>
>>>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary
>>>>>> indirection to the interactions between the PCI core and ACPI PM,
>>>>>> which is also subject to retpolines.
>>>>>>
>>>>>> Moreover, it is not particularly clear from the current code that,
>>>>>> as far as PCI PM is concerned, "platform" really means just ACPI
>>>>>> except for the special casess when Intel MID PCI PM is used or when
>>>>>> ACPI support is disabled (through the kernel config or command line,
>>>>>> or because there are no usable ACPI tables on the system).
>>>>>>
>>>>>> To address the above, rework the PCI PM code to invoke ACPI PM
>>>>>> functions directly as needed and drop the acpi_pci_platform_pm
>>>>>> object that is not necessary any more.
>>>>>>
>>>>>> Accordingly, update some of the ACPI PM functions in question to do
>>>>>> extra checks in case the ACPI support is disabled (which previously
>>>>>> was taken care of by avoiding to set the pci_platform_ops pointer
>>>>>> in those cases).
>>>>>>
>>>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>>>> ---
>>>>>>
>>>>>> v1 -> v2:
>>>>>>        * Rebase on top of the new [1/7] and move dropping struct
>>>>>>          pci_platform_pm_ops to a separate patch.
>>>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
>>>>> apply (after 1/7 applied). Should I apply this on another tree?
>>>> This is on top of
>>>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
>>>>
>>>> which is not yet in any tree.
>>>>
>>>> Sorry for the confusion.
>>> No problem at all. If I can I will try to report back tonight. Else,
>>> will be delayed 2 due to a short break.
>> With those 3 extra patches followed by 7 from this series it builds. But
>> on boot I get:
>> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core
>> Then after this it reboots. Nothing in the logs. Nothing else on
>> console, I guess something goes wrong early.
> It appears so.
>
> Can you please try just the 3 extra patches this series is on top of?
> The problem is more likely to be located in one of them.
Yes, I hope to be able to that this evening.

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Ferry Toth]

Hi

Op 24-09-2021 om 14:02 schreef Rafael J. Wysocki:
> On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote:
>> Hi
>>
>> Op 23-09-2021 om 15:51 schreef Ferry Toth:
>>> Repost (with formatting removed, sorry for the noise)
>>> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
>>>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com>  wrote:
>>>>> Hi,
>>>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
>>>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>>>>
>>>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary
>>>>>> indirection to the interactions between the PCI core and ACPI PM,
>>>>>> which is also subject to retpolines.
>>>>>>
>>>>>> Moreover, it is not particularly clear from the current code that,
>>>>>> as far as PCI PM is concerned, "platform" really means just ACPI
>>>>>> except for the special casess when Intel MID PCI PM is used or when
>>>>>> ACPI support is disabled (through the kernel config or command line,
>>>>>> or because there are no usable ACPI tables on the system).
>>>>>>
>>>>>> To address the above, rework the PCI PM code to invoke ACPI PM
>>>>>> functions directly as needed and drop the acpi_pci_platform_pm
>>>>>> object that is not necessary any more.
>>>>>>
>>>>>> Accordingly, update some of the ACPI PM functions in question to do
>>>>>> extra checks in case the ACPI support is disabled (which previously
>>>>>> was taken care of by avoiding to set the pci_platform_ops pointer
>>>>>> in those cases).
>>>>>>
>>>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
>>>>>> ---
>>>>>>
>>>>>> v1 -> v2:
>>>>>>        * Rebase on top of the new [1/7] and move dropping struct
>>>>>>          pci_platform_pm_ops to a separate patch.
>>>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
>>>>> apply (after 1/7 applied). Should I apply this on another tree?
>>>> This is on top of
>>>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
>>>>
>>>> which is not yet in any tree.
>>>>
>>>> Sorry for the confusion.
>>> No problem at all. If I can I will try to report back tonight. Else,
>>> will be delayed 2 due to a short break.
>> With those 3 extra patches followed by 7 from this series it builds. But
>> on boot I get:
>> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core
>> Then after this it reboots. Nothing in the logs. Nothing else on
>> console, I guess something goes wrong early.
> It appears so.
>
> Can you please try just the 3 extra patches this series is on top of?
> The problem is more likely to be located in one of them.
Boots fine with just the 3 so up to and including "ACPI: glue: Look for 
ACPI bus type only if ACPI companion is not known". From the log I get:


Intel MID platform detected, using MID PCI ops
PCI: Using configuration type 1 for base access
..
PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" 
and report a bug
..
PCI: Probing PCI hardware
PCI: root bus 00: using default resources
PCI: Probing PCI hardware (bus 00)
PCI: pci_cache_line_size set to 64 bytes
..
pnp: PnP ACPI init
..
pnp: PnP ACPI: found 2 devices
..
xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 1
xhci-hcd xhci-hcd.1.auto: hcc params 0x0220f06c hci version 0x100 quirks 
0x0000000002010010
xhci-hcd xhci-hcd.1.auto: irq 14, io mem 0xf9100000
usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, 
bcdDevice= 5.15
usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: xHCI Host Controller
usb usb1: Manufacturer: Linux 5.15.0-rc2-edison-acpi-standard xhci-hcd
usb usb1: SerialNumber: xhci-hcd.1.auto
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 1 port detected

I continued up to "PCI: ACPI: PM: Do not use pci_platform_pm_ops for 
ACPI", still boots.

In the logs I still see "Intel MID platform detected, using MID PCI ops".

Unfortunately no more time today, and tomorrow short holiday starts. I 
will continue after returning next Sat.

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

On Fri, Sep 24, 2021 at 11:17 PM Ferry Toth <fntoth@gmail.com> wrote:
>
> Hi
>
> Op 24-09-2021 om 14:02 schreef Rafael J. Wysocki:
> > On Thu, Sep 23, 2021 at 10:32 PM Ferry Toth <fntoth@gmail.com> wrote:
> >> Hi
> >>
> >> Op 23-09-2021 om 15:51 schreef Ferry Toth:
> >>> Repost (with formatting removed, sorry for the noise)
> >>> Op 23-09-2021 om 13:30 schreef Rafael J. Wysocki:
> >>>> On Wed, Sep 22, 2021 at 11:31 PM Ferry Toth<fntoth@gmail.com>  wrote:
> >>>>> Hi,
> >>>>> Op 20-09-2021 om 21:17 schreef Rafael J. Wysocki:
> >>>>>> From: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
> >>>>>>
> >>>>>> Using struct pci_platform_pm_ops for ACPI adds unnecessary
> >>>>>> indirection to the interactions between the PCI core and ACPI PM,
> >>>>>> which is also subject to retpolines.
> >>>>>>
> >>>>>> Moreover, it is not particularly clear from the current code that,
> >>>>>> as far as PCI PM is concerned, "platform" really means just ACPI
> >>>>>> except for the special casess when Intel MID PCI PM is used or when
> >>>>>> ACPI support is disabled (through the kernel config or command line,
> >>>>>> or because there are no usable ACPI tables on the system).
> >>>>>>
> >>>>>> To address the above, rework the PCI PM code to invoke ACPI PM
> >>>>>> functions directly as needed and drop the acpi_pci_platform_pm
> >>>>>> object that is not necessary any more.
> >>>>>>
> >>>>>> Accordingly, update some of the ACPI PM functions in question to do
> >>>>>> extra checks in case the ACPI support is disabled (which previously
> >>>>>> was taken care of by avoiding to set the pci_platform_ops pointer
> >>>>>> in those cases).
> >>>>>>
> >>>>>> Signed-off-by: Rafael J. Wysocki<rafael.j.wysocki@intel.com>
> >>>>>> ---
> >>>>>>
> >>>>>> v1 -> v2:
> >>>>>>        * Rebase on top of the new [1/7] and move dropping struct
> >>>>>>          pci_platform_pm_ops to a separate patch.
> >>>>> I wanted to test this series on 5.15-rc2 but this patch 2/7 doesn't
> >>>>> apply (after 1/7 applied). Should I apply this on another tree?
> >>>> This is on top of
> >>>> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
> >>>>
> >>>> which is not yet in any tree.
> >>>>
> >>>> Sorry for the confusion.
> >>> No problem at all. If I can I will try to report back tonight. Else,
> >>> will be delayed 2 due to a short break.
> >> With those 3 extra patches followed by 7 from this series it builds. But
> >> on boot I get:
> >> dwc3 dwc3.0.auto: this is not a DesignWare USB3 DRD Core
> >> Then after this it reboots. Nothing in the logs. Nothing else on
> >> console, I guess something goes wrong early.
> > It appears so.
> >
> > Can you please try just the 3 extra patches this series is on top of?
> > The problem is more likely to be located in one of them.
> Boots fine with just the 3 so up to and including "ACPI: glue: Look for
> ACPI bus type only if ACPI companion is not known". From the log I get:
>
>
> Intel MID platform detected, using MID PCI ops
> PCI: Using configuration type 1 for base access
> ..
> PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs"
> and report a bug
> ..
> PCI: Probing PCI hardware
> PCI: root bus 00: using default resources
> PCI: Probing PCI hardware (bus 00)
> PCI: pci_cache_line_size set to 64 bytes
> ..
> pnp: PnP ACPI init
> ..
> pnp: PnP ACPI: found 2 devices
> ..
> xhci-hcd xhci-hcd.1.auto: xHCI Host Controller
> xhci-hcd xhci-hcd.1.auto: new USB bus registered, assigned bus number 1
> xhci-hcd xhci-hcd.1.auto: hcc params 0x0220f06c hci version 0x100 quirks
> 0x0000000002010010
> xhci-hcd xhci-hcd.1.auto: irq 14, io mem 0xf9100000
> usb usb1: New USB device found, idVendor=1d6b, idProduct=0002,
> bcdDevice= 5.15
> usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
> usb usb1: Product: xHCI Host Controller
> usb usb1: Manufacturer: Linux 5.15.0-rc2-edison-acpi-standard xhci-hcd
> usb usb1: SerialNumber: xhci-hcd.1.auto
> hub 1-0:1.0: USB hub found
> hub 1-0:1.0: 1 port detected
>
> I continued up to "PCI: ACPI: PM: Do not use pci_platform_pm_ops for
> ACPI", still boots.
>
> In the logs I still see "Intel MID platform detected, using MID PCI ops".
>
> Unfortunately no more time today, and tomorrow short holiday starts. I
> will continue after returning next Sat.

Thanks for the testing and feedback, much appreciated!

I'm going to queue up the patches that you have tested with a
Tested-by tag from you if that's not an issue.

Also patches [3/7] ("PCI: PM: Drop struct pci_platform_pm_ops") and
[7/7] ("PCI: PM: Simplify acpi_pci_power_manageable()") are not likely
to introduce functional issue, because the former removes unused code
and the latter simply rearranges some computations, so I'm going to
queue up these two as well.

Patches [4-5/7] change behavior, if only slightly, so they need to be
double checked.

In turn, patch [6/7[ contains a bug - it makes
pci_platform_power_transition() call acpi_pci_set_power_state()
instead of platform_pci_set_power_state() which is probably why you
see the problem (nobody with an ACPI platform of any platform other
then MID would see it).  Sorry about that.

After queuing up the patches that are not problematic I'll prepare a
new 3-patch series to test on top of them.

Thanks!

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Nathan Chancellor]

On Mon, Sep 20, 2021 at 09:17:08PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Using struct pci_platform_pm_ops for ACPI adds unnecessary
> indirection to the interactions between the PCI core and ACPI PM,
> which is also subject to retpolines.
> 
> Moreover, it is not particularly clear from the current code that,
> as far as PCI PM is concerned, "platform" really means just ACPI
> except for the special casess when Intel MID PCI PM is used or when
> ACPI support is disabled (through the kernel config or command line,
> or because there are no usable ACPI tables on the system).
> 
> To address the above, rework the PCI PM code to invoke ACPI PM
> functions directly as needed and drop the acpi_pci_platform_pm
> object that is not necessary any more.
> 
> Accordingly, update some of the ACPI PM functions in question to do
> extra checks in case the ACPI support is disabled (which previously
> was taken care of by avoiding to set the pci_platform_ops pointer
> in those cases).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

This patch as commit 9896a58cdd59 ("PCI: ACPI: PM: Do not use
pci_platform_pm_ops for ACPI") in -next causes the following build error
when compiling x86_64 allmodconfig with clang:

drivers/pci/pci-acpi.c:1125:7: error: variable 'adev' is uninitialized when used here [-Werror,-Wuninitialized]
        if (!adev || !acpi_device_power_manageable(adev))
             ^~~~
drivers/pci/pci-acpi.c:1110:26: note: initialize the variable 'adev' to silence this warning
        struct acpi_device *adev;
                                ^
                                 = NULL
1 error generated.

Should the adev assignment be moved up or is there a different fix
necessary?

Cheers,
Nathan

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

On Wed, Sep 29, 2021 at 9:08 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Mon, Sep 20, 2021 at 09:17:08PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Using struct pci_platform_pm_ops for ACPI adds unnecessary
> > indirection to the interactions between the PCI core and ACPI PM,
> > which is also subject to retpolines.
> >
> > Moreover, it is not particularly clear from the current code that,
> > as far as PCI PM is concerned, "platform" really means just ACPI
> > except for the special casess when Intel MID PCI PM is used or when
> > ACPI support is disabled (through the kernel config or command line,
> > or because there are no usable ACPI tables on the system).
> >
> > To address the above, rework the PCI PM code to invoke ACPI PM
> > functions directly as needed and drop the acpi_pci_platform_pm
> > object that is not necessary any more.
> >
> > Accordingly, update some of the ACPI PM functions in question to do
> > extra checks in case the ACPI support is disabled (which previously
> > was taken care of by avoiding to set the pci_platform_ops pointer
> > in those cases).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> This patch as commit 9896a58cdd59 ("PCI: ACPI: PM: Do not use
> pci_platform_pm_ops for ACPI") in -next causes the following build error
> when compiling x86_64 allmodconfig with clang:
>
> drivers/pci/pci-acpi.c:1125:7: error: variable 'adev' is uninitialized when used here [-Werror,-Wuninitialized]
>         if (!adev || !acpi_device_power_manageable(adev))
>              ^~~~
> drivers/pci/pci-acpi.c:1110:26: note: initialize the variable 'adev' to silence this warning
>         struct acpi_device *adev;
>                                 ^
>                                  = NULL
> 1 error generated.
>
> Should the adev assignment be moved up

Yes, thanks!

I'll fix it up in the tree.

Re: [PATCH v2 2/7] PCI: ACPI: PM: Do not use pci_platform_pm_ops for ACPI

[Rafael J. Wysocki]

On Wed, Sep 29, 2021 at 9:08 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Mon, Sep 20, 2021 at 09:17:08PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Using struct pci_platform_pm_ops for ACPI adds unnecessary
> > indirection to the interactions between the PCI core and ACPI PM,
> > which is also subject to retpolines.
> >
> > Moreover, it is not particularly clear from the current code that,
> > as far as PCI PM is concerned, "platform" really means just ACPI
> > except for the special casess when Intel MID PCI PM is used or when
> > ACPI support is disabled (through the kernel config or command line,
> > or because there are no usable ACPI tables on the system).
> >
> > To address the above, rework the PCI PM code to invoke ACPI PM
> > functions directly as needed and drop the acpi_pci_platform_pm
> > object that is not necessary any more.
> >
> > Accordingly, update some of the ACPI PM functions in question to do
> > extra checks in case the ACPI support is disabled (which previously
> > was taken care of by avoiding to set the pci_platform_ops pointer
> > in those cases).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> This patch as commit 9896a58cdd59 ("PCI: ACPI: PM: Do not use
> pci_platform_pm_ops for ACPI") in -next causes the following build error
> when compiling x86_64 allmodconfig with clang:
>
> drivers/pci/pci-acpi.c:1125:7: error: variable 'adev' is uninitialized when used here [-Werror,-Wuninitialized]
>         if (!adev || !acpi_device_power_manageable(adev))
>              ^~~~
> drivers/pci/pci-acpi.c:1110:26: note: initialize the variable 'adev' to silence this warning
>         struct acpi_device *adev;
>                                 ^
>                                  = NULL
> 1 error generated.
>
> Should the adev assignment be moved up

Yes, thanks!

I'll fix it up in the tree.

[PATCH v2 3/7] PCI: PM: Drop struct pci_platform_pm_ops

[Rafael J. Wysocki]

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

After previous changes there are no more users of struct
pci_platform_pm_ops in the tree, so drop it along with all of the
remaining related code.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

New patch in v2.

---
 drivers/pci/pci.c |   11 -----------
 drivers/pci/pci.h |   39 ---------------------------------------
 2 files changed, 50 deletions(-)

Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -63,45 +63,6 @@ struct pci_cap_saved_state *pci_find_sav
 #define PCI_PM_D3HOT_WAIT       10	/* msec */
 #define PCI_PM_D3COLD_WAIT      100	/* msec */
 
-/**
- * struct pci_platform_pm_ops - Firmware PM callbacks
- *
- * @bridge_d3: Does the bridge allow entering into D3
- *
- * @is_manageable: returns 'true' if given device is power manageable by the
- *		   platform firmware
- *
- * @set_state: invokes the platform firmware to set the device's power state
- *
- * @get_state: queries the platform firmware for a device's current power state
- *
- * @refresh_state: asks the platform to refresh the device's power state data
- *
- * @choose_state: returns PCI power state of given device preferred by the
- *		  platform; to be used during system-wide transitions from a
- *		  sleeping state to the working state and vice versa
- *
- * @set_wakeup: enables/disables wakeup capability for the device
- *
- * @need_resume: returns 'true' if the given device (which is currently
- *		 suspended) needs to be resumed to be configured for system
- *		 wakeup.
- *
- * If given platform is generally capable of power managing PCI devices, all of
- * these callbacks are mandatory.
- */
-struct pci_platform_pm_ops {
-	bool (*bridge_d3)(struct pci_dev *dev);
-	bool (*is_manageable)(struct pci_dev *dev);
-	int (*set_state)(struct pci_dev *dev, pci_power_t state);
-	pci_power_t (*get_state)(struct pci_dev *dev);
-	void (*refresh_state)(struct pci_dev *dev);
-	pci_power_t (*choose_state)(struct pci_dev *dev);
-	int (*set_wakeup)(struct pci_dev *dev, bool enable);
-	bool (*need_resume)(struct pci_dev *dev);
-};
-
-int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_refresh_power_state(struct pci_dev *dev);
 int pci_power_up(struct pci_dev *dev);
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -972,17 +972,6 @@ static void pci_restore_bars(struct pci_
 		pci_update_resource(dev, i);
 }
 
-static const struct pci_platform_pm_ops *pci_platform_pm;
-
-int pci_set_platform_pm(const struct pci_platform_pm_ops *ops)
-{
-	if (!ops->is_manageable || !ops->set_state  || !ops->get_state ||
-	    !ops->choose_state  || !ops->set_wakeup || !ops->need_resume)
-		return -EINVAL;
-	pci_platform_pm = ops;
-	return 0;
-}
-
 static inline bool platform_pci_power_manageable(struct pci_dev *dev)
 {
 	if (pci_use_mid_pm())

[PATCH v2 4/7] PCI: PM: Rearrange pci_target_state()

[Rafael J. Wysocki]

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

Make pci_target_state() return D3cold or D0 without checking PME
support if the current power state of the device is D3cold or if it
does not support the standard PCI PM, respectively.

Next, drop the tergat_state local variable that has become redundant
from it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
    * Rebase on top of the new [1-3/7].

---
 drivers/pci/pci.c |   23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -2583,8 +2583,6 @@ EXPORT_SYMBOL(pci_wake_from_d3);
  */
 static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 {
-	pci_power_t target_state = PCI_D3hot;
-
 	if (platform_pci_power_manageable(dev)) {
 		/*
 		 * Call the platform to find the target state for the device.
@@ -2594,32 +2592,29 @@ static pci_power_t pci_target_state(stru
 		switch (state) {
 		case PCI_POWER_ERROR:
 		case PCI_UNKNOWN:
-			break;
+			return PCI_D3hot;
+
 		case PCI_D1:
 		case PCI_D2:
 			if (pci_no_d1d2(dev))
-				break;
-			fallthrough;
-		default:
-			target_state = state;
+				return PCI_D3hot;
 		}
 
-		return target_state;
+		return state;
 	}
 
-	if (!dev->pm_cap)
-		target_state = PCI_D0;
-
 	/*
 	 * If the device is in D3cold even though it's not power-manageable by
 	 * the platform, it may have been powered down by non-standard means.
 	 * Best to let it slumber.
 	 */
 	if (dev->current_state == PCI_D3cold)
-		target_state = PCI_D3cold;
+		return PCI_D3cold;
+	else if (!dev->pm_cap)
+		return PCI_D0;
 
 	if (wakeup && dev->pme_support) {
-		pci_power_t state = target_state;
+		pci_power_t state = PCI_D3hot;
 
 		/*
 		 * Find the deepest state from which the device can generate
@@ -2634,7 +2629,7 @@ static pci_power_t pci_target_state(stru
 			return PCI_D0;
 	}
 
-	return target_state;
+	return PCI_D3hot;
 }
 
 /**

[PATCH v2 5/7] PCI: PM: Make pci_choose_state() call pci_target_state()

[Rafael J. Wysocki]

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

The pci_choose_state() and pci_target_state() implementations are
somewhat divergent without a good reason, because they are used
for similar purposes.

Change the pci_choose_state() implementation to use pci_target_state()
internally except for transitions to the working state of the system
in which case it is expected to return D0.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---


v1 -> v2:
    * Rebase on top of the new [1-4/7].


---
 drivers/pci/pci-acpi.c |    3 --
 drivers/pci/pci.c      |   54 ++++++++++++++-----------------------------------
 2 files changed, 16 insertions(+), 41 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1394,44 +1394,6 @@ int pci_set_power_state(struct pci_dev *
 }
 EXPORT_SYMBOL(pci_set_power_state);
 
-/**
- * pci_choose_state - Choose the power state of a PCI device
- * @dev: PCI device to be suspended
- * @state: target sleep state for the whole system. This is the value
- *	   that is passed to suspend() function.
- *
- * Returns PCI power state suitable for given device and given system
- * message.
- */
-pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
-{
-	pci_power_t ret;
-
-	if (!dev->pm_cap)
-		return PCI_D0;
-
-	ret = platform_pci_choose_state(dev);
-	if (ret != PCI_POWER_ERROR)
-		return ret;
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		return PCI_D0;
-	case PM_EVENT_FREEZE:
-	case PM_EVENT_PRETHAW:
-		/* REVISIT both freeze and pre-thaw "should" use D0 */
-	case PM_EVENT_SUSPEND:
-	case PM_EVENT_HIBERNATE:
-		return PCI_D3hot;
-	default:
-		pci_info(dev, "unrecognized suspend event %d\n",
-			 state.event);
-		BUG();
-	}
-	return PCI_D0;
-}
-EXPORT_SYMBOL(pci_choose_state);
-
 #define PCI_EXP_SAVE_REGS	7
 
 static struct pci_cap_saved_state *_pci_find_saved_cap(struct pci_dev *pci_dev,
@@ -2843,6 +2805,22 @@ void pci_dev_complete_resume(struct pci_
 	spin_unlock_irq(&dev->power.lock);
 }
 
+/**
+ * pci_choose_state - Choose the power state of a PCI device.
+ * @dev: Target PCI device.
+ * @state: Target state for the whole system.
+ *
+ * Returns PCI power state suitable for @dev and @state.
+ */
+pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
+{
+	if (state.event == PM_EVENT_ON)
+		return PCI_D0;
+
+	return pci_target_state(dev, false);
+}
+EXPORT_SYMBOL(pci_choose_state);
+
 void pci_config_pm_runtime_get(struct pci_dev *pdev)
 {
 	struct device *dev = &pdev->dev;
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -910,9 +910,6 @@ pci_power_t acpi_pci_choose_state(struct
 {
 	int acpi_state, d_max;
 
-	if (acpi_pci_disabled)
-		return PCI_POWER_ERROR;
-
 	if (pdev->no_d3cold)
 		d_max = ACPI_STATE_D3_HOT;
 	else

[PATCH v2 6/7] PCI: PM: Do not call platform_pci_power_manageable() unnecessarily

[Rafael J. Wysocki]

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

Drop two invocations of platform_pci_power_manageable() that are not
necessary, because the functions called when it returns 'true' do the
requisite "power manageable" checks themselves.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Rebase on top of the new [1-5/7].
   * Move the acpi_pci_power_manageable() simplification to a separate
     patch.
   * Change the subject.

---
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1191,9 +1191,7 @@ void pci_update_current_state(struct pci
  */
 void pci_refresh_power_state(struct pci_dev *dev)
 {
-	if (platform_pci_power_manageable(dev))
-		platform_pci_refresh_power_state(dev);
-
+	platform_pci_refresh_power_state(dev);
 	pci_update_current_state(dev, dev->current_state);
 }
 
@@ -1206,14 +1204,10 @@ int pci_platform_power_transition(struct
 {
 	int error;
 
-	if (platform_pci_power_manageable(dev)) {
-		error = platform_pci_set_power_state(dev, state);
-		if (!error)
-			pci_update_current_state(dev, state);
-	} else
-		error = -ENODEV;
-
-	if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
+	error = acpi_pci_set_power_state(dev, state);
+	if (!error)
+		pci_update_current_state(dev, state);
+	else if (!dev->pm_cap) /* Fall back to PCI_D0 */
 		dev->current_state = PCI_D0;
 
 	return error;

[PATCH v2 7/7] PCI: PM: Simplify acpi_pci_power_manageable()

[Rafael J. Wysocki]

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

Make acpi_pci_power_manageable() more straightforward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

New patch in v2.

---
 drivers/pci/pci-acpi.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -969,9 +969,7 @@ bool acpi_pci_power_manageable(struct pc
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 
-	if (!adev)
-		return false;
-	return acpi_device_power_manageable(adev);
+	return adev && acpi_device_power_manageable(adev);
 }
 
 bool acpi_pci_bridge_d3(struct pci_dev *dev)

Re: [PATCH v2 0/7] PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code

[Bjorn Helgaas]

[+cc Ferry]

On Mon, Sep 20, 2021 at 08:52:19PM +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> As explained in the changelog of patch [2/7], using struct pci_platform_pm_ops
> for ACPI is not particularly beneficial, so it is better to get rid of it and
> call the functions pointed to by it directly from the PCI core.
> 
> However, struct pci_platform_pm_ops is also used by the Intel MID support code,
> but it is actually better to call the MID PM function directly from the PCI
> core either, which is done in patch [1/7].
> 
> After these changes, patch [3/7] removes struct pci_platform_pm_ops and the
> rest is just cleanups and some code consolidation on top of that.

I like these a lot.  Not sure exactly where everything is after the
conversation with Ferry.  Let me know if I should be doing anything.

Re: [PATCH v2 0/7] PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code

[Rafael J. Wysocki]

On Wed, Sep 29, 2021 at 1:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Ferry]
>
> On Mon, Sep 20, 2021 at 08:52:19PM +0200, Rafael J. Wysocki wrote:
> > Hi All,
> >
> > As explained in the changelog of patch [2/7], using struct pci_platform_pm_ops
> > for ACPI is not particularly beneficial, so it is better to get rid of it and
> > call the functions pointed to by it directly from the PCI core.
> >
> > However, struct pci_platform_pm_ops is also used by the Intel MID support code,
> > but it is actually better to call the MID PM function directly from the PCI
> > core either, which is done in patch [1/7].
> >
> > After these changes, patch [3/7] removes struct pci_platform_pm_ops and the
> > rest is just cleanups and some code consolidation on top of that.
>
> I like these a lot.  Not sure exactly where everything is after the
> conversation with Ferry.

It's mostly OK, the problem was in one of the "tail" patches that was
not rebased properly.

There will be a follow-up series to test for Ferry (later today).

>  Let me know if I should be doing anything.

I'm going to take this lot if that's not a problem.  If I need
anything from you, I'll let you know.

Re: [PATCH v2 0/7] PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code

[Bjorn Helgaas]

On Wed, Sep 29, 2021 at 02:00:59PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 29, 2021 at 1:28 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Ferry]
> >
> > On Mon, Sep 20, 2021 at 08:52:19PM +0200, Rafael J. Wysocki wrote:
> > > Hi All,
> > >
> > > As explained in the changelog of patch [2/7], using struct pci_platform_pm_ops
> > > for ACPI is not particularly beneficial, so it is better to get rid of it and
> > > call the functions pointed to by it directly from the PCI core.
> > >
> > > However, struct pci_platform_pm_ops is also used by the Intel MID support code,
> > > but it is actually better to call the MID PM function directly from the PCI
> > > core either, which is done in patch [1/7].
> > >
> > > After these changes, patch [3/7] removes struct pci_platform_pm_ops and the
> > > rest is just cleanups and some code consolidation on top of that.
> >
> > I like these a lot.  Not sure exactly where everything is after the
> > conversation with Ferry.
> 
> It's mostly OK, the problem was in one of the "tail" patches that was
> not rebased properly.
> 
> There will be a follow-up series to test for Ferry (later today).
> 
> >  Let me know if I should be doing anything.
> 
> I'm going to take this lot if that's not a problem.  If I need
> anything from you, I'll let you know.

Sounds good, thanks, Rafael!

Bjorn

Re: [PATCH v2 0/7] PCI: ACPI: Get rid of struct pci_platform_pm_ops and clean up code

[Ferry Toth]

Hi

Op 29-09-2021 om 01:28 schreef Bjorn Helgaas:
> [+cc Ferry]
> 
> On Mon, Sep 20, 2021 at 08:52:19PM +0200, Rafael J. Wysocki wrote:
>> Hi All,
>>
>> As explained in the changelog of patch [2/7], using struct pci_platform_pm_ops
>> for ACPI is not particularly beneficial, so it is better to get rid of it and
>> call the functions pointed to by it directly from the PCI core.
>>
>> However, struct pci_platform_pm_ops is also used by the Intel MID support code,
>> but it is actually better to call the MID PM function directly from the PCI
>> core either, which is done in patch [1/7].
>>
>> After these changes, patch [3/7] removes struct pci_platform_pm_ops and the
>> rest is just cleanups and some code consolidation on top of that.
> 
> I like these a lot.  Not sure exactly where everything is after the
> conversation with Ferry.  Let me know if I should be doing anything.
> 
I will happily retest likely on Sunday after I return from short holiday 
and report back here.

[PATCH v3 0/3] PCI: PM: Simplify and unify some helper functions

[Rafael J. Wysocki]

Hi All,

This series is on top of the linux-next branch from linux-pm.git:

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next

which should be included in linux-next.

Two of the 3 patches in this series, [1-2/3], were included in the "PCI: ACPI:
Get rid of struct pci_platform_pm_ops and clean up code" series:

 https://lore.kernel.org/linux-acpi/1800633.tdWV9SEqCh@kreacher/

and the remaining one, [3/3] is a new version of a problematic patch from that
series.  The rest of that series is present in the git branch above.

All of the 3 patches in this set need to be tested in order to verify that
there are no more issues that need to be addressed in them.

Ferry, please test!

Thanks!

[PATCH v3 1/3] PCI: PM: Rearrange pci_target_state()

[Rafael J. Wysocki]

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

Make pci_target_state() return D3cold or D0 without checking PME
support if the current power state of the device is D3cold or if it
does not support the standard PCI PM, respectively.

Next, drop the tergat_state local variable that has become redundant
from it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Same as the v2:

https://patchwork.kernel.org/project/linux-acpi/patch/2559274.X9hSmTKtgW@kreacher/

---
 drivers/pci/pci.c |   23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -2583,8 +2583,6 @@ EXPORT_SYMBOL(pci_wake_from_d3);
  */
 static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 {
-	pci_power_t target_state = PCI_D3hot;
-
 	if (platform_pci_power_manageable(dev)) {
 		/*
 		 * Call the platform to find the target state for the device.
@@ -2594,32 +2592,29 @@ static pci_power_t pci_target_state(stru
 		switch (state) {
 		case PCI_POWER_ERROR:
 		case PCI_UNKNOWN:
-			break;
+			return PCI_D3hot;
+
 		case PCI_D1:
 		case PCI_D2:
 			if (pci_no_d1d2(dev))
-				break;
-			fallthrough;
-		default:
-			target_state = state;
+				return PCI_D3hot;
 		}
 
-		return target_state;
+		return state;
 	}
 
-	if (!dev->pm_cap)
-		target_state = PCI_D0;
-
 	/*
 	 * If the device is in D3cold even though it's not power-manageable by
 	 * the platform, it may have been powered down by non-standard means.
 	 * Best to let it slumber.
 	 */
 	if (dev->current_state == PCI_D3cold)
-		target_state = PCI_D3cold;
+		return PCI_D3cold;
+	else if (!dev->pm_cap)
+		return PCI_D0;
 
 	if (wakeup && dev->pme_support) {
-		pci_power_t state = target_state;
+		pci_power_t state = PCI_D3hot;
 
 		/*
 		 * Find the deepest state from which the device can generate
@@ -2634,7 +2629,7 @@ static pci_power_t pci_target_state(stru
 			return PCI_D0;
 	}
 
-	return target_state;
+	return PCI_D3hot;
 }
 
 /**

[PATCH v3 2/3] PCI: PM: Make pci_choose_state() call pci_target_state()

[Rafael J. Wysocki]

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

The pci_choose_state() and pci_target_state() implementations are
somewhat divergent without a good reason, because they are used
for similar purposes.

Change the pci_choose_state() implementation to use pci_target_state()
internally except for transitions to the working state of the system
in which case it is expected to return D0.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Same as the v2:

https://patchwork.kernel.org/project/linux-acpi/patch/12860712.dW097sEU6C@kreacher/

---
 drivers/pci/pci-acpi.c |    3 --
 drivers/pci/pci.c      |   54 ++++++++++++++-----------------------------------
 2 files changed, 16 insertions(+), 41 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1394,44 +1394,6 @@ int pci_set_power_state(struct pci_dev *
 }
 EXPORT_SYMBOL(pci_set_power_state);
 
-/**
- * pci_choose_state - Choose the power state of a PCI device
- * @dev: PCI device to be suspended
- * @state: target sleep state for the whole system. This is the value
- *	   that is passed to suspend() function.
- *
- * Returns PCI power state suitable for given device and given system
- * message.
- */
-pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
-{
-	pci_power_t ret;
-
-	if (!dev->pm_cap)
-		return PCI_D0;
-
-	ret = platform_pci_choose_state(dev);
-	if (ret != PCI_POWER_ERROR)
-		return ret;
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		return PCI_D0;
-	case PM_EVENT_FREEZE:
-	case PM_EVENT_PRETHAW:
-		/* REVISIT both freeze and pre-thaw "should" use D0 */
-	case PM_EVENT_SUSPEND:
-	case PM_EVENT_HIBERNATE:
-		return PCI_D3hot;
-	default:
-		pci_info(dev, "unrecognized suspend event %d\n",
-			 state.event);
-		BUG();
-	}
-	return PCI_D0;
-}
-EXPORT_SYMBOL(pci_choose_state);
-
 #define PCI_EXP_SAVE_REGS	7
 
 static struct pci_cap_saved_state *_pci_find_saved_cap(struct pci_dev *pci_dev,
@@ -2843,6 +2805,22 @@ void pci_dev_complete_resume(struct pci_
 	spin_unlock_irq(&dev->power.lock);
 }
 
+/**
+ * pci_choose_state - Choose the power state of a PCI device.
+ * @dev: Target PCI device.
+ * @state: Target state for the whole system.
+ *
+ * Returns PCI power state suitable for @dev and @state.
+ */
+pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state)
+{
+	if (state.event == PM_EVENT_ON)
+		return PCI_D0;
+
+	return pci_target_state(dev, false);
+}
+EXPORT_SYMBOL(pci_choose_state);
+
 void pci_config_pm_runtime_get(struct pci_dev *pdev)
 {
 	struct device *dev = &pdev->dev;
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -910,9 +910,6 @@ pci_power_t acpi_pci_choose_state(struct
 {
 	int acpi_state, d_max;
 
-	if (acpi_pci_disabled)
-		return PCI_POWER_ERROR;
-
 	if (pdev->no_d3cold)
 		d_max = ACPI_STATE_D3_HOT;
 	else

[PATCH v3 3/3] PCI: PM: Do not call platform_pci_power_manageable() unnecessarily

[Rafael J. Wysocki]

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

Drop two invocations of platform_pci_power_manageable() that are not
necessary, because the functions called when it returns 'true' do the
requisite "power manageable" checks themselves.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 (https://patchwork.kernel.org/project/linux-acpi/patch/2014133.KlZ2vcFHjT@kreacher/) -> v3:
   * Call platform_pci_set_power_state() in pci_platform_power_transition() as
     appropriate.

---
 drivers/pci/pci.c |   16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1191,9 +1191,7 @@ void pci_update_current_state(struct pci
  */
 void pci_refresh_power_state(struct pci_dev *dev)
 {
-	if (platform_pci_power_manageable(dev))
-		platform_pci_refresh_power_state(dev);
-
+	platform_pci_refresh_power_state(dev);
 	pci_update_current_state(dev, dev->current_state);
 }
 
@@ -1206,14 +1204,10 @@ int pci_platform_power_transition(struct
 {
 	int error;
 
-	if (platform_pci_power_manageable(dev)) {
-		error = platform_pci_set_power_state(dev, state);
-		if (!error)
-			pci_update_current_state(dev, state);
-	} else
-		error = -ENODEV;
-
-	if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
+	error = platform_pci_set_power_state(dev, state);
+	if (!error)
+		pci_update_current_state(dev, state);
+	else if (!dev->pm_cap) /* Fall back to PCI_D0 */
 		dev->current_state = PCI_D0;
 
 	return error;

Re: [PATCH v3 0/3] PCI: PM: Simplify and unify some helper functions

[Ferry Toth]

Hi,

Op 29-09-2021 om 20:05 schreef Rafael J. Wysocki:
> Hi All,
> 
> This series is on top of the linux-next branch from linux-pm.git:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> 
> which should be included in linux-next.
> 
> Two of the 3 patches in this series, [1-2/3], were included in the "PCI: ACPI:
> Get rid of struct pci_platform_pm_ops and clean up code" series:
> 
>   https://lore.kernel.org/linux-acpi/1800633.tdWV9SEqCh@kreacher/
> 
> and the remaining one, [3/3] is a new version of a problematic patch from that
> series.  The rest of that series is present in the git branch above.
> 
> All of the 3 patches in this set need to be tested in order to verify that
> there are no more issues that need to be addressed in them.
> 
> Ferry, please test!

This is how I tested:
3 patches from 
https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/ 
on top of 5.15.0-rc2 as before
4 patches from v2 in the order of linux-pm.git
then tested without, with 1/3, 1+2/3, 1+2+3/3 on top (with only 3/3 the 
new patch, 1+2/3 taken from v2 as they are unchanged).

In all 4 cases I didn't find any trouble (related to this patch).

Thanks for doing this!

> Thanks!
> 
> 
> 
> 

Re: [PATCH v3 0/3] PCI: PM: Simplify and unify some helper functions

[Rafael J. Wysocki]

On Sun, Oct 3, 2021 at 10:14 PM Ferry Toth <fntoth@gmail.com> wrote:
>
> Hi,
>
> Op 29-09-2021 om 20:05 schreef Rafael J. Wysocki:
> > Hi All,
> >
> > This series is on top of the linux-next branch from linux-pm.git:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> >
> > which should be included in linux-next.
> >
> > Two of the 3 patches in this series, [1-2/3], were included in the "PCI: ACPI:
> > Get rid of struct pci_platform_pm_ops and clean up code" series:
> >
> >   https://lore.kernel.org/linux-acpi/1800633.tdWV9SEqCh@kreacher/
> >
> > and the remaining one, [3/3] is a new version of a problematic patch from that
> > series.  The rest of that series is present in the git branch above.
> >
> > All of the 3 patches in this set need to be tested in order to verify that
> > there are no more issues that need to be addressed in them.
> >
> > Ferry, please test!
>
> This is how I tested:
> 3 patches from
> https://patchwork.kernel.org/project/linux-acpi/patch/2793105.e9J7NaK4W3@kreacher/
> on top of 5.15.0-rc2 as before
> 4 patches from v2 in the order of linux-pm.git
> then tested without, with 1/3, 1+2/3, 1+2+3/3 on top (with only 3/3 the
> new patch, 1+2/3 taken from v2 as they are unchanged).
>
> In all 4 cases I didn't find any trouble (related to this patch).
>
> Thanks for doing this!

Thank you!