diff mbox

[5/6] PCI / ACPI / PM: Avoid disabling wakeup for bridges too early

Message ID 374495495.9Wgi8cFFrW@aspire.rjw.lan
State Superseded
Headers show

Commit Message

Rafael J. Wysocki June 19, 2017, 9:36 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If acpi_pci_propagate_wakeup() is used, it may trigger wakeup
configuration twice for the same bridge indirectly, when configuring
wakeup for two different PCI devices under that bridge.  Actually,
however, the ACPI wakeup code can only be enabled to trigger wakeup
notifications for the bridge once in a row and there is no reference
counting in that code, so wakeup signaling on the bridge can be
disabled prematurely when it is disabled for the first of those
devices.

To prevent that from happening, add optional reference counting to
the ACPI device wakeup code and make acpi_pci_propagate_wakeup()
use if for bridges.

This works, because ACPI wakeup signaling for PCI bridges is only
set up by acpi_pci_propagate_wakeup(), so the reference counting
will only be used for them.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   35 +++++++++++++++++++++++++++++++----
 drivers/pci/pci-acpi.c   |    4 ++--
 include/acpi/acpi_bus.h  |   11 +++++++++--
 3 files changed, 42 insertions(+), 8 deletions(-)

Comments

kernel test robot June 20, 2017, 12:38 p.m. UTC | #1 
Hi Rafael,

