[PATCH 0/3] PCI / ACPI / PM: Fix propagation of wakeup settings to bridges

[PATCH 0/3] PCI / ACPI / PM: Fix propagation of wakeup settings to bridges

[Rafael J. Wysocki]

Hi,

The device wakeup code in pci-acpi.c currently has a (theoretical) problem that
it can disable wakeup for a bridge prematurely in some obscure conditions.

This is described in some more detail in the changelog of patch [3/3] and it
has been there for quite a while, so it's nothing new, but it would be good
to fix it at last and hence this series.

Patches [1/3] and [2/3] are mostly preparatory, although the former may be
regarded as a fix by itself and the latter is a cleanup.

Thanks,
Rafael

[PATCH 1/3] PCI / PM: Skip bridges in pci_enable_wake()

[Rafael J. Wysocki]

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

PCI bridges only have a reason to generate wakeup signals on behalf
of devices below them, so avoid preparing bridges for wakeup directly
in pci_enable_wake().

Also drop the pci_has_subordinate() check from pci_pm_default_resume()
as this will be done by pci_enable_wake() itself now.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |    4 +---
 drivers/pci/pci.c        |    7 +++++++
 2 files changed, 8 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1909,6 +1909,13 @@ int pci_enable_wake(struct pci_dev *dev,
 {
 	int ret = 0;
 
+	/*
+	 * Bridges can only signal wakeup on behalf of subordinate devices,
+	 * but that is set up elsewhere, so skip them.
+	 */
+	if (pci_has_subordinate(dev))
+		return 0;
+
 	/* Don't do the same thing twice in a row for one device. */
 	if (!!enable == !!dev->wakeup_prepared)
 		return 0;
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -642,9 +642,7 @@ static int pci_legacy_resume(struct devi
 static void pci_pm_default_resume(struct pci_dev *pci_dev)
 {
 	pci_fixup_device(pci_fixup_resume, pci_dev);
-
-	if (!pci_has_subordinate(pci_dev))
-		pci_enable_wake(pci_dev, PCI_D0, false);
+	pci_enable_wake(pci_dev, PCI_D0, false);
 }
 
 static void pci_pm_default_suspend(struct pci_dev *pci_dev)

Re: [PATCH 1/3] PCI / PM: Skip bridges in pci_enable_wake()

[Mika Westerberg]

On Fri, Jul 21, 2017 at 02:38:08PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> PCI bridges only have a reason to generate wakeup signals on behalf
> of devices below them, so avoid preparing bridges for wakeup directly
> in pci_enable_wake().
> 
> Also drop the pci_has_subordinate() check from pci_pm_default_resume()
> as this will be done by pci_enable_wake() itself now.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/3] PCI / PM: Skip bridges in pci_enable_wake()

[Bjorn Helgaas]

On Fri, Jul 21, 2017 at 02:38:08PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> PCI bridges only have a reason to generate wakeup signals on behalf
> of devices below them, so avoid preparing bridges for wakeup directly
> in pci_enable_wake().
> 
> Also drop the pci_has_subordinate() check from pci_pm_default_resume()
> as this will be done by pci_enable_wake() itself now.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/pci-driver.c |    4 +---
>  drivers/pci/pci.c        |    7 +++++++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -1909,6 +1909,13 @@ int pci_enable_wake(struct pci_dev *dev,
>  {
>  	int ret = 0;
>  
> +	/*
> +	 * Bridges can only signal wakeup on behalf of subordinate devices,
> +	 * but that is set up elsewhere, so skip them.

A specific pointer to this "elsewhere" would be useful here.

> +	 */
> +	if (pci_has_subordinate(dev))
> +		return 0;
> +
>  	/* Don't do the same thing twice in a row for one device. */
>  	if (!!enable == !!dev->wakeup_prepared)
>  		return 0;
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -642,9 +642,7 @@ static int pci_legacy_resume(struct devi
>  static void pci_pm_default_resume(struct pci_dev *pci_dev)
>  {
>  	pci_fixup_device(pci_fixup_resume, pci_dev);
> -
> -	if (!pci_has_subordinate(pci_dev))
> -		pci_enable_wake(pci_dev, PCI_D0, false);
> +	pci_enable_wake(pci_dev, PCI_D0, false);
>  }
>  
>  static void pci_pm_default_suspend(struct pci_dev *pci_dev)
> 

Re: [PATCH 1/3] PCI / PM: Skip bridges in pci_enable_wake()

[Rafael J. Wysocki]

On Mon, Jul 31, 2017 at 10:53 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Jul 21, 2017 at 02:38:08PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> PCI bridges only have a reason to generate wakeup signals on behalf
>> of devices below them, so avoid preparing bridges for wakeup directly
>> in pci_enable_wake().
>>
>> Also drop the pci_has_subordinate() check from pci_pm_default_resume()
>> as this will be done by pci_enable_wake() itself now.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks!

>> ---
>>  drivers/pci/pci-driver.c |    4 +---
>>  drivers/pci/pci.c        |    7 +++++++
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> Index: linux-pm/drivers/pci/pci.c
>> ===================================================================
>> --- linux-pm.orig/drivers/pci/pci.c
>> +++ linux-pm/drivers/pci/pci.c
>> @@ -1909,6 +1909,13 @@ int pci_enable_wake(struct pci_dev *dev,
>>  {
>>       int ret = 0;
>>
>> +     /*
>> +      * Bridges can only signal wakeup on behalf of subordinate devices,
>> +      * but that is set up elsewhere, so skip them.
>
> A specific pointer to this "elsewhere" would be useful here.

OK

[PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Rafael J. Wysocki]

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

To prepare for a subsequent change and make the code somewhat easier
to follow, do the following in the ACPI device wakeup handling code:

 * Replace wakeup.flags.enabled under struct acpi_device with
   wakeup.enable_count as that will be necessary going forward.

   For now, wakeup.enable_count is not allowed to grow beyond 1,
   so the current behavior is retained.

 * Split acpi_device_wakeup() into acpi_device_wakeup_enable()
   and acpi_device_wakeup_disable() and modify the callers of
   it accordingly.

 * Introduce a new acpi_wakeup_lock mutex to protect the wakeup
   enabling/disabling code from races in case it is executed
   more than once in parallel for the same device (which may
   happen for bridges theoretically).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |  121 ++++++++++++++++++++++++++++++-----------------
 include/acpi/acpi_bus.h  |    2 
 2 files changed, 80 insertions(+), 43 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -680,47 +680,74 @@ static void acpi_pm_notify_work_func(str
 	}
 }
 
+static DEFINE_MUTEX(acpi_wakeup_lock);
+
 /**
- * acpi_device_wakeup - Enable/disable wakeup functionality for device.
- * @adev: ACPI device to enable/disable wakeup functionality for.
+ * acpi_device_wakeup_enable - Enable wakeup functionality for device.
+ * @adev: ACPI device to enable wakeup functionality for.
  * @target_state: State the system is transitioning into.
- * @enable: Whether to enable or disable the wakeup functionality.
  *
- * Enable/disable the GPE associated with @adev so that it can generate
- * wakeup signals for the device in response to external (remote) events and
- * enable/disable device wakeup power.
+ * Enable the GPE associated with @adev so that it can generate wakeup signals
+ * for the device in response to external (remote) events and enable wakeup
+ * power for it.
  *
  * Callers must ensure that @adev is a valid ACPI device node before executing
  * this function.
  */
-static int acpi_device_wakeup(struct acpi_device *adev, u32 target_state,
-			      bool enable)
+static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
 {
 	struct acpi_device_wakeup *wakeup = &adev->wakeup;
+	acpi_status status;
+	int error = 0;
 
-	if (enable) {
-		acpi_status res;
-		int error;
+	mutex_lock(&acpi_wakeup_lock);
 
-		if (adev->wakeup.flags.enabled)
-			return 0;
+	if (wakeup->enable_count > 0)
+		goto out;
 
-		error = acpi_enable_wakeup_device_power(adev, target_state);
-		if (error)
-			return error;
+	error = acpi_enable_wakeup_device_power(adev, target_state);
+	if (error)
+		goto out;
 
-		res = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
-		if (ACPI_FAILURE(res)) {
-			acpi_disable_wakeup_device_power(adev);
-			return -EIO;
-		}
-		adev->wakeup.flags.enabled = 1;
-	} else if (adev->wakeup.flags.enabled) {
-		acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number);
+	status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
+	if (ACPI_FAILURE(status)) {
 		acpi_disable_wakeup_device_power(adev);
-		adev->wakeup.flags.enabled = 0;
+		error = -EIO;
+		goto out;
 	}
-	return 0;
+
+	wakeup->enable_count++;
+
+out:
+	mutex_unlock(&acpi_wakeup_lock);
+	return error;
+}
+
+/**
+ * acpi_device_wakeup_disable - Disable wakeup functionality for device.
+ * @adev: ACPI device to disable wakeup functionality for.
+ *
+ * Disable the GPE associated with @adev and disable wakeup power for it.
+ *
+ * Callers must ensure that @adev is a valid ACPI device node before executing
+ * this function.
+ */
+static void acpi_device_wakeup_disable(struct acpi_device *adev)
+{
+	struct acpi_device_wakeup *wakeup = &adev->wakeup;
+
+	mutex_lock(&acpi_wakeup_lock);
+
+	if (!wakeup->enable_count)
+		goto out;
+
+	acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number);
+	acpi_disable_wakeup_device_power(adev);
+
+	wakeup->enable_count--;
+
+out:
+	mutex_unlock(&acpi_wakeup_lock);
 }
 
 /**
@@ -742,9 +769,15 @@ int acpi_pm_set_device_wakeup(struct dev
 	if (!acpi_device_can_wakeup(adev))
 		return -EINVAL;
 
-	error = acpi_device_wakeup(adev, acpi_target_system_state(), enable);
+	if (!enable) {
+		acpi_device_wakeup_disable(adev);
+		dev_dbg(dev, "Wakeup disabled by ACPI\n");
+		return 0;
+	}
+
+	error = acpi_device_wakeup_enable(adev, acpi_target_system_state());
 	if (!error)
-		dev_dbg(dev, "Wakeup %s by ACPI\n", enable ? "enabled" : "disabled");
+		dev_dbg(dev, "Wakeup enabled by ACPI\n");
 
 	return error;
 }
@@ -798,13 +831,15 @@ int acpi_dev_runtime_suspend(struct devi
 
 	remote_wakeup = dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) >
 				PM_QOS_FLAGS_NONE;
-	error = acpi_device_wakeup(adev, ACPI_STATE_S0, remote_wakeup);
-	if (remote_wakeup && error)
-		return -EAGAIN;
+	if (remote_wakeup) {
+		error = acpi_device_wakeup_enable(adev, ACPI_STATE_S0);
+		if (error)
+			return -EAGAIN;
+	}
 
 	error = acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
-	if (error)
-		acpi_device_wakeup(adev, ACPI_STATE_S0, false);
+	if (error && remote_wakeup)
+		acpi_device_wakeup_disable(adev);
 
 	return error;
 }
@@ -827,7 +862,7 @@ int acpi_dev_runtime_resume(struct devic
 		return 0;
 
 	error = acpi_dev_pm_full_power(adev);
-	acpi_device_wakeup(adev, ACPI_STATE_S0, false);
+	acpi_device_wakeup_disable(adev);
 	return error;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume);
@@ -882,13 +917,15 @@ int acpi_dev_suspend_late(struct device
 
 	target_state = acpi_target_system_state();
 	wakeup = device_may_wakeup(dev) && acpi_device_can_wakeup(adev);
-	error = acpi_device_wakeup(adev, target_state, wakeup);
-	if (wakeup && error)
-		return error;
+	if (wakeup) {
+		error = acpi_device_wakeup_enable(adev, target_state);
+		if (error)
+			return error;
+	}
 
 	error = acpi_dev_pm_low_power(dev, adev, target_state);
-	if (error)
-		acpi_device_wakeup(adev, ACPI_STATE_UNKNOWN, false);
+	if (error && wakeup)
+		acpi_device_wakeup_disable(adev);
 
 	return error;
 }
@@ -911,7 +948,7 @@ int acpi_dev_resume_early(struct device
 		return 0;
 
 	error = acpi_dev_pm_full_power(adev);
-	acpi_device_wakeup(adev, ACPI_STATE_UNKNOWN, false);
+	acpi_device_wakeup_disable(adev);
 	return error;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_resume_early);
@@ -1054,7 +1091,7 @@ static void acpi_dev_pm_detach(struct de
 			 */
 			dev_pm_qos_hide_latency_limit(dev);
 			dev_pm_qos_hide_flags(dev);
-			acpi_device_wakeup(adev, ACPI_STATE_S0, false);
+			acpi_device_wakeup_disable(adev);
 			acpi_dev_pm_low_power(dev, adev, ACPI_STATE_S0);
 		}
 	}
@@ -1098,7 +1135,7 @@ int acpi_dev_pm_attach(struct device *de
 	dev_pm_domain_set(dev, &acpi_general_pm_domain);
 	if (power_on) {
 		acpi_dev_pm_full_power(adev);
-		acpi_device_wakeup(adev, ACPI_STATE_S0, false);
+		acpi_device_wakeup_disable(adev);
 	}
 
 	dev->pm_domain->detach = acpi_dev_pm_detach;
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -316,7 +316,6 @@ struct acpi_device_perf {
 struct acpi_device_wakeup_flags {
 	u8 valid:1;		/* Can successfully enable wakeup? */
 	u8 notifier_present:1;  /* Wake-up notify handler has been installed */
-	u8 enabled:1;		/* Enabled for wakeup */
 };
 
 struct acpi_device_wakeup_context {
@@ -333,6 +332,7 @@ struct acpi_device_wakeup {
 	struct acpi_device_wakeup_context context;
 	struct wakeup_source *ws;
 	int prepare_count;
+	int enable_count;
 };
 
 struct acpi_device_physical_node {

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Andy Shevchenko]

On Fri, Jul 21, 2017 at 3:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> To prepare for a subsequent change and make the code somewhat easier
> to follow, do the following in the ACPI device wakeup handling code:
>
>  * Replace wakeup.flags.enabled under struct acpi_device with
>    wakeup.enable_count as that will be necessary going forward.
>
>    For now, wakeup.enable_count is not allowed to grow beyond 1,
>    so the current behavior is retained.
>
>  * Split acpi_device_wakeup() into acpi_device_wakeup_enable()
>    and acpi_device_wakeup_disable() and modify the callers of
>    it accordingly.
>
>  * Introduce a new acpi_wakeup_lock mutex to protect the wakeup
>    enabling/disabling code from races in case it is executed
>    more than once in parallel for the same device (which may
>    happen for bridges theoretically).

I prefer more self-explaining labels, though it's minor here

To be constructive:
out -> err_unlock
out -> out_unlock or err_unlock (depends on context)


> +out:
> +       mutex_unlock(&acpi_wakeup_lock);
> +       return error;

> +out:
> +       mutex_unlock(&acpi_wakeup_lock);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Rafael J. Wysocki]

On Friday, July 21, 2017 06:27:39 PM Andy Shevchenko wrote:
> On Fri, Jul 21, 2017 at 3:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > To prepare for a subsequent change and make the code somewhat easier
> > to follow, do the following in the ACPI device wakeup handling code:
> >
> >  * Replace wakeup.flags.enabled under struct acpi_device with
> >    wakeup.enable_count as that will be necessary going forward.
> >
> >    For now, wakeup.enable_count is not allowed to grow beyond 1,
> >    so the current behavior is retained.
> >
> >  * Split acpi_device_wakeup() into acpi_device_wakeup_enable()
> >    and acpi_device_wakeup_disable() and modify the callers of
> >    it accordingly.
> >
> >  * Introduce a new acpi_wakeup_lock mutex to protect the wakeup
> >    enabling/disabling code from races in case it is executed
> >    more than once in parallel for the same device (which may
> >    happen for bridges theoretically).
> 
> I prefer more self-explaining labels, though it's minor here

Well, I prefer shorter ones.

> To be constructive:
> out -> err_unlock
> out -> out_unlock or err_unlock (depends on context)
> 
> 
> > +out:
> > +       mutex_unlock(&acpi_wakeup_lock);
> > +       return error;
> 
> > +out:
> > +       mutex_unlock(&acpi_wakeup_lock);
> 
> 

So while I don't have a particular problem with appending the "_unlock" to the
"out", I'm not exactly sure why this would be an improvement.

If that's just a matter of personal preference, then I would prefer to follow
my personal preference here, with all due respect.  [And besides, it follows
the general style of this file which matters too IMO.]

But if there's more to it, just please let me know. :-)

Thanks,
Rafael

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Andy Shevchenko]

On Fri, Jul 21, 2017 at 11:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, July 21, 2017 06:27:39 PM Andy Shevchenko wrote:
>> On Fri, Jul 21, 2017 at 3:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

>> I prefer more self-explaining labels, though it's minor here
>
> Well, I prefer shorter ones.
>
>> To be constructive:
>> out -> err_unlock
>> out -> out_unlock or err_unlock (depends on context)
>>
>>
>> > +out:
>> > +       mutex_unlock(&acpi_wakeup_lock);
>> > +       return error;
>>
>> > +out:
>> > +       mutex_unlock(&acpi_wakeup_lock);
>>
>>
>
> So while I don't have a particular problem with appending the "_unlock" to the
> "out", I'm not exactly sure why this would be an improvement.
>
> If that's just a matter of personal preference, then I would prefer to follow
> my personal preference here, with all due respect.  [And besides, it follows
> the general style of this file which matters too IMO.]
>
> But if there's more to it, just please let me know. :-)

"Choose label names which say what the goto does or why the goto exists.  An
example of a good name could be ``out_free_buffer:`` if the goto frees
``buffer``.
Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
renumber them if you ever add or remove exit paths, and they make correctness
difficult to verify anyway."

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Rafael J. Wysocki]

On Saturday, July 22, 2017 12:19:51 AM Andy Shevchenko wrote:
> On Fri, Jul 21, 2017 at 11:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, July 21, 2017 06:27:39 PM Andy Shevchenko wrote:
> >> On Fri, Jul 21, 2017 at 3:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> >> I prefer more self-explaining labels, though it's minor here
> >
> > Well, I prefer shorter ones.
> >
> >> To be constructive:
> >> out -> err_unlock
> >> out -> out_unlock or err_unlock (depends on context)
> >>
> >>
> >> > +out:
> >> > +       mutex_unlock(&acpi_wakeup_lock);
> >> > +       return error;
> >>
> >> > +out:
> >> > +       mutex_unlock(&acpi_wakeup_lock);
> >>
> >>
> >
> > So while I don't have a particular problem with appending the "_unlock" to the
> > "out", I'm not exactly sure why this would be an improvement.
> >
> > If that's just a matter of personal preference, then I would prefer to follow
> > my personal preference here, with all due respect.  [And besides, it follows
> > the general style of this file which matters too IMO.]
> >
> > But if there's more to it, just please let me know. :-)
> 
> "Choose label names which say what the goto does or why the goto exists.  An
> example of a good name could be ``out_free_buffer:`` if the goto frees
> ``buffer``.
> Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
> renumber them if you ever add or remove exit paths, and they make correctness
> difficult to verify anyway."

This is a totally made-up argument in this particular case.

Both of the functions in question are 1 screen long and you can *see* what
happens in there without even scrolling them.

Second, the subsequent patch actually *does* add a label to one of the without
renamling the existing one or such.

"out" pretty much represents the purpose of the goto in both cases and making
the label longer doesn't make it any better.

Thanks,
Rafael

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Andy Shevchenko]

On Sat, Jul 22, 2017 at 12:16 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Saturday, July 22, 2017 12:19:51 AM Andy Shevchenko wrote:
>> On Fri, Jul 21, 2017 at 11:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Friday, July 21, 2017 06:27:39 PM Andy Shevchenko wrote:

>> >> I prefer more self-explaining labels, though it's minor here

...

>> > But if there's more to it, just please let me know. :-)
>>
>> "Choose label names which say what the goto does or why the goto exists.  An
>> example of a good name could be ``out_free_buffer:`` if the goto frees
>> ``buffer``.
>> Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
>> renumber them if you ever add or remove exit paths, and they make correctness
>> difficult to verify anyway."
>
> This is a totally made-up argument in this particular case.
>
> Both of the functions in question are 1 screen long and you can *see* what
> happens in there without even scrolling them.
>
> Second, the subsequent patch actually *does* add a label to one of the without
> renamling the existing one or such.
>
> "out" pretty much represents the purpose of the goto in both cases and making
> the label longer doesn't make it any better.

That's why I put "though it's a minor here".

You can read my first message as "you might consider change label
names if you like the idea".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Rafael J. Wysocki]

On Saturday, July 22, 2017 12:31:14 AM Andy Shevchenko wrote:
> On Sat, Jul 22, 2017 at 12:16 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Saturday, July 22, 2017 12:19:51 AM Andy Shevchenko wrote:
> >> On Fri, Jul 21, 2017 at 11:49 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Friday, July 21, 2017 06:27:39 PM Andy Shevchenko wrote:
> 
> >> >> I prefer more self-explaining labels, though it's minor here
> 
> ...
> 
> >> > But if there's more to it, just please let me know. :-)
> >>
> >> "Choose label names which say what the goto does or why the goto exists.  An
> >> example of a good name could be ``out_free_buffer:`` if the goto frees
> >> ``buffer``.
> >> Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
> >> renumber them if you ever add or remove exit paths, and they make correctness
> >> difficult to verify anyway."
> >
> > This is a totally made-up argument in this particular case.
> >
> > Both of the functions in question are 1 screen long and you can *see* what
> > happens in there without even scrolling them.
> >
> > Second, the subsequent patch actually *does* add a label to one of the without
> > renamling the existing one or such.
> >
> > "out" pretty much represents the purpose of the goto in both cases and making
> > the label longer doesn't make it any better.
> 
> That's why I put "though it's a minor here".
> 
> You can read my first message as "you might consider change label
> names if you like the idea".

Fair enough.

I clearly don't like it, then. :-)

Re: [PATCH 2/3] ACPI / PM: Split acpi_device_wakeup()

[Mika Westerberg]

On Fri, Jul 21, 2017 at 02:40:49PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> To prepare for a subsequent change and make the code somewhat easier
> to follow, do the following in the ACPI device wakeup handling code:
> 
>  * Replace wakeup.flags.enabled under struct acpi_device with
>    wakeup.enable_count as that will be necessary going forward.
> 
>    For now, wakeup.enable_count is not allowed to grow beyond 1,
>    so the current behavior is retained.
> 
>  * Split acpi_device_wakeup() into acpi_device_wakeup_enable()
>    and acpi_device_wakeup_disable() and modify the callers of
>    it accordingly.
> 
>  * Introduce a new acpi_wakeup_lock mutex to protect the wakeup
>    enabling/disabling code from races in case it is executed
>    more than once in parallel for the same device (which may
>    happen for bridges theoretically).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

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

The acpi_pci_propagate_wakeup() routine is there to handle cases in
which PCI bridges (or PCIe ports) are expected to signal wakeup
for devices below them, but currently it doesn't do that correctly.

The problem is that acpi_pci_propagate_wakeup() uses
acpi_pm_set_device_wakeup() for bridges and if that routine is
called for multiple times to disable wakeup for the same device,
it will disable it on the first invocation and the next calls
will have no effect (it works analogously when called to enable
wakeup, but that is not a problem).

Now, say acpi_pci_propagate_wakeup() has been called for two
different devices under the same bridge and it has called
acpi_pm_set_device_wakeup() for that bridge each time.  The
bridge is now enabled to generate wakeup signals.  Next,
suppose that one of the devices below it resumes and
acpi_pci_propagate_wakeup() is called to disable wakeup for that
device.  It will then call acpi_pm_set_device_wakeup() for the bridge
and that will effectively disable remote wakeup for all devices under
it even though some of them may still be suspended and remote wakeup
may be expected to work for them.

To address this (arguably theoretical) issue, allow
wakeup.enable_count under struct acpi_device to grow beyond 1 in
certain situations.  In particular, allow that to happen in
acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
for PCI bridges, so that wakeup is actually disabled for the
bridge when all devices under it resume and not when just one
of them does that.

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

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -682,19 +682,8 @@ static void acpi_pm_notify_work_func(str
 
 static DEFINE_MUTEX(acpi_wakeup_lock);
 
-/**
- * acpi_device_wakeup_enable - Enable wakeup functionality for device.
- * @adev: ACPI device to enable wakeup functionality for.
- * @target_state: State the system is transitioning into.
- *
- * Enable the GPE associated with @adev so that it can generate wakeup signals
- * for the device in response to external (remote) events and enable wakeup
- * power for it.
- *
- * Callers must ensure that @adev is a valid ACPI device node before executing
- * this function.
- */
-static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
+static int __acpi_device_wakeup_enable(struct acpi_device *adev,
+				       u32 target_state, int max_count)
 {
 	struct acpi_device_wakeup *wakeup = &adev->wakeup;
 	acpi_status status;
@@ -702,8 +691,12 @@ static int acpi_device_wakeup_enable(str
 
 	mutex_lock(&acpi_wakeup_lock);
 
-	if (wakeup->enable_count > 0)
-		goto out;
+	if (wakeup->enable_count > 0) {
+		if (wakeup->enable_count < max_count)
+			goto inc;
+		else
+			goto out;
+	}
 
 	error = acpi_enable_wakeup_device_power(adev, target_state);
 	if (error)
@@ -716,6 +709,7 @@ static int acpi_device_wakeup_enable(str
 		goto out;
 	}
 
+inc:
 	wakeup->enable_count++;
 
 out:
@@ -724,6 +718,23 @@ out:
 }
 
 /**
+ * acpi_device_wakeup_enable - Enable wakeup functionality for device.
+ * @adev: ACPI device to enable wakeup functionality for.
+ * @target_state: State the system is transitioning into.
+ *
+ * Enable the GPE associated with @adev so that it can generate wakeup signals
+ * for the device in response to external (remote) events and enable wakeup
+ * power for it.
+ *
+ * Callers must ensure that @adev is a valid ACPI device node before executing
+ * this function.
+ */
+static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
+{
+	return __acpi_device_wakeup_enable(adev, target_state, 1);
+}
+
+/**
  * acpi_device_wakeup_disable - Disable wakeup functionality for device.
  * @adev: ACPI device to disable wakeup functionality for.
  *
@@ -754,8 +765,9 @@ out:
  * acpi_pm_set_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.
+ * @max_count: Maximum value of the enable reference counter.
  */
-int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
+int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
 {
 	struct acpi_device *adev;
 	int error;
@@ -775,13 +787,14 @@ int acpi_pm_set_device_wakeup(struct dev
 		return 0;
 	}
 
-	error = acpi_device_wakeup_enable(adev, acpi_target_system_state());
+	error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(),
+					    max_count);
 	if (!error)
 		dev_dbg(dev, "Wakeup enabled by ACPI\n");
 
 	return error;
 }
-EXPORT_SYMBOL(acpi_pm_set_device_wakeup);
+EXPORT_SYMBOL(__acpi_pm_set_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
@@ -605,7 +605,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_set_device_wakeup(struct device *dev, bool enable);
+int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count);
 #else
 static inline void acpi_pm_wakeup_event(struct device *dev)
 {
@@ -632,12 +632,22 @@ 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_set_device_wakeup(struct device *dev, bool enable)
+static inline int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
 {
 	return -ENODEV;
 }
 #endif
 
+static inline int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_set_device_wakeup(dev, enable, 1);
+}
+
+static inline int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX);
+}
+
 #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
@@ -573,7 +573,7 @@ static int acpi_pci_propagate_wakeup(str
 {
 	while (bus->parent) {
 		if (acpi_pm_device_can_wakeup(&bus->self->dev))
-			return acpi_pm_set_device_wakeup(&bus->self->dev, enable);
+			return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);
 
 		bus = bus->parent;
 	}
@@ -581,7 +581,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_set_device_wakeup(bus->bridge, enable);
+			return acpi_pm_set_bridge_wakeup(bus->bridge, enable);
 	}
 	return 0;
 }

Re: [PATCH 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Andy Shevchenko]

On Fri, Jul 21, 2017 at 3:42 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> The acpi_pci_propagate_wakeup() routine is there to handle cases in
> which PCI bridges (or PCIe ports) are expected to signal wakeup
> for devices below them, but currently it doesn't do that correctly.
>
> The problem is that acpi_pci_propagate_wakeup() uses
> acpi_pm_set_device_wakeup() for bridges and if that routine is
> called for multiple times to disable wakeup for the same device,
> it will disable it on the first invocation and the next calls
> will have no effect (it works analogously when called to enable
> wakeup, but that is not a problem).
>
> Now, say acpi_pci_propagate_wakeup() has been called for two
> different devices under the same bridge and it has called
> acpi_pm_set_device_wakeup() for that bridge each time.  The
> bridge is now enabled to generate wakeup signals.  Next,
> suppose that one of the devices below it resumes and
> acpi_pci_propagate_wakeup() is called to disable wakeup for that
> device.  It will then call acpi_pm_set_device_wakeup() for the bridge
> and that will effectively disable remote wakeup for all devices under
> it even though some of them may still be suspended and remote wakeup
> may be expected to work for them.
>
> To address this (arguably theoretical) issue, allow
> wakeup.enable_count under struct acpi_device to grow beyond 1 in
> certain situations.  In particular, allow that to happen in
> acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
> for PCI bridges, so that wakeup is actually disabled for the
> bridge when all devices under it resume and not when just one
> of them does that.

> -       if (wakeup->enable_count > 0)
> -               goto out;
> +       if (wakeup->enable_count > 0) {
> +               if (wakeup->enable_count < max_count)
> +                       goto inc;
> +               else
> +                       goto out;
> +       }

Wouldn't be simpler

    if (wakeup->enable_count >= max_count)
      goto out;

    if (wakeup->enable_count > 0)
      goto inc;

If max_count can be <= 0,

    if (max_count > 0 && wakeup->enable_count >= max_count)
      goto out;


> +inc:
>         wakeup->enable_count++;
>
>  out:



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

On Friday, July 21, 2017 06:45:03 PM Andy Shevchenko wrote:
> On Fri, Jul 21, 2017 at 3:42 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> > The acpi_pci_propagate_wakeup() routine is there to handle cases in
> > which PCI bridges (or PCIe ports) are expected to signal wakeup
> > for devices below them, but currently it doesn't do that correctly.
> >
> > The problem is that acpi_pci_propagate_wakeup() uses
> > acpi_pm_set_device_wakeup() for bridges and if that routine is
> > called for multiple times to disable wakeup for the same device,
> > it will disable it on the first invocation and the next calls
> > will have no effect (it works analogously when called to enable
> > wakeup, but that is not a problem).
> >
> > Now, say acpi_pci_propagate_wakeup() has been called for two
> > different devices under the same bridge and it has called
> > acpi_pm_set_device_wakeup() for that bridge each time.  The
> > bridge is now enabled to generate wakeup signals.  Next,
> > suppose that one of the devices below it resumes and
> > acpi_pci_propagate_wakeup() is called to disable wakeup for that
> > device.  It will then call acpi_pm_set_device_wakeup() for the bridge
> > and that will effectively disable remote wakeup for all devices under
> > it even though some of them may still be suspended and remote wakeup
> > may be expected to work for them.
> >
> > To address this (arguably theoretical) issue, allow
> > wakeup.enable_count under struct acpi_device to grow beyond 1 in
> > certain situations.  In particular, allow that to happen in
> > acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
> > for PCI bridges, so that wakeup is actually disabled for the
> > bridge when all devices under it resume and not when just one
> > of them does that.
> 
> > -       if (wakeup->enable_count > 0)
> > -               goto out;
> > +       if (wakeup->enable_count > 0) {
> > +               if (wakeup->enable_count < max_count)
> > +                       goto inc;
> > +               else
> > +                       goto out;
> > +       }
> 
> Wouldn't be simpler

I'm not really sure what you mean.

In general, ->

>     if (wakeup->enable_count >= max_count)
>       goto out;

-> this is unlikely and ->>

>     if (wakeup->enable_count > 0)
>       goto inc;

->> this isn't.

Why would checking an unlikely condition before a likely one covering it
ever be better?

> If max_count can be <= 0,

No, it can't be.

Thanks,
Rafael

Re: [PATCH 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

On Friday, July 21, 2017 10:44:30 PM Rafael J. Wysocki wrote:
> On Friday, July 21, 2017 06:45:03 PM Andy Shevchenko wrote:
> > On Fri, Jul 21, 2017 at 3:42 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > 
> > > The acpi_pci_propagate_wakeup() routine is there to handle cases in
> > > which PCI bridges (or PCIe ports) are expected to signal wakeup
> > > for devices below them, but currently it doesn't do that correctly.
> > >
> > > The problem is that acpi_pci_propagate_wakeup() uses
> > > acpi_pm_set_device_wakeup() for bridges and if that routine is
> > > called for multiple times to disable wakeup for the same device,
> > > it will disable it on the first invocation and the next calls
> > > will have no effect (it works analogously when called to enable
> > > wakeup, but that is not a problem).
> > >
> > > Now, say acpi_pci_propagate_wakeup() has been called for two
> > > different devices under the same bridge and it has called
> > > acpi_pm_set_device_wakeup() for that bridge each time.  The
> > > bridge is now enabled to generate wakeup signals.  Next,
> > > suppose that one of the devices below it resumes and
> > > acpi_pci_propagate_wakeup() is called to disable wakeup for that
> > > device.  It will then call acpi_pm_set_device_wakeup() for the bridge
> > > and that will effectively disable remote wakeup for all devices under
> > > it even though some of them may still be suspended and remote wakeup
> > > may be expected to work for them.
> > >
> > > To address this (arguably theoretical) issue, allow
> > > wakeup.enable_count under struct acpi_device to grow beyond 1 in
> > > certain situations.  In particular, allow that to happen in
> > > acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
> > > for PCI bridges, so that wakeup is actually disabled for the
> > > bridge when all devices under it resume and not when just one
> > > of them does that.
> > 
> > > -       if (wakeup->enable_count > 0)
> > > -               goto out;
> > > +       if (wakeup->enable_count > 0) {
> > > +               if (wakeup->enable_count < max_count)
> > > +                       goto inc;
> > > +               else
> > > +                       goto out;
> > > +       }
> > 
> > Wouldn't be simpler
> 
> I'm not really sure what you mean.
> 
> In general, ->
> 
> >     if (wakeup->enable_count >= max_count)
> >       goto out;
> 
> -> this is unlikely and ->>
> 
> >     if (wakeup->enable_count > 0)
> >       goto inc;
> 
> ->> this isn't.
> 
> Why would checking an unlikely condition before a likely one covering it
> ever be better?

OK, the common case is max_cout == 1 and it that case
enable_count >= max_count is equivalent to enable_count > 0,
so I guess fair enough.

Thanks,
Rafael

[PATCH v2 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

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

The acpi_pci_propagate_wakeup() routine is there to handle cases in
which PCI bridges (or PCIe ports) are expected to signal wakeup
for devices below them, but currently it doesn't do that correctly.

The problem is that acpi_pci_propagate_wakeup() uses
acpi_pm_set_device_wakeup() for bridges and if that routine is
called for multiple times to disable wakeup for the same device,
it will disable it on the first invocation and the next calls
will have no effect (it works analogously when called to enable
wakeup, but that is not a problem).

Now, say acpi_pci_propagate_wakeup() has been called for two
different devices under the same bridge and it has called
acpi_pm_set_device_wakeup() for that bridge each time.  The
bridge is now enabled to generate wakeup signals.  Next,
suppose that one of the devices below it resumes and
acpi_pci_propagate_wakeup() is called to disable wakeup for that
device.  It will then call acpi_pm_set_device_wakeup() for the bridge
and that will effectively disable remote wakeup for all devices under
it even though some of them may still be suspended and remote wakeup
may be expected to work for them.

To address this (arguably theoretical) issue, allow
wakeup.enable_count under struct acpi_device to grow beyond 1 in
certain situations.  In particular, allow that to happen in
acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
for PCI bridges, so that wakeup is actually disabled for the
bridge when all devices under it resume and not when just one
of them does that.

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

-> v2: Rearrange checks in acpi_device_wakeup_enable() to reduce indentation
          level and possibly save some unnecessary checks for max_count == 1.

---
 drivers/acpi/device_pm.c |   46 +++++++++++++++++++++++++++++-----------------
 drivers/pci/pci-acpi.c   |    4 ++--
 include/acpi/acpi_bus.h  |   14 ++++++++++++--
 3 files changed, 43 insertions(+), 21 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -682,19 +682,8 @@ static void acpi_pm_notify_work_func(str
 
 static DEFINE_MUTEX(acpi_wakeup_lock);
 
-/**
- * acpi_device_wakeup_enable - Enable wakeup functionality for device.
- * @adev: ACPI device to enable wakeup functionality for.
- * @target_state: State the system is transitioning into.
- *
- * Enable the GPE associated with @adev so that it can generate wakeup signals
- * for the device in response to external (remote) events and enable wakeup
- * power for it.
- *
- * Callers must ensure that @adev is a valid ACPI device node before executing
- * this function.
- */
-static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
+static int __acpi_device_wakeup_enable(struct acpi_device *adev,
+				       u32 target_state, int max_count)
 {
 	struct acpi_device_wakeup *wakeup = &adev->wakeup;
 	acpi_status status;
@@ -702,9 +691,12 @@ static int acpi_device_wakeup_enable(str
 
 	mutex_lock(&acpi_wakeup_lock);
 
-	if (wakeup->enable_count > 0)
+	if (wakeup->enable_count >= max_count)
 		goto out;
 
+	if (wakeup->enable_count > 0)
+		goto inc;
+
 	error = acpi_enable_wakeup_device_power(adev, target_state);
 	if (error)
 		goto out;
@@ -716,6 +708,7 @@ static int acpi_device_wakeup_enable(str
 		goto out;
 	}
 
+inc:
 	wakeup->enable_count++;
 
 out:
@@ -724,6 +717,23 @@ out:
 }
 
 /**
+ * acpi_device_wakeup_enable - Enable wakeup functionality for device.
+ * @adev: ACPI device to enable wakeup functionality for.
+ * @target_state: State the system is transitioning into.
+ *
+ * Enable the GPE associated with @adev so that it can generate wakeup signals
+ * for the device in response to external (remote) events and enable wakeup
+ * power for it.
+ *
+ * Callers must ensure that @adev is a valid ACPI device node before executing
+ * this function.
+ */
+static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
+{
+	return __acpi_device_wakeup_enable(adev, target_state, 1);
+}
+
+/**
  * acpi_device_wakeup_disable - Disable wakeup functionality for device.
  * @adev: ACPI device to disable wakeup functionality for.
  *
@@ -754,8 +764,9 @@ out:
  * acpi_pm_set_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.
+ * @max_count: Maximum value of the enable reference counter.
  */
-int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
+int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
 {
 	struct acpi_device *adev;
 	int error;
@@ -775,13 +786,14 @@ int acpi_pm_set_device_wakeup(struct dev
 		return 0;
 	}
 
-	error = acpi_device_wakeup_enable(adev, acpi_target_system_state());
+	error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(),
+					    max_count);
 	if (!error)
 		dev_dbg(dev, "Wakeup enabled by ACPI\n");
 
 	return error;
 }
-EXPORT_SYMBOL(acpi_pm_set_device_wakeup);
+EXPORT_SYMBOL(__acpi_pm_set_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
@@ -605,7 +605,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_set_device_wakeup(struct device *dev, bool enable);
+int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count);
 #else
 static inline void acpi_pm_wakeup_event(struct device *dev)
 {
@@ -632,12 +632,22 @@ 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_set_device_wakeup(struct device *dev, bool enable)
+static inline int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
 {
 	return -ENODEV;
 }
 #endif
 
+static inline int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_set_device_wakeup(dev, enable, 1);
+}
+
+static inline int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX);
+}
+
 #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
@@ -573,7 +573,7 @@ static int acpi_pci_propagate_wakeup(str
 {
 	while (bus->parent) {
 		if (acpi_pm_device_can_wakeup(&bus->self->dev))
-			return acpi_pm_set_device_wakeup(&bus->self->dev, enable);
+			return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);
 
 		bus = bus->parent;
 	}
@@ -581,7 +581,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_set_device_wakeup(bus->bridge, enable);
+			return acpi_pm_set_bridge_wakeup(bus->bridge, enable);
 	}
 	return 0;
 }

Re: [PATCH v2 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Andy Shevchenko]

On Sat, Jul 22, 2017 at 12:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The acpi_pci_propagate_wakeup() routine is there to handle cases in
> which PCI bridges (or PCIe ports) are expected to signal wakeup
> for devices below them, but currently it doesn't do that correctly.
>
> The problem is that acpi_pci_propagate_wakeup() uses
> acpi_pm_set_device_wakeup() for bridges and if that routine is
> called for multiple times to disable wakeup for the same device,
> it will disable it on the first invocation and the next calls
> will have no effect (it works analogously when called to enable
> wakeup, but that is not a problem).
>
> Now, say acpi_pci_propagate_wakeup() has been called for two
> different devices under the same bridge and it has called
> acpi_pm_set_device_wakeup() for that bridge each time.  The
> bridge is now enabled to generate wakeup signals.  Next,
> suppose that one of the devices below it resumes and
> acpi_pci_propagate_wakeup() is called to disable wakeup for that
> device.  It will then call acpi_pm_set_device_wakeup() for the bridge
> and that will effectively disable remote wakeup for all devices under
> it even though some of them may still be suspended and remote wakeup
> may be expected to work for them.
>
> To address this (arguably theoretical) issue, allow
> wakeup.enable_count under struct acpi_device to grow beyond 1 in
> certain situations.  In particular, allow that to happen in
> acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
> for PCI bridges, so that wakeup is actually disabled for the
> bridge when all devices under it resume and not when just one
> of them does that.

Thanks  for an update! At least to me it's now looks better.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> -> v2: Rearrange checks in acpi_device_wakeup_enable() to reduce indentation
>           level and possibly save some unnecessary checks for max_count == 1.
>
> ---
>  drivers/acpi/device_pm.c |   46 +++++++++++++++++++++++++++++-----------------
>  drivers/pci/pci-acpi.c   |    4 ++--
>  include/acpi/acpi_bus.h  |   14 ++++++++++++--
>  3 files changed, 43 insertions(+), 21 deletions(-)
>
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -682,19 +682,8 @@ static void acpi_pm_notify_work_func(str
>
>  static DEFINE_MUTEX(acpi_wakeup_lock);
>
> -/**
> - * acpi_device_wakeup_enable - Enable wakeup functionality for device.
> - * @adev: ACPI device to enable wakeup functionality for.
> - * @target_state: State the system is transitioning into.
> - *
> - * Enable the GPE associated with @adev so that it can generate wakeup signals
> - * for the device in response to external (remote) events and enable wakeup
> - * power for it.
> - *
> - * Callers must ensure that @adev is a valid ACPI device node before executing
> - * this function.
> - */
> -static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
> +static int __acpi_device_wakeup_enable(struct acpi_device *adev,
> +                                      u32 target_state, int max_count)
>  {
>         struct acpi_device_wakeup *wakeup = &adev->wakeup;
>         acpi_status status;
> @@ -702,9 +691,12 @@ static int acpi_device_wakeup_enable(str
>
>         mutex_lock(&acpi_wakeup_lock);
>
> -       if (wakeup->enable_count > 0)
> +       if (wakeup->enable_count >= max_count)
>                 goto out;
>
> +       if (wakeup->enable_count > 0)
> +               goto inc;
> +
>         error = acpi_enable_wakeup_device_power(adev, target_state);
>         if (error)
>                 goto out;
> @@ -716,6 +708,7 @@ static int acpi_device_wakeup_enable(str
>                 goto out;
>         }
>
> +inc:
>         wakeup->enable_count++;
>
>  out:
> @@ -724,6 +717,23 @@ out:
>  }
>
>  /**
> + * acpi_device_wakeup_enable - Enable wakeup functionality for device.
> + * @adev: ACPI device to enable wakeup functionality for.
> + * @target_state: State the system is transitioning into.
> + *
> + * Enable the GPE associated with @adev so that it can generate wakeup signals
> + * for the device in response to external (remote) events and enable wakeup
> + * power for it.
> + *
> + * Callers must ensure that @adev is a valid ACPI device node before executing
> + * this function.
> + */
> +static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
> +{
> +       return __acpi_device_wakeup_enable(adev, target_state, 1);
> +}
> +
> +/**
>   * acpi_device_wakeup_disable - Disable wakeup functionality for device.
>   * @adev: ACPI device to disable wakeup functionality for.
>   *
> @@ -754,8 +764,9 @@ out:
>   * acpi_pm_set_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.
> + * @max_count: Maximum value of the enable reference counter.
>   */
> -int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
> +int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
>  {
>         struct acpi_device *adev;
>         int error;
> @@ -775,13 +786,14 @@ int acpi_pm_set_device_wakeup(struct dev
>                 return 0;
>         }
>
> -       error = acpi_device_wakeup_enable(adev, acpi_target_system_state());
> +       error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(),
> +                                           max_count);
>         if (!error)
>                 dev_dbg(dev, "Wakeup enabled by ACPI\n");
>
>         return error;
>  }
> -EXPORT_SYMBOL(acpi_pm_set_device_wakeup);
> +EXPORT_SYMBOL(__acpi_pm_set_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
> @@ -605,7 +605,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_set_device_wakeup(struct device *dev, bool enable);
> +int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count);
>  #else
>  static inline void acpi_pm_wakeup_event(struct device *dev)
>  {
> @@ -632,12 +632,22 @@ 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_set_device_wakeup(struct device *dev, bool enable)
> +static inline int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
>  {
>         return -ENODEV;
>  }
>  #endif
>
> +static inline int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
> +{
> +       return __acpi_pm_set_device_wakeup(dev, enable, 1);
> +}
> +
> +static inline int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
> +{
> +       return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX);
> +}
> +
>  #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
> @@ -573,7 +573,7 @@ static int acpi_pci_propagate_wakeup(str
>  {
>         while (bus->parent) {
>                 if (acpi_pm_device_can_wakeup(&bus->self->dev))
> -                       return acpi_pm_set_device_wakeup(&bus->self->dev, enable);
> +                       return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);
>
>                 bus = bus->parent;
>         }
> @@ -581,7 +581,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_set_device_wakeup(bus->bridge, enable);
> +                       return acpi_pm_set_bridge_wakeup(bus->bridge, enable);
>         }
>         return 0;
>  }
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Mika Westerberg]

