* Re: [PATCH] PCI: add NO_D3_DURING_SLEEP flag and revert 151b61284776be2
From: Steven Rostedt @ 2012-06-13 20:15 UTC (permalink / raw)
To: Greg KH
Cc: Rafael J. Wysocki, Alan Stern, Oleksij Rempel (fishor),
Dâniel Fraga, Andrey Rahmatullin,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <20120613200310.GA11110-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Wed, 2012-06-13 at 13:03 -0700, Greg KH wrote:
> On Wed, Jun 13, 2012 at 10:04:43PM +0200, Rafael J. Wysocki wrote:
13:03 -0700 == 20:03 UTC
10:04PM +0200 == 22:04 +0200 == 20:04 UTC
> >
> > Please take it, if that's not a problem, I don't have any other
> > patches for 3.5 pending. And please add:
> >
> > Reviewed-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
>
> Ok, I'll go queue it up right now.
>
> greg k-h
One of you (or both) need to learn to use NTP, or perhaps, as we all
suspect, Greg is really psychic.
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: add NO_D3_DURING_SLEEP flag and revert 151b61284776be2
From: Rafael J. Wysocki @ 2012-06-13 20:04 UTC (permalink / raw)
To: Greg KH
Cc: Dâniel Fraga, USB list, Steven Rostedt, Andrey Rahmatullin,
Oleksij Rempel (fishor), linux-pm
In-Reply-To: <20120613152701.GA28116@kroah.com>
On Wednesday, June 13, 2012, Greg KH wrote:
> On Wed, Jun 13, 2012 at 11:20:19AM -0400, Alan Stern wrote:
> > This patch (as1558) fixes a problem affecting several ASUS computers:
> > The machine crashes or corrupts memory when going into suspend if the
> > ehci-hcd driver is bound to any controllers. Users have been forced
> > to unbind or unload ehci-hcd before putting their systems to sleep.
> >
> > After extensive testing, it was determined that the machines don't
> > like going into suspend when any EHCI controllers are in the PCI D3
> > power state. Presumably this is a firmware bug, but there's nothing
> > we can do about it except to avoid putting the controllers in D3
> > during system sleep.
> >
> > The patch adds a new flag to indicate whether the problem is present,
> > and avoids changing the controller's power state if the flag is set.
> > Runtime suspend is unaffected; this matters only for system suspend.
> > However as a side effect, the controller will not respond to remote
> > wakeup requests while the system is asleep. Hence USB wakeup is not
> > functional -- but of course, this is already true in the current state
> > of affairs.
> >
> > A similar patch has already been applied as commit
> > 151b61284776be2d6f02d48c23c3625678960b97 (USB: EHCI: fix crash during
> > suspend on ASUS computers). The patch supersedes that one and reverts
> > it. There are two differences:
> >
> > The old patch added the flag at the USB level; this patch
> > adds it at the PCI level.
> >
> > The old patch applied to all chipsets with the same vendor,
> > subsystem vendor, and product IDs; this patch makes an
> > exception for a known-good system (based on DMI information).
> >
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > Tested-by: Dâniel Fraga <fragabr@gmail.com>
> > Tested-by: Andrey Rahmatullin <wrar@wrar.name>
> > Tested-by: Steven Rostedt <rostedt@goodmis.org>
> > CC: Greg KH <greg@kroah.com>
> > CC: <stable@vger.kernel.org>
> >
> > ---
> >
> > Greg, do you mind if this goes in through Rafael's tree?
>
> Not at all, but I can also take it.
>
> Rafael, if you want to take it, please add:
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> to it, or let me know, and I can take this through my tree.
Please take it, if that's not a problem, I don't have any other
patches for 3.5 pending. And please add:
Reviewed-by: Rafael J. Wysocki <rjw@sisk.pl>
to it.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] PCI: add NO_D3_DURING_SLEEP flag and revert 151b61284776be2
From: Greg KH @ 2012-06-13 20:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Dâniel Fraga, USB list, Steven Rostedt, Andrey Rahmatullin,
Oleksij Rempel (fishor), linux-pm
In-Reply-To: <201206132204.43622.rjw@sisk.pl>
On Wed, Jun 13, 2012 at 10:04:43PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 13, 2012, Greg KH wrote:
> > On Wed, Jun 13, 2012 at 11:20:19AM -0400, Alan Stern wrote:
> > > This patch (as1558) fixes a problem affecting several ASUS computers:
> > > The machine crashes or corrupts memory when going into suspend if the
> > > ehci-hcd driver is bound to any controllers. Users have been forced
> > > to unbind or unload ehci-hcd before putting their systems to sleep.
> > >
> > > After extensive testing, it was determined that the machines don't
> > > like going into suspend when any EHCI controllers are in the PCI D3
> > > power state. Presumably this is a firmware bug, but there's nothing
> > > we can do about it except to avoid putting the controllers in D3
> > > during system sleep.
> > >
> > > The patch adds a new flag to indicate whether the problem is present,
> > > and avoids changing the controller's power state if the flag is set.
> > > Runtime suspend is unaffected; this matters only for system suspend.
> > > However as a side effect, the controller will not respond to remote
> > > wakeup requests while the system is asleep. Hence USB wakeup is not
> > > functional -- but of course, this is already true in the current state
> > > of affairs.
> > >
> > > A similar patch has already been applied as commit
> > > 151b61284776be2d6f02d48c23c3625678960b97 (USB: EHCI: fix crash during
> > > suspend on ASUS computers). The patch supersedes that one and reverts
> > > it. There are two differences:
> > >
> > > The old patch added the flag at the USB level; this patch
> > > adds it at the PCI level.
> > >
> > > The old patch applied to all chipsets with the same vendor,
> > > subsystem vendor, and product IDs; this patch makes an
> > > exception for a known-good system (based on DMI information).
> > >
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > > Tested-by: Dâniel Fraga <fragabr@gmail.com>
> > > Tested-by: Andrey Rahmatullin <wrar@wrar.name>
> > > Tested-by: Steven Rostedt <rostedt@goodmis.org>
> > > CC: Greg KH <greg@kroah.com>
> > > CC: <stable@vger.kernel.org>
> > >
> > > ---
> > >
> > > Greg, do you mind if this goes in through Rafael's tree?
> >
> > Not at all, but I can also take it.
> >
> > Rafael, if you want to take it, please add:
> >
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > to it, or let me know, and I can take this through my tree.
>
> Please take it, if that's not a problem, I don't have any other
> patches for 3.5 pending. And please add:
>
> Reviewed-by: Rafael J. Wysocki <rjw@sisk.pl>
Ok, I'll go queue it up right now.
greg k-h
^ permalink raw reply
* Re: [PATCH] ACPI, cpuidle: Fix suspend/resume regression caused by cpuidle cleanup.
From: Rafael J. Wysocki @ 2012-06-13 20:01 UTC (permalink / raw)
To: Deepthi Dharwar
Cc: Dave Hansen, Linux PM mailing list, linux-pm, linux-acpi,
Len Brown, Jean Pihet, Arjan van de Ven, Kevin Hilman,
Arnd Bergmann, Ferenc Wagner, Tomas M., Srivatsa S. Bhat, preeti
In-Reply-To: <4FD87249.6010302@linux.vnet.ibm.com>
On Wednesday, June 13, 2012, Deepthi Dharwar wrote:
>
> From: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
>
> Fix suspend/resume regression caused by cpuidle cleanup.
>
> Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move
> dev->last_residency update to driver enter routine; remove dev->last_state)
> was breaking suspend on laptops, as reported in the below link
> - https://lkml.org/lkml/2011/11/11/164
>
> This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584
> (ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression)
> by removing acpi_idle_suspend flag.
> - https://lkml.org/lkml/2011/11/14/74
>
> But this fix did not work on all systems
> as Suspend/resume regression was reported on Lenovo S10-3
> recently by Dave.
> - https://lkml.org/lkml/2012/5/27/115
> It looked like with commit e978aa7d broke suspend and
> with commit 3439a8da resume was not working with acpi_idle driver.
>
> This patch fixes the regression that caused this issue
> in the first place. acpi_idle_suspend flag is essential on
> some x86 systems to prevent the cpus from going to deeper C-states
> when suspend is triggered ( commit b04e7bdb984 )
> So reverting the commit 3439a8da is essential.
>
> By default, irqs are disabled in cpu_idle arch specific call
> and re-enabled in idle state return path . As the acpi_idle_suspend
> flag was being set during suspend, which prevented the cpus
> going to deeper idle states, it is essential to
> enabling the irqs in its return path too.
>
> To address the suspend issue,
> we were not re-enabling the interrupts while returning from
> acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set.
> and this was causing suspend failure.
>
> In addition to the above fix, a sanity check has also been added
> in x86 arch specific cpu_idle call to ensure that the idle call
> always returns with IRQs enabled.
>
> This patch applies on 3.5-rc2
> ---
>
> Reported-and-Tested-by: Dav Hansen <dave@linux.vnet.ibm.com>
> Tested-by: Preeti Murthy <preeti@linux.vnet.ibm.com>
> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> Reviewed-by: Srivatsa S Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> arch/x86/kernel/process.c | 6 ++++++
> drivers/acpi/processor_idle.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 735279e..8ab76ad 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -460,6 +460,12 @@ void cpu_idle(void)
> pm_idle();
>
> rcu_idle_exit();
> +
> + /*
> + * Sanity check to ensure that idle call returns
> + * with IRQs enabled
> + */
> + WARN_ON(irqs_disabled());
> start_critical_timings();
>
> /* In many cases the interrupt that ended idle
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index f3decb3..c2ffd84 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> /*
> * Suspend / resume control
> */
> +static int acpi_idle_suspend;
> static u32 saved_bm_rld;
>
> static void acpi_idle_bm_rld_save(void)
> @@ -242,13 +243,21 @@ static void acpi_idle_bm_rld_restore(void)
>
> int acpi_processor_suspend(struct acpi_device * device, pm_message_t state)
> {
> + if (acpi_idle_suspend == 1)
> + return 0;
> +
> acpi_idle_bm_rld_save();
> + acpi_idle_suspend = 1;
> return 0;
> }
>
> int acpi_processor_resume(struct acpi_device * device)
> {
> + if (acpi_idle_suspend == 0)
> + return 0;
> +
> acpi_idle_bm_rld_restore();
> + acpi_idle_suspend = 0;
> return 0;
> }
>
> @@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>
> local_irq_disable();
>
> + if (acpi_idle_suspend) {
> + local_irq_enable();
> + cpu_relax();
> + return -EINVAL;
> + }
> +
> lapic_timer_state_broadcast(pr, cx, 1);
> kt1 = ktime_get_real();
> acpi_idle_do_entry(cx);
> @@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>
> local_irq_disable();
>
> + if (acpi_idle_suspend) {
> + local_irq_enable();
> + cpu_relax();
> + return -EINVAL;
> + }
> +
> if (cx->entry_method != ACPI_CSTATE_FFH) {
> current_thread_info()->status &= ~TS_POLLING;
> /*
> @@ -901,6 +922,13 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
> if (unlikely(!pr))
> return -EINVAL;
>
> + if (acpi_idle_suspend) {
> + if (irqs_disabled())
> + local_irq_enable();
> + cpu_relax();
> + return -EINVAL;
> + }
> +
> if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
> if (drv->safe_state_index >= 0) {
> return drv->states[drv->safe_state_index].enter(dev,
>
> Regards,
> Deepthi
>
>
>
^ permalink raw reply
* Re: [PATCH] ACPI, cpuidle: Fix suspend/resume regression caused by cpuidle cleanup.
From: Dave Hansen @ 2012-06-13 17:25 UTC (permalink / raw)
To: Deepthi Dharwar
Cc: Linux PM mailing list, linux-pm, linux-acpi, Len Brown,
Rafael J. Wysocki, Jean Pihet, Arjan van de Ven, Kevin Hilman,
Arnd Bergmann, Ferenc Wagner, Tomas M., Srivatsa S. Bhat, preeti
In-Reply-To: <4FD87249.6010302@linux.vnet.ibm.com>
On 06/13/2012 03:58 AM, Deepthi Dharwar wrote:
> In addition to the above fix, a sanity check has also been added
> in x86 arch specific cpu_idle call to ensure that the idle call
> always returns with IRQs enabled.
>
> This patch applies on 3.5-rc2
Just applied this on top of a post-3.5-rc2 kernel (7794f70).
Suspend/resume seems to work just fine on the hardware where it was
broken. Thanks, Deepthi!
My tested-by was for the original hackish patch, but this is just to
spell out that it worked for me with this patch too.
Tested-by: Dave Hansen <dave@linux.vnet.ibm.com>
^ permalink raw reply
* Re: [PATCH] PCI: add NO_D3_DURING_SLEEP flag and revert 151b61284776be2
From: Greg KH @ 2012-06-13 15:27 UTC (permalink / raw)
To: Alan Stern
Cc: Rafael J. Wysocki, Oleksij Rempel (fishor), Dâniel Fraga,
Andrey Rahmatullin, Steven Rostedt,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, USB list
In-Reply-To: <Pine.LNX.4.44L0.1206131117260.1401-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
On Wed, Jun 13, 2012 at 11:20:19AM -0400, Alan Stern wrote:
> This patch (as1558) fixes a problem affecting several ASUS computers:
> The machine crashes or corrupts memory when going into suspend if the
> ehci-hcd driver is bound to any controllers. Users have been forced
> to unbind or unload ehci-hcd before putting their systems to sleep.
>
> After extensive testing, it was determined that the machines don't
> like going into suspend when any EHCI controllers are in the PCI D3
> power state. Presumably this is a firmware bug, but there's nothing
> we can do about it except to avoid putting the controllers in D3
> during system sleep.
>
> The patch adds a new flag to indicate whether the problem is present,
> and avoids changing the controller's power state if the flag is set.
> Runtime suspend is unaffected; this matters only for system suspend.
> However as a side effect, the controller will not respond to remote
> wakeup requests while the system is asleep. Hence USB wakeup is not
> functional -- but of course, this is already true in the current state
> of affairs.
>
> A similar patch has already been applied as commit
> 151b61284776be2d6f02d48c23c3625678960b97 (USB: EHCI: fix crash during
> suspend on ASUS computers). The patch supersedes that one and reverts
> it. There are two differences:
>
> The old patch added the flag at the USB level; this patch
> adds it at the PCI level.
>
> The old patch applied to all chipsets with the same vendor,
> subsystem vendor, and product IDs; this patch makes an
> exception for a known-good system (based on DMI information).
>
> Signed-off-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Tested-by: Dâniel Fraga <fragabr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Andrey Rahmatullin <wrar-Bxt/JK/G85ezQB+pC5nmwQ@public.gmane.org>
> Tested-by: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>
> CC: Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
> CC: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>
> ---
>
> Greg, do you mind if this goes in through Rafael's tree?
Not at all, but I can also take it.
Rafael, if you want to take it, please add:
Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
to it, or let me know, and I can take this through my tree.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] PCI: add NO_D3_DURING_SLEEP flag and revert 151b61284776be2
From: Alan Stern @ 2012-06-13 15:20 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Dâniel Fraga, Greg KH, USB list, Steven Rostedt,
Andrey Rahmatullin, Oleksij Rempel (fishor), linux-pm
In-Reply-To: <201206131122.36198.rjw@sisk.pl>
This patch (as1558) fixes a problem affecting several ASUS computers:
The machine crashes or corrupts memory when going into suspend if the
ehci-hcd driver is bound to any controllers. Users have been forced
to unbind or unload ehci-hcd before putting their systems to sleep.
After extensive testing, it was determined that the machines don't
like going into suspend when any EHCI controllers are in the PCI D3
power state. Presumably this is a firmware bug, but there's nothing
we can do about it except to avoid putting the controllers in D3
during system sleep.
The patch adds a new flag to indicate whether the problem is present,
and avoids changing the controller's power state if the flag is set.
Runtime suspend is unaffected; this matters only for system suspend.
However as a side effect, the controller will not respond to remote
wakeup requests while the system is asleep. Hence USB wakeup is not
functional -- but of course, this is already true in the current state
of affairs.
A similar patch has already been applied as commit
151b61284776be2d6f02d48c23c3625678960b97 (USB: EHCI: fix crash during
suspend on ASUS computers). The patch supersedes that one and reverts
it. There are two differences:
The old patch added the flag at the USB level; this patch
adds it at the PCI level.
The old patch applied to all chipsets with the same vendor,
subsystem vendor, and product IDs; this patch makes an
exception for a known-good system (based on DMI information).
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Tested-by: Dâniel Fraga <fragabr@gmail.com>
Tested-by: Andrey Rahmatullin <wrar@wrar.name>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
CC: Greg KH <greg@kroah.com>
CC: <stable@vger.kernel.org>
---
Greg, do you mind if this goes in through Rafael's tree?
drivers/pci/pci.c | 5 +++++
drivers/pci/quirks.c | 26 ++++++++++++++++++++++++++
drivers/usb/core/hcd-pci.c | 9 ---------
drivers/usb/host/ehci-pci.c | 8 --------
include/linux/pci.h | 2 ++
include/linux/usb/hcd.h | 2 --
6 files changed, 33 insertions(+), 19 deletions(-)
Index: usb-3.4/include/linux/pci.h
===================================================================
--- usb-3.4.orig/include/linux/pci.h
+++ usb-3.4/include/linux/pci.h
@@ -176,6 +176,8 @@ enum pci_dev_flags {
PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
/* Provide indication device is assigned by a Virtual Machine Manager */
PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
+ /* Device causes system crash if in D3 during S3 sleep */
+ PCI_DEV_FLAGS_NO_D3_DURING_SLEEP = (__force pci_dev_flags_t) 8,
};
enum pci_irq_reroute_variant {
Index: usb-3.4/drivers/pci/pci.c
===================================================================
--- usb-3.4.orig/drivers/pci/pci.c
+++ usb-3.4/drivers/pci/pci.c
@@ -1743,6 +1743,11 @@ int pci_prepare_to_sleep(struct pci_dev
if (target_state == PCI_POWER_ERROR)
return -EIO;
+ /* Some devices mustn't be in D3 during system sleep */
+ if (target_state == PCI_D3hot &&
+ (dev->dev_flags & PCI_DEV_FLAGS_NO_D3_DURING_SLEEP))
+ return 0;
+
pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
error = pci_set_power_state(dev, target_state);
Index: usb-3.4/drivers/pci/quirks.c
===================================================================
--- usb-3.4.orig/drivers/pci/quirks.c
+++ usb-3.4/drivers/pci/quirks.c
@@ -2917,6 +2917,32 @@ static void __devinit disable_igfx_irq(s
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
+/*
+ * The Intel 6 Series/C200 Series chipset's EHCI controllers on many
+ * ASUS motherboards will cause memory corruption or a system crash
+ * if they are in D3 while the system is put into S3 sleep.
+ */
+static void __devinit asus_ehci_no_d3(struct pci_dev *dev)
+{
+ const char *sys_info;
+ static const char good_Asus_board[] = "P8Z68-V";
+
+ if (dev->dev_flags & PCI_DEV_FLAGS_NO_D3_DURING_SLEEP)
+ return;
+ if (dev->subsystem_vendor != PCI_VENDOR_ID_ASUSTEK)
+ return;
+ sys_info = dmi_get_system_info(DMI_BOARD_NAME);
+ if (sys_info && memcmp(sys_info, good_Asus_board,
+ sizeof(good_Asus_board) - 1) == 0)
+ return;
+
+ dev_info(&dev->dev, "broken D3 during system sleep on ASUS\n");
+ dev->dev_flags |= PCI_DEV_FLAGS_NO_D3_DURING_SLEEP;
+ device_set_wakeup_capable(&dev->dev, false);
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1c26, asus_ehci_no_d3);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1c2d, asus_ehci_no_d3);
+
static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
struct pci_fixup *end)
{
Index: usb-3.4/drivers/usb/core/hcd-pci.c
===================================================================
--- usb-3.4.orig/drivers/usb/core/hcd-pci.c
+++ usb-3.4/drivers/usb/core/hcd-pci.c
@@ -493,15 +493,6 @@ static int hcd_pci_suspend_noirq(struct
pci_save_state(pci_dev);
- /*
- * Some systems crash if an EHCI controller is in D3 during
- * a sleep transition. We have to leave such controllers in D0.
- */
- if (hcd->broken_pci_sleep) {
- dev_dbg(dev, "Staying in PCI D0\n");
- return retval;
- }
-
/* If the root hub is dead rather than suspended, disallow remote
* wakeup. usb_hc_died() should ensure that both hosts are marked as
* dying, so we only need to check the primary roothub.
Index: usb-3.4/drivers/usb/host/ehci-pci.c
===================================================================
--- usb-3.4.orig/drivers/usb/host/ehci-pci.c
+++ usb-3.4/drivers/usb/host/ehci-pci.c
@@ -151,14 +151,6 @@ static int ehci_pci_setup(struct usb_hcd
hcd->has_tt = 1;
tdi_reset(ehci);
}
- if (pdev->subsystem_vendor == PCI_VENDOR_ID_ASUSTEK) {
- /* EHCI #1 or #2 on 6 Series/C200 Series chipset */
- if (pdev->device == 0x1c26 || pdev->device == 0x1c2d) {
- ehci_info(ehci, "broken D3 during system sleep on ASUS\n");
- hcd->broken_pci_sleep = 1;
- device_set_wakeup_capable(&pdev->dev, false);
- }
- }
break;
case PCI_VENDOR_ID_TDI:
if (pdev->device == PCI_DEVICE_ID_TDI_EHCI) {
Index: usb-3.4/include/linux/usb/hcd.h
===================================================================
--- usb-3.4.orig/include/linux/usb/hcd.h
+++ usb-3.4/include/linux/usb/hcd.h
@@ -126,8 +126,6 @@ struct usb_hcd {
unsigned wireless:1; /* Wireless USB HCD */
unsigned authorized_default:1;
unsigned has_tt:1; /* Integrated TT in root hub */
- unsigned broken_pci_sleep:1; /* Don't put the
- controller in PCI-D3 for system sleep */
unsigned int irq; /* irq allocated */
void __iomem *regs; /* device memory/io */
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [RFC PATCH 11/12] thermal: introduce cooling state arbitrator
From: Eduardo Valentin @ 2012-06-13 14:23 UTC (permalink / raw)
To: Zhang Rui; +Cc: linux-acpi@vger.kernel.org, linux-pm
In-Reply-To: <1339384839.1492.165.camel@rui.sh.intel.com>
Hello Rui,
On Mon, Jun 11, 2012 at 11:20:39AM +0800, Zhang Rui wrote:
>
> Introduce simple arbitrator for setting device cooling state,
> to fix the problem that a cooling device may be referenced by
> by multiple trip points in multiple thermal zones.
>
>
> With this patch, we have two stages for updating a thermal zone,
> 1. check if a thermal_instance needs to be updated or not
> 2. update the cooling device, based on the target cooling state
> of all its instances.
>
> Note that, currently, the cooling device is set to the deepest
> cooling state required.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/thermal/thermal_sys.c | 41 ++++++++++++++++++++++++++++++++++++++---
> include/linux/thermal.h | 1 +
> 2 files changed, 39 insertions(+), 3 deletions(-)
>
> Index: rtd3/drivers/thermal/thermal_sys.c
> ===================================================================
> --- rtd3.orig/drivers/thermal/thermal_sys.c
> +++ rtd3/drivers/thermal/thermal_sys.c
> @@ -54,6 +54,7 @@ struct thermal_instance {
> int trip;
> unsigned long upper; /* Highest cooling state for this trip point */
> unsigned long lower; /* Lowest cooling state for this trip point */
> + unsigned long target; /* expected cooling state */
> char attr_name[THERMAL_NAME_LENGTH];
> struct device_attribute attr;
> struct list_head tz_node; /* node in tz->instances */
> @@ -812,6 +813,7 @@ int thermal_zone_bind_cooling_device(str
> dev->trip = trip;
> dev->upper = upper;
> dev->lower = lower;
> + dev->target = -1;
>
> result = get_idr(&tz->idr, &tz->lock, &dev->id);
> if (result)
> @@ -949,6 +951,7 @@ thermal_cooling_device_register(char *ty
> strcpy(cdev->type, type);
> INIT_LIST_HEAD(&cdev->instances);
> cdev->ops = ops;
> + cdev->updated = 1;
> cdev->device.class = &thermal_class;
> cdev->devdata = devdata;
> dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> @@ -1040,6 +1043,32 @@ void thermal_cooling_device_unregister(s
> }
> EXPORT_SYMBOL(thermal_cooling_device_unregister);
>
> +static void thermal_zone_do_update(struct thermal_zone_device *tz)
> +{
> + struct thermal_instance *instance1, *instance2;
> + struct thermal_cooling_device *cdev;
> + int target;
> +
> + list_for_each_entry(instance1, &tz->instances, tz_node) {
> + cdev = instance1->cdev;
> +
> + /* cooling device has already been updated*/
> + if (cdev->updated)
> + continue;
> +
> + target = 0;
> + /* Make sure cdev enters the deepest cooling state */
> + list_for_each_entry(instance2, &cdev->instances, cdev_node) {
> + if (instance2->target == -1)
> + continue;
> + if (instance2->target > target)
> + target = instance2->target;
> + }
> + cdev->ops->set_cur_state(cdev, target);
> + cdev->updated = 1;
> + }
> +}
I think the above arbitrator solution does not escalate. As I can see,
the arbitrator takes care of cooling device instances in the same thermal zone.
What if you have a device which expose cooling device instances in different
thermal zones?
Besides, the thermal constraint might collide with pm QoS or with settings
coming from user space interfaces.
That's actually why I was suggesting to have this 'arbitrator' or constraint
management outside the thermal framework. And managed per device at some
other abstraction layer.
> +
> /*
> * Cooling algorithm for active trip points
> *
> @@ -1086,19 +1115,24 @@ static void thermal_zone_trip_update(str
> cur_state = cur_state > instance->lower ?
> (cur_state - 1) : instance->lower;
> }
> - cdev->ops->set_cur_state(cdev, cur_state);
> + instance->target = cur_state;
> + cdev->updated = 0; /* cooling device needs update */
> }
> } else { /* below trip */
> list_for_each_entry(instance, &tz->instances, tz_node) {
> if (instance->trip != trip)
> continue;
>
> + /* Do not use the deacitve thermal instance */
> + if (instance->target == -1)
> + continue;
> cdev = instance->cdev;
> cdev->ops->get_cur_state(cdev, &cur_state);
>
> cur_state = cur_state > instance->lower ?
> - (cur_state - 1) : instance->lower;
> - cdev->ops->set_cur_state(cdev, cur_state);
> + (cur_state - 1) : -1;
> + instance->target = cur_state;
> + cdev->updated = 0; /* cooling device needs update */
> }
> }
>
> @@ -1159,6 +1193,7 @@ void thermal_zone_device_update(struct t
> }
> }
>
> + thermal_zone_do_update(tz);
> if (tz->forced_passive)
> thermal_zone_device_passive(tz, temp, tz->forced_passive,
> THERMAL_TRIPS_NONE);
> Index: rtd3/include/linux/thermal.h
> ===================================================================
> --- rtd3.orig/include/linux/thermal.h
> +++ rtd3/include/linux/thermal.h
> @@ -86,6 +86,7 @@ struct thermal_cooling_device {
> struct device device;
> void *devdata;
> const struct thermal_cooling_device_ops *ops;
> + int updated; /* 1 if the cooling device does not need update */
> struct list_head instances;
> struct list_head node;
> };
>
>
^ permalink raw reply
* Re: [RFT] PCI changes related to wakeup (was: Re: ehci_hcd related S3 lockup on ASUS laptops, again)
From: Alan Stern @ 2012-06-13 14:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Mailing List, Dâniel Fraga, Steven Rostedt,
Andrey Rahmatullin, Oleksij Rempel (fishor), linux-pm
In-Reply-To: <201206131122.36198.rjw@sisk.pl>
On Wed, 13 Jun 2012, Rafael J. Wysocki wrote:
> Hi Alan,
> It looks like this worked for everyone concerned, is that correct?
Apparently.
> In that case, do you intend to send this patch for inclusion any time soon?
> It would be good to have this particular issue taken care of.
Thanks for reminding me. I'll send it soon.
Alan Stern
^ permalink raw reply
* Re: [RFC 1/4] cpuidle: define the enter function in the driver structure
From: Daniel Lezcano @ 2012-06-13 13:10 UTC (permalink / raw)
To: Jean Pihet; +Cc: Kevin Hilman, linaro-dev, linux-kernel, linux-acpi, linux-pm
In-Reply-To: <CAORVsuXEkyU_OkjqLDhe_OMq-uLEBaMLnz-pkJ1jtgnmcNxZpw@mail.gmail.com>
On 06/13/2012 02:44 PM, Jean Pihet wrote:
> Hi Daniel,
>
> On Fri, Jun 8, 2012 at 11:34 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 06/08/2012 07:33 PM, Deepthi Dharwar wrote:
>>> Hi Daniel,
>>
>> Hi Deepthi,
>>
>>> On 06/08/2012 09:32 PM, Daniel Lezcano wrote:
>>>
>>>> We have the state index passed as parameter to the 'enter' function.
>>>> Most of the drivers assign their 'enter' functions several times in
>>>> the cpuidle_state structure, as we have the index, we can delegate
>>>> to the driver to handle their own callback array.
>>>>
>>>> That will have the benefit of removing multiple lines of code in the
>>>> different drivers.
>>>>
>>>> In order to smoothly modify the driver, the 'enter' function are in
>>>> the driver structure and in the cpuidle state structure. That will
>>>> let the time to modify the different drivers one by one.
>>>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>>>> assigned in the driver structure and use it, otherwise it invokes
>>>> the 'enter' assigned to the cpuidle_state.
>>>
>>>
>>> Currently, the backend driver initializes
>>> all the cpuidle states supported on the platform,
>>> and each state can have its own enter routine
>>> which can be unique This is a clean approach.
>>
>> Yes, I perfectly understood the purpose of this field but as clean it is
>> it does not make sense as it is not used in this way. If it is supposed
>> to be done in the way you are describing here, we should have the same
>> number of states and enter functions. Here it is how it is used:
>>
>> --------------------------------------------------
>> | Arch | nr states | nr enter function |
>> --------------------------------------------------
>> | x86 (nehalem) | 3 | 1 |
>> --------------------------------------------------
>> | x86 (snb) | 4 | 1 |
>> --------------------------------------------------
>> | x86 (atom) | 4 | 1 |
>> --------------------------------------------------
>> | ARM tegra | 1 | 1 |
>> --------------------------------------------------
>> | ARM omap3 | 7 | 2 |
>> --------------------------------------------------
>> | ARM omap4 | 3 | 1 |
>> --------------------------------------------------
>> | ARM ux500 | 2 | 1 |
>> --------------------------------------------------
>> | ARM shmobile | 1 | 1 |
>> --------------------------------------------------
>> | ARM davinci | 2 | 1 |
>> --------------------------------------------------
>> | ARM at91 | 2 | 1 |
>> --------------------------------------------------
>> | ARM s3c64xx | 1 | 1 |
>> --------------------------------------------------
>> | ARM exynos | 2 | 1 |
>> --------------------------------------------------
>> | ARM kirkwood | 2 | 1 |
>> --------------------------------------------------
>> | SH | 3 | 1 |
>> --------------------------------------------------
>> | PPC | 2 | 2 |
>> --------------------------------------------------
>> | | | |
>> | TOTAL | 39 | 17 |
>> | | | |
>> --------------------------------------------------
>>
>>
>> As you can see most of the enter functions are only used as one.
>> The Omap3 cpuidle driver enter function for C2 calls the enter function
>> of C1. Other arch, already use a table of callbacks or the index.
> There is a plan to remove the extra enter function as part of an
> optimization, cf. [1]. The fix is planned to reach the 3.6 mainline
> kernel via Kevin's tree [2].
>
> [1] http://marc.info/?l=linux-omap&m=133856365818099&w=2
> [2] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=shortlog;h=refs/heads/for_3.6/pm/performance
>
> The result is that there will be only one enter function for OMAP3.
Another argument in favor of moving this field to the driver structure.
Thanks Jean for the information.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* Re: [linux-pm] [RFC 1/4] cpuidle: define the enter function in the driver structure
From: Jean Pihet @ 2012-06-13 12:44 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Kevin Hilman, Deepthi Dharwar, linaro-dev-cunTk1MwBs8s++Sfvej+rw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
In-Reply-To: <4FD27000.7000208-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Hi Daniel,
On Fri, Jun 8, 2012 at 11:34 PM, Daniel Lezcano
<daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 06/08/2012 07:33 PM, Deepthi Dharwar wrote:
>> Hi Daniel,
>
> Hi Deepthi,
>
>> On 06/08/2012 09:32 PM, Daniel Lezcano wrote:
>>
>>> We have the state index passed as parameter to the 'enter' function.
>>> Most of the drivers assign their 'enter' functions several times in
>>> the cpuidle_state structure, as we have the index, we can delegate
>>> to the driver to handle their own callback array.
>>>
>>> That will have the benefit of removing multiple lines of code in the
>>> different drivers.
>>>
>>> In order to smoothly modify the driver, the 'enter' function are in
>>> the driver structure and in the cpuidle state structure. That will
>>> let the time to modify the different drivers one by one.
>>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>>> assigned in the driver structure and use it, otherwise it invokes
>>> the 'enter' assigned to the cpuidle_state.
>>
>>
>> Currently, the backend driver initializes
>> all the cpuidle states supported on the platform,
>> and each state can have its own enter routine
>> which can be unique This is a clean approach.
>
> Yes, I perfectly understood the purpose of this field but as clean it is
> it does not make sense as it is not used in this way. If it is supposed
> to be done in the way you are describing here, we should have the same
> number of states and enter functions. Here it is how it is used:
>
> --------------------------------------------------
> | Arch | nr states | nr enter function |
> --------------------------------------------------
> | x86 (nehalem) | 3 | 1 |
> --------------------------------------------------
> | x86 (snb) | 4 | 1 |
> --------------------------------------------------
> | x86 (atom) | 4 | 1 |
> --------------------------------------------------
> | ARM tegra | 1 | 1 |
> --------------------------------------------------
> | ARM omap3 | 7 | 2 |
> --------------------------------------------------
> | ARM omap4 | 3 | 1 |
> --------------------------------------------------
> | ARM ux500 | 2 | 1 |
> --------------------------------------------------
> | ARM shmobile | 1 | 1 |
> --------------------------------------------------
> | ARM davinci | 2 | 1 |
> --------------------------------------------------
> | ARM at91 | 2 | 1 |
> --------------------------------------------------
> | ARM s3c64xx | 1 | 1 |
> --------------------------------------------------
> | ARM exynos | 2 | 1 |
> --------------------------------------------------
> | ARM kirkwood | 2 | 1 |
> --------------------------------------------------
> | SH | 3 | 1 |
> --------------------------------------------------
> | PPC | 2 | 2 |
> --------------------------------------------------
> | | | |
> | TOTAL | 39 | 17 |
> | | | |
> --------------------------------------------------
>
>
> As you can see most of the enter functions are only used as one.
> The Omap3 cpuidle driver enter function for C2 calls the enter function
> of C1. Other arch, already use a table of callbacks or the index.
There is a plan to remove the extra enter function as part of an
optimization, cf. [1]. The fix is planned to reach the 3.6 mainline
kernel via Kevin's tree [2].
[1] http://marc.info/?l=linux-omap&m=133856365818099&w=2
[2] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=shortlog;h=refs/heads/for_3.6/pm/performance
The result is that there will be only one enter function for OMAP3.
Regards,
Jean
>> By moving the enter routine into the driver,
>> we are enforcing in having only one enter state.
>> There is unnecessary overhead involved
>> in calling a wrapper routine just to
>> index into the right idle state routine
>> for many platforms at runtime.
>
> I don't agree. For the sake of encapsulated code, we duplicate n-times a
> field and that is not used in this way. It is quite easy to have in the
> driver specific code a common enter function to ventilate to the right
> routine without adding extra overhead and let the common code use a
> single enter routine (which is already the case today).
>
>
> _______________________________________________
> linux-pm mailing list
> linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
^ permalink raw reply
* [PATCH] ACPI, cpuidle: Fix suspend/resume regression caused by cpuidle cleanup.
From: Deepthi Dharwar @ 2012-06-13 10:58 UTC (permalink / raw)
To: Dave Hansen, Linux PM mailing list, linux-pm, linux-acpi,
Len Brown, Rafael J. Wysocki
Cc: Kevin Hilman, Arnd Bergmann, Tomas M., Srivatsa S. Bhat, preeti,
Ferenc Wagner, Arjan van de Ven, Jean Pihet
From: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
Fix suspend/resume regression caused by cpuidle cleanup.
Commit e978aa7d7d57d04eb5f88a7507c4fb98577def77 ( cpuidle: Move
dev->last_residency update to driver enter routine; remove dev->last_state)
was breaking suspend on laptops, as reported in the below link
- https://lkml.org/lkml/2011/11/11/164
This was fixed in commit 3439a8da16bcad6b0982ece938c9f8299bb53584
(ACPI / cpuidle: Remove acpi_idle_suspend (to fix suspend regression)
by removing acpi_idle_suspend flag.
- https://lkml.org/lkml/2011/11/14/74
But this fix did not work on all systems
as Suspend/resume regression was reported on Lenovo S10-3
recently by Dave.
- https://lkml.org/lkml/2012/5/27/115
It looked like with commit e978aa7d broke suspend and
with commit 3439a8da resume was not working with acpi_idle driver.
This patch fixes the regression that caused this issue
in the first place. acpi_idle_suspend flag is essential on
some x86 systems to prevent the cpus from going to deeper C-states
when suspend is triggered ( commit b04e7bdb984 )
So reverting the commit 3439a8da is essential.
By default, irqs are disabled in cpu_idle arch specific call
and re-enabled in idle state return path . As the acpi_idle_suspend
flag was being set during suspend, which prevented the cpus
going to deeper idle states, it is essential to
enabling the irqs in its return path too.
To address the suspend issue,
we were not re-enabling the interrupts while returning from
acpi_idle_enter_bm() routine if acpi_idle_suspend flag is set.
and this was causing suspend failure.
In addition to the above fix, a sanity check has also been added
in x86 arch specific cpu_idle call to ensure that the idle call
always returns with IRQs enabled.
This patch applies on 3.5-rc2
---
Reported-and-Tested-by: Dav Hansen <dave@linux.vnet.ibm.com>
Tested-by: Preeti Murthy <preeti@linux.vnet.ibm.com>
Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
Reviewed-by: Srivatsa S Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---
arch/x86/kernel/process.c | 6 ++++++
drivers/acpi/processor_idle.c | 28 ++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 735279e..8ab76ad 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -460,6 +460,12 @@ void cpu_idle(void)
pm_idle();
rcu_idle_exit();
+
+ /*
+ * Sanity check to ensure that idle call returns
+ * with IRQs enabled
+ */
+ WARN_ON(irqs_disabled());
start_critical_timings();
/* In many cases the interrupt that ended idle
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index f3decb3..c2ffd84 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -224,6 +224,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
/*
* Suspend / resume control
*/
+static int acpi_idle_suspend;
static u32 saved_bm_rld;
static void acpi_idle_bm_rld_save(void)
@@ -242,13 +243,21 @@ static void acpi_idle_bm_rld_restore(void)
int acpi_processor_suspend(struct acpi_device * device, pm_message_t state)
{
+ if (acpi_idle_suspend == 1)
+ return 0;
+
acpi_idle_bm_rld_save();
+ acpi_idle_suspend = 1;
return 0;
}
int acpi_processor_resume(struct acpi_device * device)
{
+ if (acpi_idle_suspend == 0)
+ return 0;
+
acpi_idle_bm_rld_restore();
+ acpi_idle_suspend = 0;
return 0;
}
@@ -754,6 +763,12 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
local_irq_disable();
+ if (acpi_idle_suspend) {
+ local_irq_enable();
+ cpu_relax();
+ return -EINVAL;
+ }
+
lapic_timer_state_broadcast(pr, cx, 1);
kt1 = ktime_get_real();
acpi_idle_do_entry(cx);
@@ -823,6 +838,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
local_irq_disable();
+ if (acpi_idle_suspend) {
+ local_irq_enable();
+ cpu_relax();
+ return -EINVAL;
+ }
+
if (cx->entry_method != ACPI_CSTATE_FFH) {
current_thread_info()->status &= ~TS_POLLING;
/*
@@ -901,6 +922,13 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
if (unlikely(!pr))
return -EINVAL;
+ if (acpi_idle_suspend) {
+ if (irqs_disabled())
+ local_irq_enable();
+ cpu_relax();
+ return -EINVAL;
+ }
+
if (!cx->bm_sts_skip && acpi_idle_bm_check()) {
if (drv->safe_state_index >= 0) {
return drv->states[drv->safe_state_index].enter(dev,
Regards,
Deepthi
^ permalink raw reply related
* Re: [RFT] PCI changes related to wakeup (was: Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again)
From: Rafael J. Wysocki @ 2012-06-13 9:22 UTC (permalink / raw)
To: Alan Stern
Cc: Oleksij Rempel (fishor), Dâniel Fraga, Andrey Rahmatullin,
Steven Rostedt, linux-pm, ACPI Devel Mailing List
In-Reply-To: <Pine.LNX.4.44L0.1205311704340.1178-100000@iolanthe.rowland.org>
Hi Alan,
On Thursday, May 31, 2012, Alan Stern wrote:
> On Tue, 29 May 2012, Rafael J. Wysocki wrote:
>
> > > > > Therefore we really do need a quirk, probably in ehci-hcd like the
> > > > > original patch. If it is restricted to apply only in cases where the
> > > > > DMI information lists ASUSTeK as the manufacturer, perhaps that will be
> > > > > sufficient. (For some reason, the manufacturer field in Dâniel's BIOS
> > > > > isn't initialized.)
> > > >
> > > > Yeah.
> > > >
> > > > I'll have a deeper look at this later today, I think.
> > >
> > > It's easy enough to write such a check (or perhaps more reliably, check
> > > for a product name matching "P8Z68-V").
> >
> > I think we should try to express it as a PCI quirk in quirks.c, though.
>
> Here's my attempt. Everybody, please try this patch with the
> 151b61284776 commit removed. Make sure that CONFIG_USB_DEBUG is
> enabled so we can check the controller's power state during suspend,
> and check that the "broken D3 during system sleep on ASUS" message
> shows up during booting.
It looks like this worked for everyone concerned, is that correct?
In that case, do you intend to send this patch for inclusion any time soon?
It would be good to have this particular issue taken care of.
Thanks,
Rafael
> Index: usb-3.4/drivers/pci/pci.c
> ===================================================================
> --- usb-3.4.orig/drivers/pci/pci.c
> +++ usb-3.4/drivers/pci/pci.c
> @@ -1743,6 +1743,11 @@ int pci_prepare_to_sleep(struct pci_dev
> if (target_state == PCI_POWER_ERROR)
> return -EIO;
>
> + /* Some devices mustn't be in D3 during system sleep */
> + if (target_state == PCI_D3hot &&
> + (dev->dev_flags & PCI_DEV_FLAGS_NO_D3_DURING_SLEEP))
> + return 0;
> +
> pci_enable_wake(dev, target_state, device_may_wakeup(&dev->dev));
>
> error = pci_set_power_state(dev, target_state);
> Index: usb-3.4/drivers/pci/quirks.c
> ===================================================================
> --- usb-3.4.orig/drivers/pci/quirks.c
> +++ usb-3.4/drivers/pci/quirks.c
> @@ -2917,6 +2917,32 @@ static void __devinit disable_igfx_irq(s
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>
> +/*
> + * The Intel 6 Series/C200 Series chipset's EHCI controllers on many
> + * ASUS motherboards will cause memory corruption or a system crash
> + * if they are in D3 while the system is put into S3 sleep.
> + */
> +static void __devinit asus_ehci_no_d3(struct pci_dev *dev)
> +{
> + const char *sys_info;
> + static const char good_Asus_board[] = "P8Z68-V";
> +
> + if (dev->dev_flags & PCI_DEV_FLAGS_NO_D3_DURING_SLEEP)
> + return;
> + if (dev->subsystem_vendor != PCI_VENDOR_ID_ASUSTEK)
> + return;
> + sys_info = dmi_get_system_info(DMI_BOARD_NAME);
> + if (sys_info && memcmp(sys_info, good_Asus_board,
> + sizeof(good_Asus_board) - 1) == 0)
> + return;
> +
> + dev_info(&dev->dev, "broken D3 during system sleep on ASUS\n");
> + dev->dev_flags |= PCI_DEV_FLAGS_NO_D3_DURING_SLEEP;
> + device_set_wakeup_capable(&dev->dev, false);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1c26, asus_ehci_no_d3);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1c2d, asus_ehci_no_d3);
> +
> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> struct pci_fixup *end)
> {
> Index: usb-3.4/include/linux/pci.h
> ===================================================================
> --- usb-3.4.orig/include/linux/pci.h
> +++ usb-3.4/include/linux/pci.h
> @@ -176,6 +176,8 @@ enum pci_dev_flags {
> PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
> /* Provide indication device is assigned by a Virtual Machine Manager */
> PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
> + /* Device causes system crash if in D3 during S3 sleep */
> + PCI_DEV_FLAGS_NO_D3_DURING_SLEEP = (__force pci_dev_flags_t) 8,
> };
>
> enum pci_irq_reroute_variant {
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] PM: devfreq: add freq table and available_freqs
From: Xiaoguang Chen @ 2012-06-13 5:28 UTC (permalink / raw)
To: myungjoo.ham
Cc: 박경민, Xiaoguang Chen,
linux-kernel@vger.kernel.org, linux-pm
In-Reply-To: <16928680.915361339563225913.JavaMail.weblogic@epml04>
[-- Attachment #1.1: Type: text/plain, Size: 5210 bytes --]
I think Devfreq should not be combined with OPP,
OPP framework does contain one frequency table, but the frequency is
combined with voltage. some platforms may don't want to use this but
handling voltage seperately in their clock driver.
and some platforms don't use OPP, and they want a frequency list.
then this is necessary. also devfreq should contain a frequency list even
without any other frameworks, don't you think so ?
Thanks
Xiaoguang
2012/6/13 MyungJoo Ham <myungjoo.ham@samsung.com>
> > Devfreq framework don't have a frequency table, add it
> > for easy use.
> >
> > Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
>
> If you need a predefined data structure to support frequency table,
> you can simply use OPP, which has helper functions implemented in
> devfreq subsystem. Is there any reason not to use OPP and to implement
> another data structure to store a frequency table attached to a device?
>
>
> Cheers!
> MyungJoo.
>
> > ---
> > drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++++++++
> > include/linux/devfreq.h | 12 ++++++++++++
> > 2 files changed, 38 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 70c31d4..2144200 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -460,6 +460,17 @@ int devfreq_remove_device(struct devfreq *devfreq)
> > return 0;
> > }
> >
> > +/*
> > + * devfreq_set_freq_table()- Set frequency table for devfreq
> > + * @devfreq The devfreq instance
> > + * @table The frequency table that device supports
> > + */
> > +void devfreq_set_freq_table(struct devfreq *devfreq,
> > + struct devfreq_frequency_table *table)
> > +{
> > + devfreq->freq_table = table;
> > +}
> > +
> > static ssize_t show_governor(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > @@ -472,6 +483,20 @@ static ssize_t show_freq(struct device *dev,
> > return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq);
> > }
> >
> > +static ssize_t show_avail_freq(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int len = 0, i;
> > + struct devfreq *devfreq = to_devfreq(dev);
> > + if (devfreq->freq_table)
> > + for (i = 0; devfreq->freq_table[i].frequency !=
> DEVFREQ_TABLE_END; i++)
> > + len += sprintf(buf + len, "%lu\n",
> > + devfreq->freq_table[i].frequency);
> > + if (len == 0)
> > + len += sprintf(buf + len, "No frequency table is
> provided\n");
> > + return len;
> > +}
> > +
> > static ssize_t show_polling_interval(struct device *dev,
> > struct device_attribute *attr, char
> *buf)
> > {
> > @@ -595,6 +620,7 @@ static struct device_attribute devfreq_attrs[] = {
> > store_polling_interval),
> > __ATTR(min_freq, S_IRUGO | S_IWUSR, show_min_freq, store_min_freq),
> > __ATTR(max_freq, S_IRUGO | S_IWUSR, show_max_freq, store_max_freq),
> > + __ATTR(available_freqs, S_IRUGO, show_avail_freq, NULL),
> > { },
> > };
> >
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index 281c72a..e5e4036 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -52,6 +52,14 @@ struct devfreq_dev_status {
> > */
> > #define DEVFREQ_FLAG_LEAST_UPPER_BOUND 0x1
> >
> > +#define DEVFREQ_ENTRY_INVALID (~0)
> > +#define DEVFREQ_TABLE_END (~1)
> > +
> > +struct devfreq_frequency_table {
> > + unsigned int index;
> > + unsigned long frequency;
> > +};
> > +
> > /**
> > * struct devfreq_dev_profile - Devfreq's user device profile
> > * @initial_freq The operating frequency when devfreq_add_device()
> is
> > @@ -130,6 +138,7 @@ struct devfreq_governor {
> > * "devfreq_monitor" executions to reevaluate
> > * frequency/voltage of the device. Set by
> > * profile's polling_ms interval.
> > + * @freq_table The frequency table that device supports
> > * @data Private data of the governor. The devfreq framework does
> not
> > * touch this.
> > * @being_removed a flag to mark that this object is being removed in
> > @@ -157,6 +166,7 @@ struct devfreq {
> > unsigned long polling_jiffies;
> > unsigned long previous_freq;
> > unsigned int next_polling;
> > + struct devfreq_frequency_table *freq_table;
> >
> > void *data; /* private data for governors */
> >
> > @@ -180,6 +190,8 @@ extern int devfreq_register_opp_notifier(struct
> device *dev,
> > struct devfreq *devfreq);
> > extern int devfreq_unregister_opp_notifier(struct device *dev,
> > struct devfreq *devfreq);
> > +extern void devfreq_set_freq_table(struct devfreq *devfreq,
> > + struct devfreq_frequency_table *table);
> >
> > #ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
> > extern const struct devfreq_governor devfreq_powersave;
> > --
> > 1.7.0.4
> >
>
[-- Attachment #1.2: Type: text/html, Size: 6428 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* RE: [PATCH 2/2] PM: Devfreq: Add frequency get function in profile
From: Xiaoguang Chen @ 2012-06-13 5:16 UTC (permalink / raw)
To: myungjoo.ham@samsung.com, linux-kernel@vger.kernel.org
Cc: 박경민, linux-pm@lists.linux-foundation.org
In-Reply-To: <1290271.915811339563455292.JavaMail.weblogic@epml04>
Ok, I got it. So this patch has no too much use.
Thanks
Xiaoguang
-----Original Message-----
From: MyungJoo Ham [mailto:myungjoo.ham@samsung.com]
Sent: 2012年6月13日 12:58
To: Xiaoguang Chen; linux-kernel@vger.kernel.org
Cc: 박경민; linux-pm@lists.linux-foundation.org
Subject: Re: [PATCH 2/2] PM: Devfreq: Add frequency get function in profile
> when devfreq set one frequency, the final frequency may not the same
> as the requested frequency. Add get function in profile to let devfreq
> driver return the final frequency.
>
> Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
That is why in update_freq() function that you've modified uses:
err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
not
err = devfreq->profile->target(devfreq->dev.parent, freq, flags);
The target function will return the configured frequency right there.
Calling "__devfreq_get()" at the same location wouldn't help as long as the "target()" callback is implemented correctly.
Cheers!
MyungJoo
> ---
> drivers/devfreq/devfreq.c | 12 +++++++++++-
> include/linux/devfreq.h | 2 ++
> 2 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2144200..50a4fc0 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -72,6 +72,16 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> return ERR_PTR(-ENODEV);
> }
>
> +static unsigned long __devfreq_get(struct device *dev) {
> + struct devfreq *devfreq = to_devfreq(dev);
> + unsigned long ret_freq = devfreq->previous_freq;
> + if (!devfreq->profile->get)
> + return ret_freq;
> + ret_freq = devfreq->profile->get(devfreq->dev.parent);
> + return ret_freq;
> +}
> +
> /**
> * update_devfreq() - Reevaluate the device and configure frequency.
> * @devfreq: the devfreq instance.
> @@ -116,7 +126,7 @@ int update_devfreq(struct devfreq *devfreq)
> if (err)
> return err;
>
> - devfreq->previous_freq = freq;
> + devfreq->previous_freq = __devfreq_get(&devfreq->dev);
> return err;
> }
>
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index
> e5e4036..f789314 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -65,6 +65,7 @@ struct devfreq_frequency_table {
> * @initial_freq The operating frequency when devfreq_add_device() is
> * called.
> * @polling_ms The polling interval in ms. 0 disables polling.
> + * @get Get the devices's actual frequency
> * @target The device should set its operating frequency at
> * freq or lowest-upper-than-freq value. If freq is
> * higher than any operable frequency, set maximum.
> @@ -84,6 +85,7 @@ struct devfreq_dev_profile {
> unsigned long initial_freq;
> unsigned int polling_ms;
>
> + unsigned long (*get)(struct device *dev);
> int (*target)(struct device *dev, unsigned long *freq, u32 flags);
> int (*get_dev_status)(struct device *dev,
> struct devfreq_dev_status *stat);
> --
> 1.7.0.4
^ permalink raw reply
* Re: [PATCH 2/2] PM: Devfreq: Add frequency get function in profile
From: MyungJoo Ham @ 2012-06-13 4:57 UTC (permalink / raw)
To: Xiaoguang Chen, linux-kernel@vger.kernel.org
Cc: 박경민, linux-pm@lists.linux-foundation.org
> when devfreq set one frequency, the final frequency may not
> the same as the requested frequency. Add get function in profile
> to let devfreq driver return the final frequency.
>
> Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
That is why in update_freq() function that you've modified uses:
err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
not
err = devfreq->profile->target(devfreq->dev.parent, freq, flags);
The target function will return the configured frequency right there.
Calling "__devfreq_get()" at the same location wouldn't help as long as
the "target()" callback is implemented correctly.
Cheers!
MyungJoo
> ---
> drivers/devfreq/devfreq.c | 12 +++++++++++-
> include/linux/devfreq.h | 2 ++
> 2 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2144200..50a4fc0 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -72,6 +72,16 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> return ERR_PTR(-ENODEV);
> }
>
> +static unsigned long __devfreq_get(struct device *dev)
> +{
> + struct devfreq *devfreq = to_devfreq(dev);
> + unsigned long ret_freq = devfreq->previous_freq;
> + if (!devfreq->profile->get)
> + return ret_freq;
> + ret_freq = devfreq->profile->get(devfreq->dev.parent);
> + return ret_freq;
> +}
> +
> /**
> * update_devfreq() - Reevaluate the device and configure frequency.
> * @devfreq: the devfreq instance.
> @@ -116,7 +126,7 @@ int update_devfreq(struct devfreq *devfreq)
> if (err)
> return err;
>
> - devfreq->previous_freq = freq;
> + devfreq->previous_freq = __devfreq_get(&devfreq->dev);
> return err;
> }
>
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index e5e4036..f789314 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -65,6 +65,7 @@ struct devfreq_frequency_table {
> * @initial_freq The operating frequency when devfreq_add_device() is
> * called.
> * @polling_ms The polling interval in ms. 0 disables polling.
> + * @get Get the devices's actual frequency
> * @target The device should set its operating frequency at
> * freq or lowest-upper-than-freq value. If freq is
> * higher than any operable frequency, set maximum.
> @@ -84,6 +85,7 @@ struct devfreq_dev_profile {
> unsigned long initial_freq;
> unsigned int polling_ms;
>
> + unsigned long (*get)(struct device *dev);
> int (*target)(struct device *dev, unsigned long *freq, u32 flags);
> int (*get_dev_status)(struct device *dev,
> struct devfreq_dev_status *stat);
> --
> 1.7.0.4
^ permalink raw reply
* Re: [PATCH 1/2] PM: devfreq: add freq table and available_freqs
From: MyungJoo Ham @ 2012-06-13 4:53 UTC (permalink / raw)
To: Xiaoguang Chen, linux-kernel@vger.kernel.org
Cc: 박경민, linux-pm
> Devfreq framework don't have a frequency table, add it
> for easy use.
>
> Signed-off-by: Xiaoguang Chen <chenxg@marvell.com>
If you need a predefined data structure to support frequency table,
you can simply use OPP, which has helper functions implemented in
devfreq subsystem. Is there any reason not to use OPP and to implement
another data structure to store a frequency table attached to a device?
Cheers!
MyungJoo.
> ---
> drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++++++++
> include/linux/devfreq.h | 12 ++++++++++++
> 2 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 70c31d4..2144200 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -460,6 +460,17 @@ int devfreq_remove_device(struct devfreq *devfreq)
> return 0;
> }
>
> +/*
> + * devfreq_set_freq_table()- Set frequency table for devfreq
> + * @devfreq The devfreq instance
> + * @table The frequency table that device supports
> + */
> +void devfreq_set_freq_table(struct devfreq *devfreq,
> + struct devfreq_frequency_table *table)
> +{
> + devfreq->freq_table = table;
> +}
> +
> static ssize_t show_governor(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -472,6 +483,20 @@ static ssize_t show_freq(struct device *dev,
> return sprintf(buf, "%lu\n", to_devfreq(dev)->previous_freq);
> }
>
> +static ssize_t show_avail_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int len = 0, i;
> + struct devfreq *devfreq = to_devfreq(dev);
> + if (devfreq->freq_table)
> + for (i = 0; devfreq->freq_table[i].frequency != DEVFREQ_TABLE_END; i++)
> + len += sprintf(buf + len, "%lu\n",
> + devfreq->freq_table[i].frequency);
> + if (len == 0)
> + len += sprintf(buf + len, "No frequency table is provided\n");
> + return len;
> +}
> +
> static ssize_t show_polling_interval(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> @@ -595,6 +620,7 @@ static struct device_attribute devfreq_attrs[] = {
> store_polling_interval),
> __ATTR(min_freq, S_IRUGO | S_IWUSR, show_min_freq, store_min_freq),
> __ATTR(max_freq, S_IRUGO | S_IWUSR, show_max_freq, store_max_freq),
> + __ATTR(available_freqs, S_IRUGO, show_avail_freq, NULL),
> { },
> };
>
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 281c72a..e5e4036 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -52,6 +52,14 @@ struct devfreq_dev_status {
> */
> #define DEVFREQ_FLAG_LEAST_UPPER_BOUND 0x1
>
> +#define DEVFREQ_ENTRY_INVALID (~0)
> +#define DEVFREQ_TABLE_END (~1)
> +
> +struct devfreq_frequency_table {
> + unsigned int index;
> + unsigned long frequency;
> +};
> +
> /**
> * struct devfreq_dev_profile - Devfreq's user device profile
> * @initial_freq The operating frequency when devfreq_add_device() is
> @@ -130,6 +138,7 @@ struct devfreq_governor {
> * "devfreq_monitor" executions to reevaluate
> * frequency/voltage of the device. Set by
> * profile's polling_ms interval.
> + * @freq_table The frequency table that device supports
> * @data Private data of the governor. The devfreq framework does not
> * touch this.
> * @being_removed a flag to mark that this object is being removed in
> @@ -157,6 +166,7 @@ struct devfreq {
> unsigned long polling_jiffies;
> unsigned long previous_freq;
> unsigned int next_polling;
> + struct devfreq_frequency_table *freq_table;
>
> void *data; /* private data for governors */
>
> @@ -180,6 +190,8 @@ extern int devfreq_register_opp_notifier(struct device *dev,
> struct devfreq *devfreq);
> extern int devfreq_unregister_opp_notifier(struct device *dev,
> struct devfreq *devfreq);
> +extern void devfreq_set_freq_table(struct devfreq *devfreq,
> + struct devfreq_frequency_table *table);
>
> #ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
> extern const struct devfreq_governor devfreq_powersave;
> --
> 1.7.0.4
>
^ permalink raw reply
* Re: [RFC] [PATCH 1/4] thermal: Use thermal zone device id in netlink messages
From: Valentin, Eduardo @ 2012-06-13 3:57 UTC (permalink / raw)
To: R, Durgadoss; +Cc: linux-pm@lists.linux-foundation.org
In-Reply-To: <4D68720C2E767A4AA6A8796D42C8EB59136298@BGSMSX101.gar.corp.intel.com>
Hello Durga,
On Tue, Jun 12, 2012 at 8:02 PM, R, Durgadoss <durgadoss.r@intel.com> wrote:
> Hi Eduardo,
>
>>
>> This patch changes the function thermal_generate_netlink_event
>> to receive a thermal zone device instead of a originator id.
>>
>> This way, the messages will always be bound to a thermal zone.
>
> I agree with you on this patch implementation.
>
Ok..
> But I am thinking most user space Apps today are using UEvent based
> mechanism, may be it is time for us to re-visit this implemention.
> There are standard UEvent parsers available which can be used.
> (instead of writing custom netlink parsing implementations)
Right... But do we have any legacy application relying on this netlink channel?
>
> I submitted this patch an year ago, (inspired by some acpi driver,
> which uses this same netlink mechanism) and have a couple of
> platforms using this. I can change them for my side.
Oh.. Ok. Well, on my side, I don't really have any application
relying on it. I just want to make sure we don't break things while
changing the current implementation. If you are OK to change your
applications then fine. We can also, introduce the sysfs notify and
mark the netlink as deprecated, while the applications are being
modified. Then finally remove it.
>
> I don't know what other platforms are using this. Need Rui's
> help/suggestion here.
Indeed. Rui?
>
> Thanks,
> Durga
All best,
--
Eduardo Valentin
^ permalink raw reply
* Re: [RFC PATCH 3/12] thermal: set upper and lower limits when binding
From: Zhang Rui @ 2012-06-13 0:51 UTC (permalink / raw)
To: eduardo.valentin
Cc: linux-pm, linux-acpi@vger.kernel.org, Amit Kachhap, R, Durgadoss,
Len, Brown, Rafael J. Wysocki, Matthew Garrett
In-Reply-To: <20120612095215.GA3837@besouro>
On 二, 2012-06-12 at 12:52 +0300, Eduardo Valentin wrote:
> Hello Rui,
>
> On Mon, Jun 11, 2012 at 11:20:01AM +0800, Zhang Rui wrote:
> >
> > Set upper and lower limits when binding
> > a thermal cooling device to a thermal zone device.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/acpi/thermal.c | 34 ++++++++++++++++++++++++----------
> > drivers/platform/x86/acerhdf.c | 2 +-
> > drivers/thermal/thermal_sys.c | 20 ++++++++++++++------
> > include/linux/thermal.h | 3 ++-
> > 4 files changed, 41 insertions(+), 18 deletions(-)
> >
> > Index: rtd3/drivers/thermal/thermal_sys.c
> > ===================================================================
> > --- rtd3.orig/drivers/thermal/thermal_sys.c
> > +++ rtd3/drivers/thermal/thermal_sys.c
> > @@ -248,7 +248,7 @@ passive_store(struct device *dev, struct
> > sizeof("Processor")))
> > thermal_zone_bind_cooling_device(tz,
> > THERMAL_TRIPS_NONE,
> > - cdev);
> > + cdev, -1, -1);
> > }
> > mutex_unlock(&thermal_list_lock);
> > if (!tz->passive_delay)
> > @@ -760,7 +760,8 @@ static void thermal_zone_device_check(st
> > */
> > int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
> > int trip,
> > - struct thermal_cooling_device *cdev)
> > + struct thermal_cooling_device *cdev,
> > + long upper, long lower)
> > {
> > struct thermal_cooling_device_instance *dev;
> > struct thermal_cooling_device_instance *pos;
> > @@ -784,6 +785,15 @@ int thermal_zone_bind_cooling_device(str
> > if (tz != pos1 || cdev != pos2)
> > return -EINVAL;
> >
> > + cdev->ops->get_max_state(cdev, &max_state);
> > +
> > + /* lower default 0, upper default max_state */
> > + lower = lower < 0 ? 0 : lower;
> > + upper = upper < 0 ? max_state : upper;
> > +
> > + if (lower > upper || upper > max_state)
> > + return -EINVAL;
> > +
>
> From this version of the code, I assume we want to:
> a. Allow cooling binding instances to have ranges overlapping.
yep.
> b. Allow a binding with only one state (lower == upper).
>
yep.
> Is this the expected behavior?
>
yes. :)
thanks,
rui
> > dev =
> > kzalloc(sizeof(struct thermal_cooling_device_instance), GFP_KERNEL);
> > if (!dev)
> > @@ -791,10 +801,8 @@ int thermal_zone_bind_cooling_device(str
> > dev->tz = tz;
> > dev->cdev = cdev;
> > dev->trip = trip;
> > -
> > - cdev->ops->get_max_state(dev, &max_state);
> > - dev->upper = max_state;
> > - dev->lower = 0;
> > + dev->upper = upper;
> > + dev->lower = lower;
> >
> > result = get_idr(&tz->idr, &tz->lock, &dev->id);
> > if (result)
> > Index: rtd3/include/linux/thermal.h
> > ===================================================================
> > --- rtd3.orig/include/linux/thermal.h
> > +++ rtd3/include/linux/thermal.h
> > @@ -143,7 +143,8 @@ struct thermal_zone_device *thermal_zone
> > void thermal_zone_device_unregister(struct thermal_zone_device *);
> >
> > int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
> > - struct thermal_cooling_device *);
> > + struct thermal_cooling_device *,
> > + long, long);
> > int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
> > struct thermal_cooling_device *);
> > void thermal_zone_device_update(struct thermal_zone_device *);
> > Index: rtd3/drivers/acpi/thermal.c
> > ===================================================================
> > --- rtd3.orig/drivers/acpi/thermal.c
> > +++ rtd3/drivers/acpi/thermal.c
> > @@ -729,11 +729,9 @@ static int thermal_notify(struct thermal
> > return 0;
> > }
> >
> > -typedef int (*cb)(struct thermal_zone_device *, int,
> > - struct thermal_cooling_device *);
> > static int acpi_thermal_cooling_device_cb(struct thermal_zone_device *thermal,
> > struct thermal_cooling_device *cdev,
> > - cb action)
> > + int bind)
> > {
> > struct acpi_device *device = cdev->devdata;
> > struct acpi_thermal *tz = thermal->devdata;
> > @@ -758,7 +756,14 @@ static int acpi_thermal_cooling_device_c
> > handle = tz->trips.passive.devices.handles[i];
> > status = acpi_bus_get_device(handle, &dev);
> > if (ACPI_SUCCESS(status) && (dev == device)) {
> > - result = action(thermal, trip, cdev);
> > + if (bind)
> > + result =
> > + thermal_zone_bind_cooling_device(
> > + thermal, trip, cdev, -1, -1);
> > + else
> > + result =
> > + thermal_zone_unbind_cooling_device(
> > + thermal, trip, cdev);
> > if (result)
> > goto failed;
> > }
> > @@ -775,7 +780,13 @@ static int acpi_thermal_cooling_device_c
> > handle = tz->trips.active[i].devices.handles[j];
> > status = acpi_bus_get_device(handle, &dev);
> > if (ACPI_SUCCESS(status) && (dev == device)) {
> > - result = action(thermal, trip, cdev);
> > + if (bind)
> > + result =
> > + thermal_zone_bind_cooling_device(
> > + thermal, trip, cdev, -1, -1);
> > + else
> > + result = thermal_zone_unbind_cooling_device(
> > + thermal, trip, cdev);
> > if (result)
> > goto failed;
> > }
> > @@ -786,7 +797,12 @@ static int acpi_thermal_cooling_device_c
> > handle = tz->devices.handles[i];
> > status = acpi_bus_get_device(handle, &dev);
> > if (ACPI_SUCCESS(status) && (dev == device)) {
> > - result = action(thermal, -1, cdev);
> > + if (bind)
> > + result = thermal_zone_bind_cooling_device(thermal,
> > + -1, cdev, -1, -1);
> > + else
> > + result = thermal_zone_unbind_cooling_device(thermal,
> > + -1, cdev);
> > if (result)
> > goto failed;
> > }
> > @@ -800,16 +816,14 @@ static int
> > acpi_thermal_bind_cooling_device(struct thermal_zone_device *thermal,
> > struct thermal_cooling_device *cdev)
> > {
> > - return acpi_thermal_cooling_device_cb(thermal, cdev,
> > - thermal_zone_bind_cooling_device);
> > + return acpi_thermal_cooling_device_cb(thermal, cdev, 1);
> > }
> >
> > static int
> > acpi_thermal_unbind_cooling_device(struct thermal_zone_device *thermal,
> > struct thermal_cooling_device *cdev)
> > {
> > - return acpi_thermal_cooling_device_cb(thermal, cdev,
> > - thermal_zone_unbind_cooling_device);
> > + return acpi_thermal_cooling_device_cb(thermal, cdev, 0);
> > }
> >
> > static const struct thermal_zone_device_ops acpi_thermal_zone_ops = {
> > Index: rtd3/drivers/platform/x86/acerhdf.c
> > ===================================================================
> > --- rtd3.orig/drivers/platform/x86/acerhdf.c
> > +++ rtd3/drivers/platform/x86/acerhdf.c
> > @@ -329,7 +329,7 @@ static int acerhdf_bind(struct thermal_z
> > if (cdev != cl_dev)
> > return 0;
> >
> > - if (thermal_zone_bind_cooling_device(thermal, 0, cdev)) {
> > + if (thermal_zone_bind_cooling_device(thermal, 0, cdev, -1, -1)) {
> > pr_err("error binding cooling dev\n");
> > return -EINVAL;
> > }
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] [PATCH 1/4] thermal: Use thermal zone device id in netlink messages
From: R, Durgadoss @ 2012-06-12 17:02 UTC (permalink / raw)
To: Eduardo Valentin, Zhang, Rui; +Cc: linux-pm@lists.linux-foundation.org
In-Reply-To: <1339507519-14434-2-git-send-email-eduardo.valentin@ti.com>
Hi Eduardo,
>
> This patch changes the function thermal_generate_netlink_event
> to receive a thermal zone device instead of a originator id.
>
> This way, the messages will always be bound to a thermal zone.
I agree with you on this patch implementation.
But I am thinking most user space Apps today are using UEvent based
mechanism, may be it is time for us to re-visit this implemention.
There are standard UEvent parsers available which can be used.
(instead of writing custom netlink parsing implementations)
I submitted this patch an year ago, (inspired by some acpi driver,
which uses this same netlink mechanism) and have a couple of
platforms using this. I can change them for my side.
I don't know what other platforms are using this. Need Rui's
help/suggestion here.
Thanks,
Durga
^ permalink raw reply
* Re: [RFC PATCH 7/12] thermal: rename structure thermal_cooling_device_instance to thermal_instance
From: Eduardo Valentin @ 2012-06-12 14:39 UTC (permalink / raw)
To: Zhang Rui
Cc: linux-pm, Amit Kachhap, eduardo, R, Durgadoss, Len, Brown,
Rafael J. Wysocki, Matthew Garrett, linux-acpi@vger.kernel.org
In-Reply-To: <1339395696.1492.179.camel@rui.sh.intel.com>
Hello Rui,
On Mon, Jun 11, 2012 at 02:21:36PM +0800, Zhang Rui wrote:
> seems this patch is missed. Resend it.
>
> On 一, 2012-06-11 at 11:20 +0800, Zhang Rui wrote:
> > Rename structure thermal_cooling_device_instance to thermal_instance.
> >
> > This struct is used to describe the behavior for a thermal
> > cooling device on a certain trip point for a certain thremal zone.
typo
> >
> > thermal_cooling_device_instance is not accurate, as a cooling device
> > can be used for more than one trip point in one thermal zone device.
'thermal_instance' seams to be a bit misleading though...
It looks a bit too generic. One reading it won't figure it out that
it is supposed the mapping between cooling device vs. trip vs. thermal zone...
How about 'thermal_mapping' or 'thermal_binding'?
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/thermal/thermal_sys.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > Index: rtd3/drivers/thermal/thermal_sys.c
> > ===================================================================
> > --- rtd3.orig/drivers/thermal/thermal_sys.c
> > +++ rtd3/drivers/thermal/thermal_sys.c
> > @@ -46,7 +46,7 @@ MODULE_LICENSE("GPL");
> > * a certain cooling device on a certain trip point
> > * in a certain thermal zone
> > */
> > -struct thermal_cooling_device_instance {
> > +struct thermal_instance {
> > int id;
> > char name[THERMAL_NAME_LENGTH];
> > struct thermal_zone_device *tz;
> > @@ -388,10 +388,10 @@ static ssize_t
> > thermal_cooling_device_trip_point_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > - struct thermal_cooling_device_instance *instance;
> > + struct thermal_instance *instance;
> >
> > instance =
> > - container_of(attr, struct thermal_cooling_device_instance, attr);
> > + container_of(attr, struct thermal_instance, attr);
> >
> > if (instance->trip == THERMAL_TRIPS_NONE)
> > return sprintf(buf, "-1\n");
> > @@ -686,7 +686,7 @@ static void thermal_zone_device_passive(
> > int temp, int trip_temp, int trip)
> > {
> > int trend = 0;
> > - struct thermal_cooling_device_instance *instance;
> > + struct thermal_instance *instance;
> > struct thermal_cooling_device *cdev;
> > long state, max_state;
> >
> > @@ -771,8 +771,8 @@ int thermal_zone_bind_cooling_device(str
> > struct thermal_cooling_device *cdev,
> > long upper, long lower)
> > {
> > - struct thermal_cooling_device_instance *dev;
> > - struct thermal_cooling_device_instance *pos;
> > + struct thermal_instance *dev;
> > + struct thermal_instance *pos;
> > struct thermal_zone_device *pos1;
> > struct thermal_cooling_device *pos2;
> > unsigned long max_state;
> > @@ -803,7 +803,7 @@ int thermal_zone_bind_cooling_device(str
> > return -EINVAL;
> >
> > dev =
> > - kzalloc(sizeof(struct thermal_cooling_device_instance), GFP_KERNEL);
> > + kzalloc(sizeof(struct thermal_instance), GFP_KERNEL);
> > if (!dev)
> > return -ENOMEM;
> > dev->tz = tz;
> > @@ -868,7 +868,7 @@ int thermal_zone_unbind_cooling_device(s
> > int trip,
> > struct thermal_cooling_device *cdev)
> > {
> > - struct thermal_cooling_device_instance *pos, *next;
> > + struct thermal_instance *pos, *next;
> >
> > mutex_lock(&tz->lock);
> > list_for_each_entry_safe(pos, next, &tz->cooling_devices, node) {
> > @@ -1054,7 +1054,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unr
> > static void thermal_zone_trip_update(struct thermal_zone_device *tz,
> > int trip, long temp)
> > {
> > - struct thermal_cooling_device_instance *instance;
> > + struct thermal_instance *instance;
> > struct thermal_cooling_device *cdev = NULL;
> > unsigned long cur_state, max_state;
> > long trip_temp;
> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 4/4] thermal sys: check for invalid trip setup when registering thermal device
From: Eduardo Valentin @ 2012-06-12 13:25 UTC (permalink / raw)
To: rui.zhang, durgadoss.r; +Cc: linux-pm
In-Reply-To: <1339507519-14434-1-git-send-email-eduardo.valentin@ti.com>
This patch adds an extra check in the data structure while registering
a thermal device. The check is to avoid registering zones with a number
of trips greater than zero, but with no .get_trip_temp nor .get_trip_type
callbacks. Receiving such data structure may end in wrong data access.
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
drivers/thermal/thermal_sys.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 61d0003..50830f2 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1129,6 +1129,9 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
if (!ops || !ops->get_temp)
return ERR_PTR(-EINVAL);
+ if (trips > 0 && !ops->get_trip_type)
+ return ERR_PTR(-EINVAL);
+
tz = kzalloc(sizeof(struct thermal_zone_device), GFP_KERNEL);
if (!tz)
return ERR_PTR(-ENOMEM);
--
1.7.7.1.488.ge8e1c
^ permalink raw reply related
* [PATCH 3/4] thermal: cleanup: use dev_* helper functions
From: Eduardo Valentin @ 2012-06-12 13:25 UTC (permalink / raw)
To: rui.zhang, durgadoss.r; +Cc: linux-pm
In-Reply-To: <1339507519-14434-1-git-send-email-eduardo.valentin@ti.com>
Change the logging messages to used dev_* helper functions.
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
drivers/thermal/thermal_sys.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 1ef9fc1..61d0003 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1022,7 +1022,8 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
if (tz->ops->get_temp(tz, &temp)) {
/* get_temp failed - retry it later */
- pr_warn("failed to read out thermal zone %d\n", tz->id);
+ dev_warn(&tz->device, "Failed to read out thermal zone %d\n",
+ tz->id);
goto leave;
}
@@ -1037,8 +1038,10 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
ret = tz->ops->notify(tz, count,
trip_type);
if (!ret) {
- pr_emerg("Critical temperature reached (%ld C), shutting down\n",
- temp/1000);
+ dev_emerg(&tz->device,
+ "Critical temperature reached"
+ " (%ld C), shutting down.\n",
+ temp/1000);
orderly_poweroff(true);
}
}
@@ -1346,7 +1349,7 @@ int thermal_generate_netlink_event(struct thermal_zone_device *tz,
result = genlmsg_multicast(skb, 0, thermal_event_mcgrp.id, GFP_ATOMIC);
if (result)
- pr_info("failed to send netlink event:%d\n", result);
+ dev_err(&tz->device, "Failed to send netlink event:%d", result);
return result;
}
--
1.7.7.1.488.ge8e1c
^ permalink raw reply related
* [PATCH 2/4] thermal: remove unnecessary include
From: Eduardo Valentin @ 2012-06-12 13:25 UTC (permalink / raw)
To: rui.zhang, durgadoss.r; +Cc: linux-pm
In-Reply-To: <1339507519-14434-1-git-send-email-eduardo.valentin@ti.com>
No need for spinlocks in this file, then removing its header.
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
drivers/thermal/thermal_sys.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 650cefb..1ef9fc1 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -32,7 +32,6 @@
#include <linux/kdev_t.h>
#include <linux/idr.h>
#include <linux/thermal.h>
-#include <linux/spinlock.h>
#include <linux/reboot.h>
#include <net/netlink.h>
#include <net/genetlink.h>
--
1.7.7.1.488.ge8e1c
^ permalink raw reply related
* [RFC] [PATCH 1/4] thermal: Use thermal zone device id in netlink messages
From: Eduardo Valentin @ 2012-06-12 13:25 UTC (permalink / raw)
To: rui.zhang, durgadoss.r; +Cc: linux-pm
In-Reply-To: <1339507519-14434-1-git-send-email-eduardo.valentin@ti.com>
This patch changes the function thermal_generate_netlink_event
to receive a thermal zone device instead of a originator id.
This way, the messages will always be bound to a thermal zone.
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
Documentation/thermal/sysfs-api.txt | 5 +++--
drivers/thermal/thermal_sys.c | 8 ++++++--
include/linux/thermal.h | 6 ++++--
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 1733ab9..670644a 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt
@@ -285,8 +285,9 @@ The framework includes a simple notification mechanism, in the form of a
netlink event. Netlink socket initialization is done during the _init_
of the framework. Drivers which intend to use the notification mechanism
just need to call thermal_generate_netlink_event() with two arguments viz
-(originator, event). Typically the originator will be an integer assigned
-to a thermal_zone_device when it registers itself with the framework. The
+(originator, event). The originator is a pointer to struct thermal_zone_device
+from where the event has been originated. An integer which represents the
+thermal zone device will be used in the message to identify the zone. The
event will be one of:{THERMAL_AUX0, THERMAL_AUX1, THERMAL_CRITICAL,
THERMAL_DEV_FAULT}. Notification can be sent when the current temperature
crosses any of the configured thresholds.
diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c
index 022bacb..650cefb 100644
--- a/drivers/thermal/thermal_sys.c
+++ b/drivers/thermal/thermal_sys.c
@@ -1287,7 +1287,8 @@ static struct genl_multicast_group thermal_event_mcgrp = {
.name = THERMAL_GENL_MCAST_GROUP_NAME,
};
-int thermal_generate_netlink_event(u32 orig, enum events event)
+int thermal_generate_netlink_event(struct thermal_zone_device *tz,
+ enum events event)
{
struct sk_buff *skb;
struct nlattr *attr;
@@ -1297,6 +1298,9 @@ int thermal_generate_netlink_event(u32 orig, enum events event)
int result;
static unsigned int thermal_event_seqnum;
+ if (!tz)
+ return -EINVAL;
+
/* allocate memory */
size = nla_total_size(sizeof(struct thermal_genl_event)) +
nla_total_size(0);
@@ -1331,7 +1335,7 @@ int thermal_generate_netlink_event(u32 orig, enum events event)
memset(thermal_event, 0, sizeof(struct thermal_genl_event));
- thermal_event->orig = orig;
+ thermal_event->orig = tz->id;
thermal_event->event = event;
/* send multicast genetlink message */
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 796f1ff..b754203 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -152,9 +152,11 @@ struct thermal_cooling_device *thermal_cooling_device_register(char *, void *,
void thermal_cooling_device_unregister(struct thermal_cooling_device *);
#ifdef CONFIG_NET
-extern int thermal_generate_netlink_event(u32 orig, enum events event);
+extern int thermal_generate_netlink_event(struct thermal_zone_device *tz,
+ enum events event);
#else
-static inline int thermal_generate_netlink_event(u32 orig, enum events event)
+static int thermal_generate_netlink_event(struct thermal_zone_device *tz,
+ enum events event)
{
return 0;
}
--
1.7.7.1.488.ge8e1c
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox