Re: linux-next: build failure after merge of the tip tree

Re: linux-next: build failure after merge of the tip tree

[Mikulas Patocka]

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);

Re: linux-next: build failure after merge of the tip tree

[Rafael J. Wysocki]

On Tuesday, January 14, 2014 04:43:43 PM Mikulas Patocka wrote:
> 
> 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.

Do I think correctly that this should work regardless of whether or not
62b94a08da1bae9d187d49dfcd6665af393750f8 is applied?

> 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);
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: linux-next: build failure after merge of the tip tree

[Mikulas Patocka]

On Tue, 14 Jan 2014, Rafael J. Wysocki wrote:

> On Tuesday, January 14, 2014 04:43:43 PM Mikulas Patocka wrote:
> > 
> > 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.
> 
> Do I think correctly that this should work regardless of whether or not
> 62b94a08da1bae9d187d49dfcd6665af393750f8 is applied?

Yes.

Mikulas

Re: linux-next: build failure after merge of the tip tree

[Rafael J. Wysocki]

On Tuesday, January 14, 2014 04:52:20 PM Mikulas Patocka wrote:
> 
> On Tue, 14 Jan 2014, Rafael J. Wysocki wrote:
> 
> > On Tuesday, January 14, 2014 04:43:43 PM Mikulas Patocka wrote:
> > > 
> > > 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.
> > 
> > Do I think correctly that this should work regardless of whether or not
> > 62b94a08da1bae9d187d49dfcd6665af393750f8 is applied?
> 
> Yes.

OK

I'll replace your original patch with this version, then.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: linux-next: build failure after merge of the tip tree

[Peter Zijlstra]

On Tue, Jan 14, 2014 at 04:43:43PM -0500, Mikulas Patocka wrote:
> > @@ -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).

Egads; I think you had better, this is vile beyond reason.

> >  			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.

Argh I see, this is really horrid.


Anyway, its Rafael's call, its his subsystem he gets to fix it when it
explodes.

/me shudders

[PATCH] speedstep: clean up interrupt disabling (was: linux-next: build failure after merge of the tip tree)

[Mikulas Patocka]

On Thu, 16 Jan 2014, Peter Zijlstra wrote:

> > >  		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.
> 
> Argh I see, this is really horrid.
> 
> 
> Anyway, its Rafael's call, its his subsystem he gets to fix it when it
> explodes.
> 
> /me shudders