On Fri, Jul 21, 2017 at 11:30:24PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The acpi_pci_propagate_wakeup() routine is there to handle cases in
> which PCI bridges (or PCIe ports) are expected to signal wakeup
> for devices below them, but currently it doesn't do that correctly.
> 
> The problem is that acpi_pci_propagate_wakeup() uses
> acpi_pm_set_device_wakeup() for bridges and if that routine is
> called for multiple times to disable wakeup for the same device,
> it will disable it on the first invocation and the next calls
> will have no effect (it works analogously when called to enable
> wakeup, but that is not a problem).
> 
> Now, say acpi_pci_propagate_wakeup() has been called for two
> different devices under the same bridge and it has called
> acpi_pm_set_device_wakeup() for that bridge each time.  The
> bridge is now enabled to generate wakeup signals.  Next,
> suppose that one of the devices below it resumes and
> acpi_pci_propagate_wakeup() is called to disable wakeup for that
> device.  It will then call acpi_pm_set_device_wakeup() for the bridge
> and that will effectively disable remote wakeup for all devices under
> it even though some of them may still be suspended and remote wakeup
> may be expected to work for them.
> 
> To address this (arguably theoretical) issue, allow
> wakeup.enable_count under struct acpi_device to grow beyond 1 in
> certain situations.  In particular, allow that to happen in
> acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
> for PCI bridges, so that wakeup is actually disabled for the
> bridge when all devices under it resume and not when just one
> of them does that.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Bjorn Helgaas]

On Fri, Jul 21, 2017 at 11:30:24PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The acpi_pci_propagate_wakeup() routine is there to handle cases in
> which PCI bridges (or PCIe ports) are expected to signal wakeup
> for devices below them, but currently it doesn't do that correctly.
> 
> The problem is that acpi_pci_propagate_wakeup() uses
> acpi_pm_set_device_wakeup() for bridges and if that routine is
> called for multiple times to disable wakeup for the same device,
> it will disable it on the first invocation and the next calls
> will have no effect (it works analogously when called to enable
> wakeup, but that is not a problem).
> 
> Now, say acpi_pci_propagate_wakeup() has been called for two
> different devices under the same bridge and it has called
> acpi_pm_set_device_wakeup() for that bridge each time.  The
> bridge is now enabled to generate wakeup signals.  Next,
> suppose that one of the devices below it resumes and
> acpi_pci_propagate_wakeup() is called to disable wakeup for that
> device.  It will then call acpi_pm_set_device_wakeup() for the bridge
> and that will effectively disable remote wakeup for all devices under
> it even though some of them may still be suspended and remote wakeup
> may be expected to work for them.
> 
> To address this (arguably theoretical) issue, allow
> wakeup.enable_count under struct acpi_device to grow beyond 1 in
> certain situations.  In particular, allow that to happen in
> acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
> for PCI bridges, so that wakeup is actually disabled for the
> bridge when all devices under it resume and not when just one
> of them does that.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

But see questions below, stemming from my ignorance of ACPI PM.  I
don't want to start a discussion and delay this.  If my questions
don't suggest any useful changes, please ignore them.

> ---
> 
> -> v2: Rearrange checks in acpi_device_wakeup_enable() to reduce indentation
>           level and possibly save some unnecessary checks for max_count == 1.
> 
> ---
>  drivers/acpi/device_pm.c |   46 +++++++++++++++++++++++++++++-----------------
>  drivers/pci/pci-acpi.c   |    4 ++--
>  include/acpi/acpi_bus.h  |   14 ++++++++++++--
>  3 files changed, 43 insertions(+), 21 deletions(-)
> 
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -682,19 +682,8 @@ static void acpi_pm_notify_work_func(str
>  
>  static DEFINE_MUTEX(acpi_wakeup_lock);
>  
> -/**
> - * acpi_device_wakeup_enable - Enable wakeup functionality for device.
> - * @adev: ACPI device to enable wakeup functionality for.
> - * @target_state: State the system is transitioning into.
> - *
> - * Enable the GPE associated with @adev so that it can generate wakeup signals
> - * for the device in response to external (remote) events and enable wakeup
> - * power for it.
> - *
> - * Callers must ensure that @adev is a valid ACPI device node before executing
> - * this function.
> - */
> -static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
> +static int __acpi_device_wakeup_enable(struct acpi_device *adev,
> +				       u32 target_state, int max_count)

It looks like @max_count is always either 1 or INT_MAX, so it's
effectively a boolean.  Is there a way to interpret @max_count in
terms of the PCI topology?  It's obviously related to
wakeup->enable_count; does wakeup->enable_count basically mean "the
number of subordinate devices that are enabled to generate wakeups"?

__acpi_pm_set_device_wakeup() is exported; maybe only to make the
inlined callers in acpi_bus.h work?  It might be worth un-inlining
them to avoid having to export __acpi_pm_set_device_wakeup().  I'm
just thinking that exporting it makes it visible everywhere, and
@max_count seems like an internal detail that's hard to document for
use by non-core code.

I guess you still have to export both acpi_pm_set_device_wakeup()
(currently already exported) and acpi_pm_set_bridge_wakeup()
(this patch effectively exports it by implementing it as an inline
function).

It'd be nice if we could distinguish the device case from the bridge
case inside acpi_pm_set_device_wakeup() so we wouldn't have to rely on
the caller using the correct one.  But I assume you already considered
that and found it impractical.

>  {
>  	struct acpi_device_wakeup *wakeup = &adev->wakeup;
>  	acpi_status status;
> @@ -702,9 +691,12 @@ static int acpi_device_wakeup_enable(str
>  
>  	mutex_lock(&acpi_wakeup_lock);
>  
> -	if (wakeup->enable_count > 0)
> +	if (wakeup->enable_count >= max_count)
>  		goto out;
>  
> +	if (wakeup->enable_count > 0)
> +		goto inc;
> +
>  	error = acpi_enable_wakeup_device_power(adev, target_state);
>  	if (error)
>  		goto out;
> @@ -716,6 +708,7 @@ static int acpi_device_wakeup_enable(str
>  		goto out;
>  	}
>  
> +inc:
>  	wakeup->enable_count++;
>  
>  out:
> @@ -724,6 +717,23 @@ out:
>  }
>  
>  /**
> + * acpi_device_wakeup_enable - Enable wakeup functionality for device.
> + * @adev: ACPI device to enable wakeup functionality for.
> + * @target_state: State the system is transitioning into.
> + *
> + * Enable the GPE associated with @adev so that it can generate wakeup signals
> + * for the device in response to external (remote) events and enable wakeup
> + * power for it.
> + *
> + * Callers must ensure that @adev is a valid ACPI device node before executing
> + * this function.
> + */
> +static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
> +{
> +	return __acpi_device_wakeup_enable(adev, target_state, 1);
> +}
> +
> +/**
>   * acpi_device_wakeup_disable - Disable wakeup functionality for device.
>   * @adev: ACPI device to disable wakeup functionality for.
>   *
> @@ -754,8 +764,9 @@ out:
>   * acpi_pm_set_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.
> + * @max_count: Maximum value of the enable reference counter.
>   */
> -int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
> +int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
>  {
>  	struct acpi_device *adev;
>  	int error;
> @@ -775,13 +786,14 @@ int acpi_pm_set_device_wakeup(struct dev
>  		return 0;
>  	}
>  
> -	error = acpi_device_wakeup_enable(adev, acpi_target_system_state());
> +	error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(),
> +					    max_count);
>  	if (!error)
>  		dev_dbg(dev, "Wakeup enabled by ACPI\n");
>  
>  	return error;
>  }
> -EXPORT_SYMBOL(acpi_pm_set_device_wakeup);
> +EXPORT_SYMBOL(__acpi_pm_set_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
> @@ -605,7 +605,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_set_device_wakeup(struct device *dev, bool enable);
> +int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count);
>  #else
>  static inline void acpi_pm_wakeup_event(struct device *dev)
>  {
> @@ -632,12 +632,22 @@ 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_set_device_wakeup(struct device *dev, bool enable)
> +static inline int __acpi_pm_set_device_wakeup(struct device *dev, bool enable, int max_count)
>  {
>  	return -ENODEV;
>  }
>  #endif
>  
> +static inline int acpi_pm_set_device_wakeup(struct device *dev, bool enable)
> +{
> +	return __acpi_pm_set_device_wakeup(dev, enable, 1);
> +}
> +
> +static inline int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
> +{
> +	return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX);
> +}
> +
>  #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
> @@ -573,7 +573,7 @@ static int acpi_pci_propagate_wakeup(str
>  {
>  	while (bus->parent) {
>  		if (acpi_pm_device_can_wakeup(&bus->self->dev))
> -			return acpi_pm_set_device_wakeup(&bus->self->dev, enable);
> +			return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);
>  
>  		bus = bus->parent;
>  	}
> @@ -581,7 +581,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_set_device_wakeup(bus->bridge, enable);
> +			return acpi_pm_set_bridge_wakeup(bus->bridge, enable);
>  	}
>  	return 0;
>  }
> 

Re: [PATCH v2 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

On Mon, Jul 31, 2017 at 11:59 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Jul 21, 2017 at 11:30:24PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> The acpi_pci_propagate_wakeup() routine is there to handle cases in
>> which PCI bridges (or PCIe ports) are expected to signal wakeup
>> for devices below them, but currently it doesn't do that correctly.
>>
>> The problem is that acpi_pci_propagate_wakeup() uses
>> acpi_pm_set_device_wakeup() for bridges and if that routine is
>> called for multiple times to disable wakeup for the same device,
>> it will disable it on the first invocation and the next calls
>> will have no effect (it works analogously when called to enable
>> wakeup, but that is not a problem).
>>
>> Now, say acpi_pci_propagate_wakeup() has been called for two
>> different devices under the same bridge and it has called
>> acpi_pm_set_device_wakeup() for that bridge each time.  The
>> bridge is now enabled to generate wakeup signals.  Next,
>> suppose that one of the devices below it resumes and
>> acpi_pci_propagate_wakeup() is called to disable wakeup for that
>> device.  It will then call acpi_pm_set_device_wakeup() for the bridge
>> and that will effectively disable remote wakeup for all devices under
>> it even though some of them may still be suspended and remote wakeup
>> may be expected to work for them.
>>
>> To address this (arguably theoretical) issue, allow
>> wakeup.enable_count under struct acpi_device to grow beyond 1 in
>> certain situations.  In particular, allow that to happen in
>> acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
>> for PCI bridges, so that wakeup is actually disabled for the
>> bridge when all devices under it resume and not when just one
>> of them does that.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks!

> But see questions below, stemming from my ignorance of ACPI PM.  I
> don't want to start a discussion and delay this.  If my questions
> don't suggest any useful changes, please ignore them.
>
>> ---
>>
>> -> v2: Rearrange checks in acpi_device_wakeup_enable() to reduce indentation
>>           level and possibly save some unnecessary checks for max_count == 1.
>>
>> ---
>>  drivers/acpi/device_pm.c |   46 +++++++++++++++++++++++++++++-----------------
>>  drivers/pci/pci-acpi.c   |    4 ++--
>>  include/acpi/acpi_bus.h  |   14 ++++++++++++--
>>  3 files changed, 43 insertions(+), 21 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/device_pm.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/device_pm.c
>> +++ linux-pm/drivers/acpi/device_pm.c
>> @@ -682,19 +682,8 @@ static void acpi_pm_notify_work_func(str
>>
>>  static DEFINE_MUTEX(acpi_wakeup_lock);
>>
>> -/**
>> - * acpi_device_wakeup_enable - Enable wakeup functionality for device.
>> - * @adev: ACPI device to enable wakeup functionality for.
>> - * @target_state: State the system is transitioning into.
>> - *
>> - * Enable the GPE associated with @adev so that it can generate wakeup signals
>> - * for the device in response to external (remote) events and enable wakeup
>> - * power for it.
>> - *
>> - * Callers must ensure that @adev is a valid ACPI device node before executing
>> - * this function.
>> - */
>> -static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
>> +static int __acpi_device_wakeup_enable(struct acpi_device *adev,
>> +                                    u32 target_state, int max_count)
>
> It looks like @max_count is always either 1 or INT_MAX, so it's
> effectively a boolean.  Is there a way to interpret @max_count in
> terms of the PCI topology?  It's obviously related to
> wakeup->enable_count; does wakeup->enable_count basically mean "the
> number of subordinate devices that are enabled to generate wakeups"?

Yes, it does, for bridges.

> __acpi_pm_set_device_wakeup() is exported; maybe only to make the
> inlined callers in acpi_bus.h work?  It might be worth un-inlining
> them to avoid having to export __acpi_pm_set_device_wakeup().  I'm
> just thinking that exporting it makes it visible everywhere, and
> @max_count seems like an internal detail that's hard to document for
> use by non-core code.

I can do that.

> I guess you still have to export both acpi_pm_set_device_wakeup()
> (currently already exported) and acpi_pm_set_bridge_wakeup()
> (this patch effectively exports it by implementing it as an inline
> function).
>
> It'd be nice if we could distinguish the device case from the bridge
> case inside acpi_pm_set_device_wakeup() so we wouldn't have to rely on
> the caller using the correct one.  But I assume you already considered
> that and found it impractical.

That's correct.

[PATCH v3 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

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

The acpi_pci_propagate_wakeup() routine is there to handle cases in
which PCI bridges (or PCIe ports) are expected to signal wakeup
for devices below them, but currently it doesn't do that correctly.

The problem is that acpi_pci_propagate_wakeup() uses
acpi_pm_set_device_wakeup() for bridges and if that routine is
called for multiple times to disable wakeup for the same device,
it will disable it on the first invocation and the next calls
will have no effect (it works analogously when called to enable
wakeup, but that is not a problem).

Now, say acpi_pci_propagate_wakeup() has been called for two
different devices under the same bridge and it has called
acpi_pm_set_device_wakeup() for that bridge each time.  The
bridge is now enabled to generate wakeup signals.  Next,
suppose that one of the devices below it resumes and
acpi_pci_propagate_wakeup() is called to disable wakeup for that
device.  It will then call acpi_pm_set_device_wakeup() for the bridge
and that will effectively disable remote wakeup for all devices under
it even though some of them may still be suspended and remote wakeup
may be expected to work for them.

To address this (arguably theoretical) issue, allow
wakeup.enable_count under struct acpi_device to grow beyond 1 in
certain situations.  In particular, allow that to happen in
acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
for PCI bridges, so that wakeup is actually disabled for the
bridge when all devices under it resume and not when just one
of them does that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---

v2 -> v3: Follow the Bjorn's suggestion to un-inline acpi_pm_set_device_wakeup()
  and acpi_pm_set_bridge_wakeup() to avoid exporting __acpi_pm_set_device_wakeup()
  whose max_count argument is too much of an internal detail to be documented
  for use in non-core code clearly enough.

I've retained the tags, because the changes are minor and don't affect the
behavior, but if (some of all) the tags are not applicabe any more, please let
me know. :-)

---
 drivers/acpi/device_pm.c |   72 ++++++++++++++++++++++++++++++++---------------
 drivers/pci/pci-acpi.c   |    4 +-
 include/acpi/acpi_bus.h  |    5 +++
 3 files changed, 57 insertions(+), 24 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -684,19 +684,8 @@ static void acpi_pm_notify_work_func(str
 
 static DEFINE_MUTEX(acpi_wakeup_lock);
 
-/**
- * acpi_device_wakeup_enable - Enable wakeup functionality for device.
- * @adev: ACPI device to enable wakeup functionality for.
- * @target_state: State the system is transitioning into.
- *
- * Enable the GPE associated with @adev so that it can generate wakeup signals
- * for the device in response to external (remote) events and enable wakeup
- * power for it.
- *
- * Callers must ensure that @adev is a valid ACPI device node before executing
- * this function.
- */
-static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
+static int __acpi_device_wakeup_enable(struct acpi_device *adev,
+				       u32 target_state, int max_count)
 {
 	struct acpi_device_wakeup *wakeup = &adev->wakeup;
 	acpi_status status;
@@ -704,9 +693,12 @@ static int acpi_device_wakeup_enable(str
 
 	mutex_lock(&acpi_wakeup_lock);
 
-	if (wakeup->enable_count > 0)
+	if (wakeup->enable_count >= max_count)
 		goto out;
 
+	if (wakeup->enable_count > 0)
+		goto inc;
+
 	error = acpi_enable_wakeup_device_power(adev, target_state);
 	if (error)
 		goto out;
@@ -718,6 +710,7 @@ static int acpi_device_wakeup_enable(str
 		goto out;
 	}
 
+inc:
 	wakeup->enable_count++;
 
 out:
@@ -726,6 +719,23 @@ out:
 }
 
 /**
+ * acpi_device_wakeup_enable - Enable wakeup functionality for device.
+ * @adev: ACPI device to enable wakeup functionality for.
+ * @target_state: State the system is transitioning into.
+ *
+ * Enable the GPE associated with @adev so that it can generate wakeup signals
+ * for the device in response to external (remote) events and enable wakeup
+ * power for it.
+ *
+ * Callers must ensure that @adev is a valid ACPI device node before executing
+ * this function.
+ */
+static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
+{
+	return __acpi_device_wakeup_enable(adev, target_state, 1);
+}
+
+/**
  * acpi_device_wakeup_disable - Disable wakeup functionality for device.
  * @adev: ACPI device to disable wakeup functionality for.
  *
@@ -752,12 +762,8 @@ out:
 	mutex_unlock(&acpi_wakeup_lock);
 }
 
-/**
- * acpi_pm_set_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_set_device_wakeup(struct device *dev, bool enable)
+static int __acpi_pm_set_device_wakeup(struct device *dev, bool enable,
+				       int max_count)
 {
 	struct acpi_device *adev;
 	int error;
@@ -777,13 +783,35 @@ int acpi_pm_set_device_wakeup(struct dev
 		return 0;
 	}
 
-	error = acpi_device_wakeup_enable(adev, acpi_target_system_state());
+	error = __acpi_device_wakeup_enable(adev, acpi_target_system_state(),
+					    max_count);
 	if (!error)
 		dev_dbg(dev, "Wakeup enabled by ACPI\n");
 
 	return error;
 }
-EXPORT_SYMBOL(acpi_pm_set_device_wakeup);
+
+/**
+ * acpi_pm_set_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_set_device_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_set_device_wakeup(dev, enable, 1);
+}
+EXPORT_SYMBOL_GPL(acpi_pm_set_device_wakeup);
+
+/**
+ * acpi_pm_set_bridge_wakeup - Enable/disable remote wakeup for given bridge.
+ * @dev: Bridge device to enable/disable to generate wakeup events.
+ * @enable: Whether to enable or disable the wakeup functionality.
+ */
+int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
+{
+	return __acpi_pm_set_device_wakeup(dev, enable, INT_MAX);
+}
+EXPORT_SYMBOL_GPL(acpi_pm_set_bridge_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
@@ -626,6 +626,7 @@ acpi_status acpi_remove_pm_notifier(stru
 bool acpi_pm_device_can_wakeup(struct device *dev);
 int acpi_pm_device_sleep_state(struct device *, int *, int);
 int acpi_pm_set_device_wakeup(struct device *dev, bool enable);
+int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable);
 #else
 static inline void acpi_pm_wakeup_event(struct device *dev)
 {
@@ -656,6 +657,10 @@ static inline int acpi_pm_set_device_wak
 {
 	return -ENODEV;
 }
+static inline int acpi_pm_set_bridge_wakeup(struct device *dev, bool enable)
+{
+	return -ENODEV;
+}
 #endif
 
 #ifdef CONFIG_ACPI_SLEEP
Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -573,7 +573,7 @@ static int acpi_pci_propagate_wakeup(str
 {
 	while (bus->parent) {
 		if (acpi_pm_device_can_wakeup(&bus->self->dev))
-			return acpi_pm_set_device_wakeup(&bus->self->dev, enable);
+			return acpi_pm_set_bridge_wakeup(&bus->self->dev, enable);
 
 		bus = bus->parent;
 	}
@@ -581,7 +581,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_set_device_wakeup(bus->bridge, enable);
+			return acpi_pm_set_bridge_wakeup(bus->bridge, enable);
 	}
 	return 0;
 }

Re: [PATCH 0/3] PCI / ACPI / PM: Fix propagation of wakeup settings to bridges

[Rafael J. Wysocki]

On Friday, July 21, 2017 02:36:44 PM Rafael J. Wysocki wrote:
> Hi,
> 
> The device wakeup code in pci-acpi.c currently has a (theoretical) problem that
> it can disable wakeup for a bridge prematurely in some obscure conditions.
> 
> This is described in some more detail in the changelog of patch [3/3] and it
> has been there for quite a while, so it's nothing new, but it would be good
> to fix it at last and hence this series.
> 
> Patches [1/3] and [2/3] are mostly preparatory, although the former may be
> regarded as a fix by itself and the latter is a cleanup.

Hi Bjorn,

Since all of the patches in this series have been reviewed by Mika now, please
let me know if you have any concerns regarding them from the PCI angle.

Otherwise I'm going to queue them up for 4.14 early next week.

Thanks,
Rafael