[auto build test WARNING on pm/linux-next]
[also build test WARNING on next-20170620]
[cannot apply to v4.12-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rafael-J-Wysocki/PM-Unify-the-handling-of-device-wakeup-settings/20170620-195729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-x019-201725 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/acpi/device_pm.c: In function '__acpi_pm_device_wakeup':
>> drivers/acpi/device_pm.c:735:6: warning: 'error' may be used uninitialized in this function [-Wmaybe-uninitialized]
     int error;
         ^~~~~

vim +/error +735 drivers/acpi/device_pm.c

235d81a6 Rafael J. Wysocki 2017-06-12  719  	} else if (adev->wakeup.flags.enabled) {
dee8370c Rafael J. Wysocki 2012-11-02  720  		acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number);
dee8370c Rafael J. Wysocki 2012-11-02  721  		acpi_disable_wakeup_device_power(adev);
235d81a6 Rafael J. Wysocki 2017-06-12  722  		adev->wakeup.flags.enabled = 0;
dee8370c Rafael J. Wysocki 2012-11-02  723  	}
dee8370c Rafael J. Wysocki 2012-11-02  724  	return 0;
dee8370c Rafael J. Wysocki 2012-11-02  725  }
dee8370c Rafael J. Wysocki 2012-11-02  726  
dee8370c Rafael J. Wysocki 2012-11-02  727  /**
6909f7ad Rafael J. Wysocki 2017-06-19  728   * __acpi_pm_device_wakeup - Enable/disable remote wakeup for given device.
9f7d805b Rafael J. Wysocki 2017-06-19  729   * @dev: Device to enable/disable to generate wakeup events.
dee8370c Rafael J. Wysocki 2012-11-02  730   * @enable: Whether to enable or disable the wakeup functionality.
cd7bd02d Rafael J. Wysocki 2012-11-02  731   */
6909f7ad Rafael J. Wysocki 2017-06-19  732  int __acpi_pm_device_wakeup(struct device *dev, bool enable, bool refcount)
a6ae7594 Rafael J. Wysocki 2012-11-02  733  {
a6ae7594 Rafael J. Wysocki 2012-11-02  734  	struct acpi_device *adev;
a6ae7594 Rafael J. Wysocki 2012-11-02 @735  	int error;
a6ae7594 Rafael J. Wysocki 2012-11-02  736  
17653a3e Rafael J. Wysocki 2014-07-23  737  	adev = ACPI_COMPANION(dev);
17653a3e Rafael J. Wysocki 2014-07-23  738  	if (!adev) {
17653a3e Rafael J. Wysocki 2014-07-23  739  		dev_dbg(dev, "ACPI companion missing in %s!\n", __func__);
a6ae7594 Rafael J. Wysocki 2012-11-02  740  		return -ENODEV;
a6ae7594 Rafael J. Wysocki 2012-11-02  741  	}
a6ae7594 Rafael J. Wysocki 2012-11-02  742  
9f7d805b Rafael J. Wysocki 2017-06-19  743  	if (!acpi_device_can_wakeup(adev))

:::::: The code at line 735 was first introduced by commit
:::::: a6ae7594b1b157e0e7976ed105a7be27d69a5361 ACPI / PM: Move device PM functions related to sleep states

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

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox

Patch

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -385,6 +385,7 @@  EXPORT_SYMBOL(acpi_bus_power_manageable)
 
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(acpi_pm_notifier_lock);
+static DEFINE_MUTEX(acpi_wakeup_lock);
 
 void acpi_pm_wakeup_event(struct device *dev)
 {
@@ -724,11 +725,11 @@  static int acpi_device_wakeup(struct acp
 }
 
 /**
- * acpi_pm_device_wakeup - Enable/disable remote wakeup for given device.
+ * __acpi_pm_device_wakeup - Enable/disable remote wakeup for given device.
  * @dev: Device to enable/disable to generate wakeup events.
  * @enable: Whether to enable or disable the wakeup functionality.
  */
-int acpi_pm_device_wakeup(struct device *dev, bool enable)
+int __acpi_pm_device_wakeup(struct device *dev, bool enable, bool refcount)
 {
 	struct acpi_device *adev;
 	int error;
@@ -742,13 +743,39 @@  int acpi_pm_device_wakeup(struct device
 	if (!acpi_device_can_wakeup(adev))
 		return -EINVAL;
 
+	if (refcount) {
+		mutex_lock(&acpi_wakeup_lock);
+
+		if (enable) {
+			if (++adev->wakeup.enable_count > 1)
+				goto out;
+		} else {
+			if (WARN_ON_ONCE(adev->wakeup.enable_count == 0))
+				goto out;
+
+			if (--adev->wakeup.enable_count > 0)
+				goto out;
+		}
+	}
 	error = acpi_device_wakeup(adev, acpi_target_system_state(), enable);
-	if (!error)
+	if (error) {
+		if (refcount) {
+			if (enable)
+				adev->wakeup.enable_count--;
+			else
+				adev->wakeup.enable_count++;
+		}
+	} else {
 		dev_dbg(dev, "Wakeup %s by ACPI\n", enable ? "enabled" : "disabled");
+	}
+
+out:
+	if (refcount)
+		mutex_unlock(&acpi_wakeup_lock);
 
 	return error;
 }
-EXPORT_SYMBOL(acpi_pm_device_wakeup);
+EXPORT_SYMBOL(__acpi_pm_device_wakeup);
 
 /**
  * acpi_dev_pm_low_power - Put ACPI device into a low-power state.
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -331,6 +331,7 @@  struct acpi_device_wakeup {
 	struct acpi_device_wakeup_context context;
 	struct wakeup_source *ws;
 	int prepare_count;
+	unsigned int enable_count;
 };
 
 struct acpi_device_physical_node {
@@ -603,7 +604,7 @@  acpi_status acpi_add_pm_notifier(struct
 acpi_status acpi_remove_pm_notifier(struct acpi_device *adev);
 bool acpi_pm_device_can_wakeup(struct device *dev);
 int acpi_pm_device_sleep_state(struct device *, int *, int);
-int acpi_pm_device_wakeup(struct device *dev, bool enable);
+int __acpi_pm_device_wakeup(struct device *dev, bool enable, bool refcount);
 #else
 static inline void acpi_pm_wakeup_event(struct device *dev)
 {
@@ -630,12 +631,18 @@  static inline int acpi_pm_device_sleep_s
 	return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3_COLD) ?
 		m : ACPI_STATE_D0;
 }
-static inline int acpi_pm_device_wakeup(struct device *dev, bool enable)
+static inline int __acpi_pm_device_wakeup(struct device *dev, bool enable,
+					  bool refcount)
 {
 	return -ENODEV;
 }
 #endif
 
+static inline int acpi_pm_device_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_device_wakeup(dev, enable, false);
+}
+
 #ifdef CONFIG_ACPI_SLEEP
 u32 acpi_target_system_state(void);
 #else
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -574,7 +574,7 @@  static int acpi_pci_propagate_wakeup(str
 {
 	while (bus->parent) {
 		if (acpi_pm_device_can_wakeup(&bus->self->dev))
-			return acpi_pm_device_wakeup(&bus->self->dev, enable);
+			return __acpi_pm_device_wakeup(&bus->self->dev, enable, true);
 
 		bus = bus->parent;
 	}
@@ -582,7 +582,7 @@  static int acpi_pci_propagate_wakeup(str
 	/* We have reached the root bus. */
 	if (bus->bridge) {
 		if (acpi_pm_device_can_wakeup(bus->bridge))
-			return acpi_pm_device_wakeup(bus->bridge, enable);
+			return __acpi_pm_device_wakeup(bus->bridge, enable, true);
 	}
 	return 0;
 }