speedstep_get_freqs disables the interrupts to measure the transition 
latency. It is called from speedstep-ich.c (that requires the latency) and 
from speedstep-smi.c (that passes NULL as a pointer to latency, because it 
doesn't need it).

So, you could disable interrupts in speedstep_get_freqs only when the 
transition_latency pointer is non-NULL.

I think speedstep_set_state doesn't need to disable interrupts at all - 
all that it does is one "out" instruction, you don't need to synchronize 
that "out" instruction with anything, so there is no need to disable 
interrupts. Or - does some specification say that interrupts must be 
disabled there?

I am sending this patch to clean up the mess to be applied on the top of 
my previous patch.

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>
Subject: speedstep: clean up interrupt disabling

This patch cleans up interrupt disabling in speedstep-smi and
speedstep-lib.

speedstep_get_freqs in speedstep-lib may be called from speedstep-smi or
speedstep-ich. When it is called from speedstep-ich, it needs to calculate
transition latency. When it is called from speedstep-smi, transition
latency doesn't have to be calculated.

The function speedstep_set_state in speedstep-smi needs to enable
interrupts. Previously it enabled interrupts even if it was called with
disabled interrupts, but it is dirty.

This patch changes speedstep_get_freqs so that it disables interrupts only
when it is called from speedstep-ich and when it is measuring the
transition latency. This avoids much of the code dirtiness.

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

---
 drivers/cpufreq/speedstep-lib.c |   13 ++++++-------
 drivers/cpufreq/speedstep-smi.c |   11 ++++-------
 2 files changed, 10 insertions(+), 14 deletions(-)

Index: linux-3.4.75/drivers/cpufreq/speedstep-lib.c
===================================================================
--- linux-3.4.75.orig/drivers/cpufreq/speedstep-lib.c	2014-01-16 18:51:16.000000000 +0100
+++ linux-3.4.75/drivers/cpufreq/speedstep-lib.c	2014-01-16 18:51:22.000000000 +0100
@@ -400,9 +400,6 @@ 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 */
 	set_state(SPEEDSTEP_LOW);
 	*low_speed = speedstep_get_frequency(processor);
@@ -414,15 +411,19 @@ unsigned int speedstep_get_freqs(enum sp
 	pr_debug("low speed is %u\n", *low_speed);
 
 	/* start latency measurement */
-	if (transition_latency)
+	if (transition_latency) {
+		local_irq_save(flags);
 		do_gettimeofday(&tv1);
+	}
 
 	/* switch to high state */
 	set_state(SPEEDSTEP_HIGH);
 
 	/* end latency measurement */
-	if (transition_latency)
+	if (transition_latency) {
 		do_gettimeofday(&tv2);
+		local_irq_restore(flags);
+	}
 
 	*high_speed = speedstep_get_frequency(processor);
 	if (!*high_speed) {
@@ -464,8 +465,6 @@ unsigned int speedstep_get_freqs(enum sp
 	}
 
 out:
-	local_irq_restore(flags);
-	preempt_enable();
 
 	return ret;
 }
Index: linux-3.4.75/drivers/cpufreq/speedstep-smi.c
===================================================================
--- linux-3.4.75.orig/drivers/cpufreq/speedstep-smi.c	2014-01-16 18:51:16.000000000 +0100
+++ linux-3.4.75/drivers/cpufreq/speedstep-smi.c	2014-01-16 18:51:22.000000000 +0100
@@ -180,16 +180,16 @@ static int speedstep_get_state(void)
 static void speedstep_set_state(unsigned int state)
 {
 	unsigned int result = 0, command, new_state, dummy;
-	unsigned long flags;
 	unsigned int function = SET_SPEEDSTEP_STATE;
 	unsigned int retry = 0;
 
 	if (state > 0x1)
 		return;
 
-	/* Disable IRQs */
+	WARN_ON_ONCE(irqs_disabled());
+
+	/* Disable preemption so that other processes don't run */
 	preempt_disable();
-	local_irq_save(flags);
 
 	command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
 
@@ -209,9 +209,7 @@ static void speedstep_set_state(unsigned
 			 */
 			pr_debug("retry %u, previous result %u, waiting...\n",
 					retry, result);
-			local_irq_enable();
 			mdelay(retry * 50);
-			local_irq_disable();
 		}
 		retry++;
 		__asm__ __volatile__(
@@ -226,8 +224,7 @@ static void speedstep_set_state(unsigned
 			);
 	} while ((new_state != state) && (retry <= SMI_TRIES));
 
-	/* enable IRQs */
-	local_irq_restore(flags);
+	/* enable preemption */
 	preempt_enable();
 
 	if (new_state == state)

Re: [PATCH] speedstep: clean up interrupt disabling (was: linux-next: build failure after merge of the tip tree)

[Rafael J. Wysocki]

On Thursday, January 16, 2014 02:33:02 PM Mikulas Patocka wrote:
> 
> On Thu, 16 Jan 2014, Peter Zijlstra wrote:
> 
> > > >  		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.
> > 
> > Argh I see, this is really horrid.
> > 
> > 
> > Anyway, its Rafael's call, its his subsystem he gets to fix it when it
> > explodes.
> > 
> > /me shudders
> 
> speedstep_get_freqs disables the interrupts to measure the transition 
> latency. It is called from speedstep-ich.c (that requires the latency) and 
> from speedstep-smi.c (that passes NULL as a pointer to latency, because it 
> doesn't need it).
> 
> So, you could disable interrupts in speedstep_get_freqs only when the 
> transition_latency pointer is non-NULL.
> 
> I think speedstep_set_state doesn't need to disable interrupts at all - 
> all that it does is one "out" instruction, you don't need to synchronize 
> that "out" instruction with anything, so there is no need to disable 
> interrupts. Or - does some specification say that interrupts must be 
> disabled there?
> 
> I am sending this patch to clean up the mess to be applied on the top of 
> my previous patch.

Well, I really don't appreciate sending me stuff like this two days before a
merge window.  So I've dropped the speedstep_smi patch and please send me
something I can queue up for 3.15 without making Peter shudder.

Thanks!


> From: Mikulas Patocka <mpatocka@redhat.com>
> Subject: speedstep: clean up interrupt disabling
> 
> This patch cleans up interrupt disabling in speedstep-smi and
> speedstep-lib.
> 
> speedstep_get_freqs in speedstep-lib may be called from speedstep-smi or
> speedstep-ich. When it is called from speedstep-ich, it needs to calculate
> transition latency. When it is called from speedstep-smi, transition
> latency doesn't have to be calculated.
> 
> The function speedstep_set_state in speedstep-smi needs to enable
> interrupts. Previously it enabled interrupts even if it was called with
> disabled interrupts, but it is dirty.
> 
> This patch changes speedstep_get_freqs so that it disables interrupts only
> when it is called from speedstep-ich and when it is measuring the
> transition latency. This avoids much of the code dirtiness.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/cpufreq/speedstep-lib.c |   13 ++++++-------
>  drivers/cpufreq/speedstep-smi.c |   11 ++++-------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> Index: linux-3.4.75/drivers/cpufreq/speedstep-lib.c
> ===================================================================
> --- linux-3.4.75.orig/drivers/cpufreq/speedstep-lib.c	2014-01-16 18:51:16.000000000 +0100
> +++ linux-3.4.75/drivers/cpufreq/speedstep-lib.c	2014-01-16 18:51:22.000000000 +0100
> @@ -400,9 +400,6 @@ 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 */
>  	set_state(SPEEDSTEP_LOW);
>  	*low_speed = speedstep_get_frequency(processor);
> @@ -414,15 +411,19 @@ unsigned int speedstep_get_freqs(enum sp
>  	pr_debug("low speed is %u\n", *low_speed);
>  
>  	/* start latency measurement */
> -	if (transition_latency)
> +	if (transition_latency) {
> +		local_irq_save(flags);
>  		do_gettimeofday(&tv1);
> +	}
>  
>  	/* switch to high state */
>  	set_state(SPEEDSTEP_HIGH);
>  
>  	/* end latency measurement */
> -	if (transition_latency)
> +	if (transition_latency) {
>  		do_gettimeofday(&tv2);
> +		local_irq_restore(flags);
> +	}
>  
>  	*high_speed = speedstep_get_frequency(processor);
>  	if (!*high_speed) {
> @@ -464,8 +465,6 @@ unsigned int speedstep_get_freqs(enum sp
>  	}
>  
>  out:
> -	local_irq_restore(flags);
> -	preempt_enable();
>  
>  	return ret;
>  }
> Index: linux-3.4.75/drivers/cpufreq/speedstep-smi.c
> ===================================================================
> --- linux-3.4.75.orig/drivers/cpufreq/speedstep-smi.c	2014-01-16 18:51:16.000000000 +0100
> +++ linux-3.4.75/drivers/cpufreq/speedstep-smi.c	2014-01-16 18:51:22.000000000 +0100
> @@ -180,16 +180,16 @@ static int speedstep_get_state(void)
>  static void speedstep_set_state(unsigned int state)
>  {
>  	unsigned int result = 0, command, new_state, dummy;
> -	unsigned long flags;
>  	unsigned int function = SET_SPEEDSTEP_STATE;
>  	unsigned int retry = 0;
>  
>  	if (state > 0x1)
>  		return;
>  
> -	/* Disable IRQs */
> +	WARN_ON_ONCE(irqs_disabled());
> +
> +	/* Disable preemption so that other processes don't run */
>  	preempt_disable();
> -	local_irq_save(flags);
>  
>  	command = (smi_sig & 0xffffff00) | (smi_cmd & 0xff);
>  
> @@ -209,9 +209,7 @@ static void speedstep_set_state(unsigned
>  			 */
>  			pr_debug("retry %u, previous result %u, waiting...\n",
>  					retry, result);
> -			local_irq_enable();
>  			mdelay(retry * 50);
> -			local_irq_disable();
>  		}
>  		retry++;
>  		__asm__ __volatile__(
> @@ -226,8 +224,7 @@ static void speedstep_set_state(unsigned
>  			);
>  	} while ((new_state != state) && (retry <= SMI_TRIES));
>  
> -	/* enable IRQs */
> -	local_irq_restore(flags);
> +	/* enable preemption */
>  	preempt_enable();
>  
>  	if (new_state == state)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.