Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700359 > unrolled thread

[PATCH] x86/hpet: Cure interface abuse in the resume path

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/hpet: Cure interface abuse in the resume path Thomas Gleixner <tglx@linutronix.de> - 2017-07-31 22:10 +0200
    Re: [PATCH] x86/hpet: Cure interface abuse in the resume path "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> - 2017-08-01 00:30 +0200
    Re: [PATCH] x86/hpet: Cure interface abuse in the resume path Tomi Sarvela <tomi.p.sarvela@intel.com> - 2017-08-01 09:40 +0200
      Re: [PATCH] x86/hpet: Cure interface abuse in the resume path Thomas Gleixner <tglx@linutronix.de> - 2017-08-01 09:50 +0200
        Re: [PATCH] x86/hpet: Cure interface abuse in the resume path Thomas Gleixner <tglx@linutronix.de> - 2017-08-01 13:00 +0200
    [tip:x86/urgent] x86/hpet: Cure interface abuse in the resume path tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-08-01 13:20 +0200

#1700359 — [PATCH] x86/hpet: Cure interface abuse in the resume path

The HPET resume path abuses irq_domain_[de]activate_irq() to restore the
MSI message in the HPET chip for the boot CPU on resume and it relies on an
implementation detail of the interrupt core code, which magically makes the
HPET unmask call invoked via a irq_disable/enable pair. This worked as long
as the irq code did unconditionally invoke the unmask() callback. With the
recent changes which keep track of the masked state to avoid expensive
hardware access, this does not longer work. As a consequence the HPET timer
interrupts are not unmasked which breaks resume as the boot CPU waits
forever that a timer interrupt arrives.

Make the restore of the MSI message explicit and invoke the unmask()
function directly. While at it get rid of the pointless affinity setting as
nothing can change the affinity of the interrupt and the vector across
suspend/resume. The restore of the MSI message reestablishes the previous
affinity setting which is the correct one.

Fixes: bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
Reported-by: Martin Peres <martin.peres@linux.intel.com>
Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: jeffy.chen@rock-chips.com
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Peter Ziljstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
---
 arch/x86/kernel/hpet.c |   25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -345,21 +345,10 @@ static int hpet_shutdown(struct clock_ev
 	return 0;
 }
 
-static int hpet_resume(struct clock_event_device *evt, int timer)
+static int hpet_resume(struct clock_event_device *evt)
 {
-	if (!timer) {
-		hpet_enable_legacy_int();
-	} else {
-		struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
-
-		irq_domain_deactivate_irq(irq_get_irq_data(hdev->irq));
-		irq_domain_activate_irq(irq_get_irq_data(hdev->irq));
-		disable_hardirq(hdev->irq);
-		irq_set_affinity(hdev->irq, cpumask_of(hdev->cpu));
-		enable_irq(hdev->irq);
-	}
+	hpet_enable_legacy_int();
 	hpet_print_config();
-
 	return 0;
 }
 
