Linux-Next Archive on lore.kernel.org
 help / color / Atom feed
From: Mikulas Patocka <mpatocka@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>,
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@elte.hu>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	linux-next@vger.kernel.org, linux-kernel@vger.kernel.org,
	cpufreq@vger.kernel.org
Subject: Re: linux-next: build failure after merge of the tip tree
Date: Tue, 14 Jan 2014 16:43:43 -0500 (EST)
Message-ID: <alpine.LRH.2.02.1401141629520.19206@file01.intranet.prod.int.rdu2.redhat.com> (raw)
In-Reply-To: <20140114200547.GM7572@laptop.programming.kicks-ass.net>



On Tue, 14 Jan 2014, Peter Zijlstra wrote:

> On Tue, Jan 14, 2014 at 02:06:57PM -0500, Mikulas Patocka wrote:
> > On Tue, 14 Jan 2014, Peter Zijlstra wrote:
> > > > Caused by commit 62b94a08da1b ("sched/preempt: Take away
> > > > preempt_enable_no_resched() from modules")
> 
> Read these two lines, then note that:
> 
> > Try adding #include <linux/preempt.h> to speedstep-lib.c. Does it help?
> 
> this obviously will not work as preempt_check_resched() and
> preempt_enable_no_resched() are no longer available to modules.

I see, you added commit 62b94a08da1bae9d187d49dfcd6665af393750f8 to 
linux-next, that broke my patch.

> > > I think that pm commit is a very good example of why the sched/preempt
> > > patch is a very good idea.
> > > 
> > > Also that Changelog fails to explain why enabling interrupts helps. What
> > > interrupt is required for progress, and how does it make the progress
> > > happen.
> > 
> > There is no explanation. It's hardware issue and I have no documentation 
> > for the hardware.
> 
> Rafael works for Intel, he ought to be able to figure out wtf the
> hardware does/needs.
> 
> > The general problem is that if there are bus-master transfers running (or 
> > possibly for other hardware reasons), the CPU refuses to change frequency. 
> > You can wait a little bit and retry and maybe you succeed changing the 
> > frequency next time.
> > 
> > If you enable interrupts, wait, disable interrupts and retry, you may 
> > succeed. If you keep interrupts disabled and retry, you never succeed, no 
> > matter how long do you wait. I found it experimentally, I don't know 
> > reason for that.
> 
> Sounds like magic goo..
> 
> In any case, try the below, it does the same but is far less horrid.
> 
> ---
>  drivers/cpufreq/speedstep-smi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/cpufreq/speedstep-smi.c b/drivers/cpufreq/speedstep-smi.c
> index 0f5326d6f79f..57d31538c248 100644
> --- a/drivers/cpufreq/speedstep-smi.c
> +++ b/drivers/cpufreq/speedstep-smi.c
> @@ -188,6 +188,7 @@ static void speedstep_set_state(unsigned int state)
>  		return;
>  
>  	/* Disable IRQs */
> +	preempt_disable();
>  	local_irq_save(flags);
>  
>  	command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
> @@ -200,7 +201,9 @@ static void speedstep_set_state(unsigned int state)
>  		if (retry) {
>  			pr_debug("retry %u, previous result %u, waiting...\n",
>  					retry, result);
> +			local_irq_restore(flags);

^^^ this is wrong, because the function speedstep_set_state may already be 
called with interrupts disabled from speedstep_get_freqs. So, you need to 
enable interrupts unconditionally, even if they were disabled at the 
beginning of the function speedstep_set_state.

I know it's dirty to enable interrupts in a function that was called with 
disabled interrupts, but here it must be so (you could rewrite 
speedstep_get_freqs to not disable interrupts if you want to avoid this 
dirtiness).

>  			mdelay(retry * 50);
> +			local_irq_save(flags);
>  		}
>  		retry++;
>  		__asm__ __volatile__(
> @@ -217,6 +220,7 @@ static void speedstep_set_state(unsigned int state)
>  
>  	/* enable IRQs */
>  	local_irq_restore(flags);
> +	preempt_enable();
>  
>  	if (new_state == state)
>  		pr_debug("change to %u MHz succeeded after %u tries "

You need also preempt_disable/enable in speedstep_get_freqs.


Here I'm resending the patch, to account for 
62b94a08da1bae9d187d49dfcd6665af393750f8.



From: Mikulas Patocka <mpatocka@redhat.com>

speedstep-smi: enable interrupts when waiting

On Dell Latitude C600 laptop with Pentium 3 850MHz processor, the
speedstep-smi driver sometimes loads and sometimes doesn't load with
"change to state X failed" message.

I found out that we need to enable interrupts while waiting. When we
enable interrupts, the blockage that prevents frequency transition
resolves and the transition is possible. With disabled interrupts, the
blockage doesn't resolve (no matter how long do we wait).

This patch enables interrupts in the function speedstep_set_state that can
be called with disabled interrupts. However, this function is called with
disabled interrupts only from speedstep_get_freqs, so it shouldn't cause
any problem.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/cpufreq/speedstep-lib.c |    3 +++
 drivers/cpufreq/speedstep-smi.c |   12 ++++++++++++
 2 files changed, 15 insertions(+)

Index: linux-next/drivers/cpufreq/speedstep-smi.c
===================================================================
--- linux-next.orig/drivers/cpufreq/speedstep-smi.c	2014-01-14 22:26:59.000000000 +0100
+++ linux-next/drivers/cpufreq/speedstep-smi.c	2014-01-14 22:35:11.000000000 +0100
@@ -156,6 +156,7 @@ static void speedstep_set_state(unsigned
 		return;
 
 	/* Disable IRQs */
+	preempt_disable();
 	local_irq_save(flags);
 
 	command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
@@ -166,9 +167,19 @@ static void speedstep_set_state(unsigned
 
 	do {
 		if (retry) {
+			/*
+			 * We need to enable interrupts, otherwise the blockage
+			 * won't resolve.
+			 *
+			 * We disable preemption so that other processes don't
+			 * run. If other processes were running, they could
+			 * submit more DMA requests, making the blockage worse.
+			 */
 			pr_debug("retry %u, previous result %u, waiting...\n",
 					retry, result);
+			local_irq_enable();
 			mdelay(retry * 50);
+			local_irq_disable();
 		}
 		retry++;
 		__asm__ __volatile__(
@@ -185,6 +196,7 @@ static void speedstep_set_state(unsigned
 
 	/* enable IRQs */
 	local_irq_restore(flags);
+	preempt_enable();
 
 	if (new_state == state)
 		pr_debug("change to %u MHz succeeded after %u tries "
Index: linux-next/drivers/cpufreq/speedstep-lib.c
===================================================================
--- linux-next.orig/drivers/cpufreq/speedstep-lib.c	2014-01-14 22:29:07.000000000 +0100
+++ linux-next/drivers/cpufreq/speedstep-lib.c	2014-01-14 22:31:04.000000000 +0100
@@ -400,6 +400,7 @@ unsigned int speedstep_get_freqs(enum sp
 
 	pr_debug("previous speed is %u\n", prev_speed);
 
+	preempt_disable();
 	local_irq_save(flags);
 
 	/* switch to low state */
@@ -464,6 +465,8 @@ unsigned int speedstep_get_freqs(enum sp
 
 out:
 	local_irq_restore(flags);
+	preempt_enable();
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(speedstep_get_freqs);

  reply index

Thread overview: 247+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-14  3:26 Stephen Rothwell
2014-01-14 14:14 ` Peter Zijlstra
2014-01-14 19:06   ` Mikulas Patocka
2014-01-14 20:05     ` Peter Zijlstra
2014-01-14 21:43       ` Mikulas Patocka [this message]
2014-01-14 22:03         ` Rafael J. Wysocki
2014-01-14 21:52           ` Mikulas Patocka
2014-01-14 22:18             ` Rafael J. Wysocki
2014-01-16 12:14         ` Peter Zijlstra
2014-01-16 19:33           ` [PATCH] speedstep: clean up interrupt disabling (was: linux-next: build failure after merge of the tip tree) Mikulas Patocka
2014-01-17  1:21             ` Rafael J. Wysocki
  -- strict thread matches above, loose matches on Subject: below --
2019-11-21  3:54 linux-next: build failure after merge of the tip tree Stephen Rothwell
2019-11-21  8:36 ` Peter Zijlstra
2019-11-21 18:37 ` Alex Deucher
2019-11-18  3:11 Stephen Rothwell
2019-11-18  9:49 ` Jiri Slaby
2019-11-18 12:49 ` Borislav Petkov
2019-10-21  2:13 Stephen Rothwell
2019-10-21  5:51 ` Ingo Molnar
2019-10-21  6:37   ` Stephen Rothwell
2019-10-10  2:14 Stephen Rothwell
2019-10-10  8:02 ` Ingo Molnar
2019-10-10 11:23   ` Stephen Rothwell
2019-10-10 13:44     ` Daniel Vetter
2019-11-06  2:53 ` Stephen Rothwell
2019-11-27 23:39   ` Stephen Rothwell
2019-08-29  6:20 Stephen Rothwell
2019-08-29  7:46 ` Peter Zijlstra
2019-08-29 12:24   ` Thomas Gleixner
2019-08-29 13:04     ` Peter Zijlstra
2019-08-08  4:48 Stephen Rothwell
2019-08-01  3:38 Stephen Rothwell
2019-07-09  6:54 Stephen Rothwell
2019-07-10  0:01 ` Stephen Rothwell
2019-07-10 18:21   ` Ilya Dryomov
2019-07-10 21:38     ` Stephen Rothwell
2019-07-02  5:33 Stephen Rothwell
2019-07-02 10:28 ` David Sterba
2019-07-02 12:57   ` Stephen Rothwell
2019-07-08  6:50   ` Stephen Rothwell
2019-06-25  6:04 Stephen Rothwell
2019-06-25  6:23 ` Kalle Valo
2019-06-25  6:26   ` Thomas Gleixner
2019-06-25  6:36   ` Stephen Rothwell
2019-06-25  6:51     ` Kalle Valo
2019-06-25  6:56     ` Thomas Gleixner
2019-06-25  7:47       ` Kalle Valo
2019-07-09  0:09 ` Stephen Rothwell
2019-07-09  3:46 ` Stephen Rothwell
2018-11-06  0:43 Stephen Rothwell
2018-08-08 23:24 Stephen Rothwell
2018-08-09  9:41 ` Joerg Roedel
2018-04-03  5:41 Stephen Rothwell
2018-04-03  9:30 ` Peter Zijlstra
2018-04-03 12:39 ` David Howells
2018-04-03 12:42   ` Peter Zijlstra
2018-04-03 12:41 ` David Howells
2017-11-08  2:47 Stephen Rothwell
2017-11-08  3:01 ` Josh Poimboeuf
2017-11-08  7:35   ` Stephen Rothwell
2017-11-08  9:18   ` Ingo Molnar
2017-11-08 12:14     ` Stephen Rothwell
2017-11-09  6:18       ` Ingo Molnar
2017-11-08 13:17     ` Josh Poimboeuf
2017-11-09  3:03     ` Stephen Rothwell
2017-11-01 10:51 Stephen Rothwell
2017-11-01 14:32 ` Kees Cook
2017-06-28  3:43 Stephen Rothwell
2017-06-28  4:00 ` jeffy
2017-06-28  4:24   ` Stephen Rothwell
2017-07-03  3:01 ` Stephen Rothwell
2017-05-02  3:17 Stephen Rothwell
2017-04-24  3:32 Stephen Rothwell
2017-04-24  5:31 ` Ingo Molnar
2017-04-05  3:36 Stephen Rothwell
2017-04-05  7:24 ` Mikko Perttunen
2016-12-07  1:37 Stephen Rothwell
2016-12-07  7:45 ` Ingo Molnar
2016-12-07  8:12   ` Jiri Olsa
2016-12-07 14:49     ` Arnaldo Carvalho de Melo
2016-12-07 18:56       ` Ingo Molnar
2016-12-07 20:32         ` Stephen Rothwell
2016-11-17  3:22 Stephen Rothwell
2016-09-29  3:20 Stephen Rothwell
2016-09-29 12:25 ` Rafael J. Wysocki
2016-09-29 15:54   ` Chen, Yu C
2016-09-29 20:50     ` Rafael J. Wysocki
2016-09-21  3:23 Stephen Rothwell
2016-09-21  6:44 ` Viresh Kumar
2016-07-18  8:29 Stephen Rothwell
2016-07-18 14:27 ` Paul Gortmaker
2016-07-18  5:15 Stephen Rothwell
2016-04-18  3:03 Stephen Rothwell
2016-04-14  2:14 Stephen Rothwell
2016-04-14 12:35 ` Arnaldo Carvalho de Melo
2016-04-14 12:55   ` Michael Ellerman
2016-04-14 15:08     ` Arnaldo Carvalho de Melo
2016-04-15 21:15 ` Arnaldo Carvalho de Melo
2016-04-15 21:28   ` Arnaldo Carvalho de Melo
2016-04-17 12:12     ` Jiri Olsa
2016-04-17 13:04       ` Jiri Olsa
2016-04-18 13:24         ` Arnaldo Carvalho de Melo
2016-04-18 13:28           ` Jiri Olsa
2016-03-01  1:29 Stephen Rothwell
2016-03-01  7:07 ` Ingo Molnar
2016-03-01  7:28   ` Sedat Dilek
2016-03-01  7:39     ` H. Peter Anvin
2016-03-01  8:41       ` Sedat Dilek
2016-03-01  9:45       ` Ingo Molnar
2016-03-01  9:40   ` Stephen Rothwell
2015-09-16  1:30 Stephen Rothwell
2015-09-16  4:53 ` David Miller
2015-09-16  0:12 Stephen Rothwell
2015-09-16  6:16 ` Jiri Olsa
2015-09-16  6:38   ` Jiri Olsa
2015-09-16  7:30     ` Stephen Rothwell
2015-09-16 14:17       ` Arnaldo Carvalho de Melo
2015-09-17  0:34   ` Stephen Rothwell
2015-09-30  2:56     ` Stephen Rothwell
2015-09-30  6:08       ` Jiri Olsa
2015-09-30  6:17         ` Stephen Rothwell
2015-07-28  5:33 Stephen Rothwell
2015-07-28 16:34 ` Luis R. Rodriguez
2015-07-28 18:17   ` Luis R. Rodriguez
2015-07-28  2:43 Stephen Rothwell
2015-07-28  8:41 ` Sudeep Holla
2015-07-28  9:43   ` Gregory CLEMENT
2015-07-15  1:01 Stephen Rothwell
2015-07-08  0:56 Stephen Rothwell
2015-07-08  9:40 ` Thomas Gleixner
2015-06-12 10:51 Michael Ellerman
     [not found] ` <BY1PR0701MB17063C25D627AF725A9E0D46FEBB0@BY1PR0701MB1706.namprd07.prod.outlook.com>
2015-06-12 21:22   ` David Miller
2015-06-12 21:25     ` Chickles, Derek
2015-06-09  7:15 Stephen Rothwell
2015-04-07  7:18 Stephen Rothwell
2015-04-07  8:48 ` Ingo Molnar
2015-04-07  8:56   ` Daniel Borkmann
2015-04-07  9:05     ` Stephen Rothwell
2015-04-07 11:13       ` Daniel Borkmann
2015-04-07 16:18         ` Alexei Starovoitov
2015-04-07 19:54           ` Daniel Borkmann
2015-04-08  5:03             ` Stephen Rothwell
2015-04-15  1:25               ` Stephen Rothwell
2015-04-07  8:53 ` Peter Zijlstra
2015-03-30  6:13 Stephen Rothwell
2015-03-30  7:15 ` Russell King - ARM Linux
2015-03-30  8:08   ` Stephen Rothwell
2015-03-30 14:57     ` Nathan Lynch
2015-03-30 15:08       ` Russell King - ARM Linux
2015-03-30 15:24         ` Nathan Lynch
2015-04-13 23:34         ` Stephen Rothwell
2014-07-25  4:45 Stephen Rothwell
2014-07-29 23:56 ` Stephen Rothwell
2014-07-30  3:10   ` John Stultz
2014-06-04  6:05 Stephen Rothwell
2014-06-04 10:49 ` Thomas Gleixner
2014-06-04 14:46   ` Linus Torvalds
2014-06-04 16:23   ` Olof Johansson
2014-06-04 20:02     ` Thomas Gleixner
2014-05-23  7:14 Stephen Rothwell
2014-05-29  1:38 ` Stephen Rothwell
2014-04-24  3:51 Stephen Rothwell
2014-02-12  2:41 Stephen Rothwell
2014-02-12  4:48 ` Preeti U Murthy
2014-02-11  3:00 Stephen Rothwell
2014-01-16  3:58 Stephen Rothwell
2014-01-16 12:19 ` Peter Zijlstra
2014-01-16 20:46   ` Stephen Rothwell
2014-01-16 22:25     ` Peter Zijlstra
2014-01-16 22:34       ` Stephen Rothwell
2014-01-16 22:51         ` H. Peter Anvin
2014-01-17  3:45           ` Stephen Rothwell
2014-01-18  9:46             ` Mike Galbraith
2014-01-18 12:44               ` Peter Zijlstra
2014-01-18 14:06                 ` Sabrina Dubroca
2014-01-18 15:21                 ` Mike Galbraith
2014-01-18 19:14                   ` H. Peter Anvin
2014-01-18 21:54                     ` Peter Zijlstra
2014-01-20  3:51               ` Stephen Rothwell
2014-01-20  8:42                 ` Sedat Dilek
2014-01-20  8:46                   ` Sedat Dilek
2014-01-20 13:10                     ` Stephen Rothwell
2014-01-20 15:28                       ` Sedat Dilek
2014-01-20  9:17                   ` Mike Galbraith
2014-01-20  9:25                     ` Sedat Dilek
2014-01-20  9:48                       ` Mike Galbraith
2014-01-20  4:45     ` Len Brown
2014-01-20  8:26       ` Peter Zijlstra
2014-01-20  8:56         ` H. Peter Anvin
2014-01-20  9:16           ` Peter Zijlstra
2014-01-20  9:23             ` H. Peter Anvin
2014-01-20  9:29               ` Ingo Molnar
2014-01-20 21:39                 ` Len Brown
2014-01-20 21:51                   ` Peter Zijlstra
2014-01-21  3:26                     ` Mike Galbraith
2014-01-21 10:47                     ` Ingo Molnar
2014-01-20  9:55               ` Peter Zijlstra
2014-01-20 11:00                 ` H. Peter Anvin
2014-01-20 11:05                   ` Peter Zijlstra
2014-01-16 20:56   ` H. Peter Anvin
2014-01-20  1:00 ` Len Brown
2014-01-20  1:07   ` H. Peter Anvin
2014-01-20  8:30   ` Peter Zijlstra
2014-01-20 10:13     ` Peter Zijlstra
2014-01-20 10:19       ` H. Peter Anvin
2014-01-20 11:06         ` Peter Zijlstra
2013-10-28  9:24 Stephen Rothwell
2013-10-28 10:18 ` Thierry Reding
2013-10-28 10:26   ` Ingo Molnar
2013-10-29  6:00     ` Stephen Rothwell
2013-04-29  5:45 Stephen Rothwell
2013-02-14  2:30 Stephen Rothwell
2013-02-21  2:04 ` Stephen Rothwell
2013-02-02  5:04 Stephen Rothwell
2012-08-16  3:12 Stephen Rothwell
2012-08-16 19:46 ` Paul E. McKenney
2012-08-17  0:56   ` Stephen Rothwell
2012-08-17 11:50     ` Paul E. McKenney
2012-08-20  4:54       ` Stephen Rothwell
2012-08-20 15:09         ` Paul E. McKenney
2012-03-08  4:21 Stephen Rothwell
2012-03-08 18:00 ` Greg KH
2012-03-20 22:00   ` Stephen Rothwell
2012-03-20 22:32     ` Greg KH
2012-03-21  9:53     ` Alan Cox
2012-03-21 12:19     ` Alan Cox
2012-03-21 12:29       ` Stephen Rothwell
2012-02-24  3:47 Stephen Rothwell
2012-02-24  3:34 Stephen Rothwell
2011-10-05  6:25 Stephen Rothwell
2011-10-05  8:46 ` Peter Zijlstra
2011-10-11  6:15   ` Stephen Rothwell
2011-06-27  4:22 Stephen Rothwell
2011-06-28 10:55 ` Stijn Devriendt
2011-04-01  2:00 Stephen Rothwell
2011-01-31  4:42 Stephen Rothwell
2011-02-17  3:47 ` Stephen Rothwell
2011-02-17 13:18   ` Michal Marek
2011-02-17 15:02     ` Michal Marek
2011-02-17 17:11       ` Ingo Molnar
2011-02-17 22:47       ` Stephen Rothwell
2011-02-18  3:54         ` Stephen Rothwell
2010-10-19  5:13 Stephen Rothwell
2010-07-29  3:04 Stephen Rothwell
2010-05-10  4:49 Stephen Rothwell
2010-05-10  5:09 ` H. Peter Anvin
2010-03-12  4:00 Stephen Rothwell

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LRH.2.02.1401141629520.19206@file01.intranet.prod.int.rdu2.redhat.com \
    --to=mpatocka@redhat.com \
    --cc=cpufreq@vger.kernel.org \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-next@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=sfr@canb.auug.org.au \
    --cc=tglx@linutronix.de \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Next Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-next/0 linux-next/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-next linux-next/ https://lore.kernel.org/linux-next \
		linux-next@vger.kernel.org
	public-inbox-index linux-next

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-next


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git