@@ -417,7 +406,7 @@ static int hpet_legacy_set_periodic(stru
 
 static int hpet_legacy_resume(struct clock_event_device *evt)
 {
-	return hpet_resume(evt, 0);
+	return hpet_resume(evt);
 }
 
 static int hpet_legacy_next_event(unsigned long delta,
@@ -510,8 +499,14 @@ static int hpet_msi_set_periodic(struct
 static int hpet_msi_resume(struct clock_event_device *evt)
 {
 	struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
+	struct irq_data *data = irq_get_irq_data(hdev->irq);
+	struct msi_msg msg;
 
-	return hpet_resume(evt, hdev->num);
+	/* Restore the MSI msg and unmask the interrupt */
+	irq_chip_compose_msi_msg(data, &msg);
+	hpet_msi_write(hdev, &msg);
+	hpet_msi_unmask(data);
+	return 0;
 }
 
 static int hpet_msi_next_event(unsigned long delta,

[toc] | [next] | [standalone]

#1700438

On 7/31/2017 10:07 PM, Thomas Gleixner wrote:
> The HPET resume path abuses irq_domain_[de]activate_irq() to restore the
> MSI message in the HPET chip for the boot CPU on resume and it relies on an
> implementation detail of the interrupt core code, which magically makes the
> HPET unmask call invoked via a irq_disable/enable pair. This worked as long
> as the irq code did unconditionally invoke the unmask() callback. With the
> recent changes which keep track of the masked state to avoid expensive
> hardware access, this does not longer work. As a consequence the HPET timer
> interrupts are not unmasked which breaks resume as the boot CPU waits
> forever that a timer interrupt arrives.
>
> Make the restore of the MSI message explicit and invoke the unmask()
> function directly. While at it get rid of the pointless affinity setting as
> nothing can change the affinity of the interrupt and the vector across
> suspend/resume. The restore of the MSI message reestablishes the previous
> affinity setting which is the correct one.
>
> Fixes: bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> Reported-by: Martin Peres <martin.peres@linux.intel.com>
> Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: jeffy.chen@rock-chips.com
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Peter Ziljstra <peterz@infradead.org>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

ACK

[toc] | [prev] | [next] | [standalone]

#1700695

On 31/07/17 23:07, Thomas Gleixner wrote:
> The HPET resume path abuses irq_domain_[de]activate_irq() to restore the
> MSI message in the HPET chip for the boot CPU on resume and it relies on an
> implementation detail of the interrupt core code, which magically makes the
> HPET unmask call invoked via a irq_disable/enable pair. This worked as long
> as the irq code did unconditionally invoke the unmask() callback. With the
> recent changes which keep track of the masked state to avoid expensive
> hardware access, this does not longer work. As a consequence the HPET timer
> interrupts are not unmasked which breaks resume as the boot CPU waits
> forever that a timer interrupt arrives.
> 
> Make the restore of the MSI message explicit and invoke the unmask()
> function directly. While at it get rid of the pointless affinity setting as
> nothing can change the affinity of the interrupt and the vector across
> suspend/resume. The restore of the MSI message reestablishes the previous
> affinity setting which is the correct one.
> 
> Fixes: bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> Reported-by: Martin Peres <martin.peres@linux.intel.com>
> Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: jeffy.chen@rock-chips.com
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Peter Ziljstra <peterz@infradead.org>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Tested-by: Tomi Sarvela <tomi.p.sarvela@intel.com>

Tested only on the regressed Eagle Lake testhost. This patch fixes the 
suspend/resume issue.

Tomi
-- 
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

[toc] | [prev] | [next] | [standalone]

#1700702

On Tue, 1 Aug 2017, Tomi Sarvela wrote:
> On 31/07/17 23:07, Thomas Gleixner wrote:
> > The HPET resume path abuses irq_domain_[de]activate_irq() to restore the
> > MSI message in the HPET chip for the boot CPU on resume and it relies on an
> > implementation detail of the interrupt core code, which magically makes the
> > HPET unmask call invoked via a irq_disable/enable pair. This worked as long
> > as the irq code did unconditionally invoke the unmask() callback. With the
> > recent changes which keep track of the masked state to avoid expensive
> > hardware access, this does not longer work. As a consequence the HPET timer
> > interrupts are not unmasked which breaks resume as the boot CPU waits
> > forever that a timer interrupt arrives.
> > 
> > Make the restore of the MSI message explicit and invoke the unmask()
> > function directly. While at it get rid of the pointless affinity setting as
> > nothing can change the affinity of the interrupt and the vector across
> > suspend/resume. The restore of the MSI message reestablishes the previous
> > affinity setting which is the correct one.
> > 
> > Fixes: bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
> > calls")
> > Reported-by: Martin Peres <martin.peres@linux.intel.com>
> > Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: jeffy.chen@rock-chips.com
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Peter Ziljstra <peterz@infradead.org>
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> Tested-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
> 
> Tested only on the regressed Eagle Lake testhost. This patch fixes the
> suspend/resume issue.

Tomi, can you please do me a favor?

Use plain 4.13-rc3 (without that patch) and add the following on the kernel
command line: 'nohpet'. Boot the machine and capture and provide the output
of

# dmesg
# cat /proc/interrupts
# cat /proc/timer_list

Then try the suspend cycle again.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1700866

On Tue, 1 Aug 2017, Tomi Sarvela wrote:
> On 01/08/17 10:43, Thomas Gleixner wrote:
> > Tomi, can you please do me a favor?
> > 
> > Use plain 4.13-rc3 (without that patch) and add the following on the kernel
> > command line: 'nohpet'. Boot the machine and capture and provide the output
> > of
> > 
> > # dmesg
> > # cat /proc/interrupts
> > # cat /proc/timer_list
> > 
> > Then try the suspend cycle again.
> 
> Trying with Linus' tree, tag v4.13-rc3 with nohpet on cmdline.
> 
> Outputs below. The nohpet option works, as suspend-resumes complete.

So why didn't this work the first time I asked for that?

 http://lkml.kernel.org/r/ddc71535-02de-5b42-934f-70bc5ad43bb7@intel.com

Sigh, that would have spared a lot of pointlessly wasted time....

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1700888 — [tip:x86/urgent] x86/hpet: Cure interface abuse in the resume path

Commit-ID:  bb68cfe2f5a7f43058aed299fdbb73eb281734ed
Gitweb:     http://git.kernel.org/tip/bb68cfe2f5a7f43058aed299fdbb73eb281734ed
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Mon, 31 Jul 2017 22:07:09 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 1 Aug 2017 13:02:37 +0200

x86/hpet: Cure interface abuse in the resume path

The HPET resume path abuses irq_domain_[de]activate_irq() to restore the
MSI message in the HPET chip for the boot CPU on resume and it relies on an
implementation detail of the interrupt core code, which magically makes the
HPET unmask call invoked via a irq_disable/enable pair. This worked as long
as the irq code did unconditionally invoke the unmask() callback. With the
recent changes which keep track of the masked state to avoid expensive
hardware access, this does not longer work. As a consequence the HPET timer
interrupts are not unmasked which breaks resume as the boot CPU waits
forever that a timer interrupt arrives.

Make the restore of the MSI message explicit and invoke the unmask()
function directly. While at it get rid of the pointless affinity setting as
nothing can change the affinity of the interrupt and the vector across
suspend/resume. The restore of the MSI message reestablishes the previous
affinity setting which is the correct one.

Fixes: bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
Reported-and-tested-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
Reported-by: Martin Peres <martin.peres@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: jeffy.chen@rock-chips.com
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1707312158590.2287@nanos

---
 arch/x86/kernel/hpet.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 16f82a3..8ce4212 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -345,21 +345,10 @@ static int hpet_shutdown(struct clock_event_device *evt, int timer)
 	return 0;
 }
 
-static int hpet_resume(struct clock_event_device *evt, int timer)
-{
-	if (!timer) {
-		hpet_enable_legacy_int();
-	} else {
-		struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
-
-		irq_domain_deactivate_irq(irq_get_irq_data(hdev->irq));
-		irq_domain_activate_irq(irq_get_irq_data(hdev->irq));
-		disable_hardirq(hdev->irq);
-		irq_set_affinity(hdev->irq, cpumask_of(hdev->cpu));
-		enable_irq(hdev->irq);
-	}
+static int hpet_resume(struct clock_event_device *evt)
+{
+	hpet_enable_legacy_int();
 	hpet_print_config();
-
 	return 0;
 }
 
@@ -417,7 +406,7 @@ static int hpet_legacy_set_periodic(struct clock_event_device *evt)
 
 static int hpet_legacy_resume(struct clock_event_device *evt)
 {
-	return hpet_resume(evt, 0);
+	return hpet_resume(evt);
 }
 
 static int hpet_legacy_next_event(unsigned long delta,
@@ -510,8 +499,14 @@ static int hpet_msi_set_periodic(struct clock_event_device *evt)
 static int hpet_msi_resume(struct clock_event_device *evt)
 {
 	struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
+	struct irq_data *data = irq_get_irq_data(hdev->irq);
+	struct msi_msg msg;
 
-	return hpet_resume(evt, hdev->num);
+	/* Restore the MSI msg and unmask the interrupt */
+	irq_chip_compose_msi_msg(data, &msg);
+	hpet_msi_write(hdev, &msg);
+	hpet_msi_unmask(data);
+	return 0;
 }
 
 static int hpet_msi_next_event(unsigned long delta,

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web