| Message ID | alpine.LRH.2.02.1405141012040.11670@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 14 May 2014 19:54, Mikulas Patocka <mpatocka@redhat.com> wrote: > Hi Viresh > > Some times ago, I sent these two cpufreq patches to fix frequency > transition problem on speedstep-smi, but you didn't put them to the > upstream kernel. I am resending the patches. If you would like a different > solution to this problem, please explain it. Probably these are just missed by Rafael. The patches are fine but the above paragraph you wrote would get into kernel logs and must have been placed below the three dash lines: "---" > 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. > > The hardware sometimes refuses to change frequency and in this case, we > need to retry later. I found out that we need to enable interrupts while > waiting for retry. When we enable interrupts, the hardware 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). The exact reasons for this hardware behavior are unknown. > > 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 > Cc: stable@vger.kernel.org > > --- Here. And this will be skipped by tools (git, etc) while applying the patch. > drivers/cpufreq/speedstep-lib.c | 3 +++ > drivers/cpufreq/speedstep-smi.c | 12 ++++++++++++ > 2 files changed, 15 insertions(+) For both the patches: Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 May 2014, Viresh Kumar wrote: > On 14 May 2014 19:54, Mikulas Patocka <mpatocka@redhat.com> wrote: > > Hi Viresh > > > > Some times ago, I sent these two cpufreq patches to fix frequency > > transition problem on speedstep-smi, but you didn't put them to the > > upstream kernel. I am resending the patches. If you would like a different > > solution to this problem, please explain it. > > Probably these are just missed by Rafael. The patches are fine but the > above paragraph you wrote would get into kernel logs and must have > been placed below the three dash lines: "---" I used "---" in the past, but some people complained that their script picks the text above "---" and ignores the text below "---" - thus, it does exactly the opposite of what it should do. I don't know what kind of delimiter should I use. Suppose that I want to send something like this: Hi, here I'm sending some cool stuff. *** The message that goes to git. 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-3.14-rc1/drivers/cpufreq/speedstep-smi.c =================================================================== --- linux-3.14-rc1.orig/drivers/cpufreq/speedstep-smi.c 2014-02-03 18:56:44.000000000 +0100 +++ linux-3.14-rc1/drivers/cpufreq/speedstep-smi.c 2014-02-03 19:06:21.000000000 +0100 ... the patch. Now - I'm asking - what kind of barrier should I use between "Hi, here I'm sending some cool stuff" and "The message that goes to git"? I don't really know. The file Documentation/SubmittingPatches doesn't explain it, it says that the message for git should go at the top of the email. If I want the person who receives the patch to read "Hi, here I'm sending some cool stuff" first, I need to put it at the beginning of the email. Maybe we should define a new barrier for this purpose, put it to Documentation/SubmittingPatches and update the scripts? Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 May 2014 16:07, Mikulas Patocka <mpatocka@redhat.com> wrote: > On Thu, 15 May 2014, Viresh Kumar wrote: >> On 14 May 2014 19:54, Mikulas Patocka <mpatocka@redhat.com> wrote: >> > Hi Viresh >> > >> > Some times ago, I sent these two cpufreq patches to fix frequency >> > transition problem on speedstep-smi, but you didn't put them to the >> > upstream kernel. I am resending the patches. If you would like a different >> > solution to this problem, please explain it. >> >> Probably these are just missed by Rafael. The patches are fine but the >> above paragraph you wrote would get into kernel logs and must have >> been placed below the three dash lines: "---" > > I used "---" in the past, but some people complained that their script > picks the text above "---" and ignores the text below "---" - thus, it > does exactly the opposite of what it should do. Have you added an extra line with --- ? Probably yes. I haven't asked you to add another line, but move the part you don't want to be commited below --- already present in patch.. Like: > --- Blurb here :) > drivers/cpufreq/speedstep-lib.c | 3 +++ > drivers/cpufreq/speedstep-smi.c | 12 ++++++++++++ > 2 files changed, 15 insertions(+) > > I don't know what kind of delimiter should I use. Suppose that I want to > send something like this: Will go through below mail of yours once we are clear on above.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 15 May 2014, Viresh Kumar wrote: > On 15 May 2014 16:07, Mikulas Patocka <mpatocka@redhat.com> wrote: > > On Thu, 15 May 2014, Viresh Kumar wrote: > >> On 14 May 2014 19:54, Mikulas Patocka <mpatocka@redhat.com> wrote: > >> > Hi Viresh > >> > > >> > Some times ago, I sent these two cpufreq patches to fix frequency > >> > transition problem on speedstep-smi, but you didn't put them to the > >> > upstream kernel. I am resending the patches. If you would like a different > >> > solution to this problem, please explain it. > >> > >> Probably these are just missed by Rafael. The patches are fine but the > >> above paragraph you wrote would get into kernel logs and must have > >> been placed below the three dash lines: "---" > > > > I used "---" in the past, but some people complained that their script > > picks the text above "---" and ignores the text below "---" - thus, it > > does exactly the opposite of what it should do. > > Have you added an extra line with --- ? Probably yes. > I haven't asked you to add another line, but move the part you don't want > to be commited below --- already present in patch.. > > Like: > > > --- > > Blurb here :) Sure - but if I want you to read "Blurb here :)" before reading the git message, I need to place it at the top of the email, before the git message. > > drivers/cpufreq/speedstep-lib.c | 3 +++ > > drivers/cpufreq/speedstep-smi.c | 12 ++++++++++++ > > 2 files changed, 15 insertions(+) > > > > > > I don't know what kind of delimiter should I use. Suppose that I want to > > send something like this: > > Will go through below mail of yours once we are clear on above.. Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 May 2014 18:15, Mikulas Patocka <mpatocka@redhat.com> wrote: >> > --- >> >> Blurb here :) > > Sure - but if I want you to read "Blurb here :)" before reading the git > message, I need to place it at the top of the email, before the git > message. You can't, git will pick it up :) And people are trained enough to read stuff at this place as it was present over the top of mail :) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/2014 07:00 AM, Viresh Kumar wrote: > On 15 May 2014 18:15, Mikulas Patocka <mpatocka@redhat.com> wrote: >>>> --- >>> >>> Blurb here :) >> >> Sure - but if I want you to read "Blurb here :)" before reading the git >> message, I need to place it at the top of the email, before the git >> message. > > You can't, git will pick it up :) Right. The patch description goes at the top of the email. If you want to include text that should not be part of the patch description, you can put it after the --- or you can put it into an introductory email (like [PATCH 0/5]). > And people are trained enough to read stuff at this place as it was > present over the top of mail :)
Index: linux-3.14-rc1/drivers/cpufreq/speedstep-smi.c =================================================================== --- linux-3.14-rc1.orig/drivers/cpufreq/speedstep-smi.c 2014-02-03 18:56:44.000000000 +0100 +++ linux-3.14-rc1/drivers/cpufreq/speedstep-smi.c 2014-02-03 19:06:21.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-3.14-rc1/drivers/cpufreq/speedstep-lib.c =================================================================== --- linux-3.14-rc1.orig/drivers/cpufreq/speedstep-lib.c 2014-02-03 18:54:39.000000000 +0100 +++ linux-3.14-rc1/drivers/cpufreq/speedstep-lib.c 2014-02-03 19:06:21.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);
Hi Viresh Some times ago, I sent these two cpufreq patches to fix frequency transition problem on speedstep-smi, but you didn't put them to the upstream kernel. I am resending the patches. If you would like a different solution to this problem, please explain it. Mikulas 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. The hardware sometimes refuses to change frequency and in this case, we need to retry later. I found out that we need to enable interrupts while waiting for retry. When we enable interrupts, the hardware 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). The exact reasons for this hardware behavior are unknown. 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 Cc: stable@vger.kernel.org --- drivers/cpufreq/speedstep-lib.c | 3 +++ drivers/cpufreq/speedstep-smi.c | 12 ++++++++++++ 2 files changed, 15 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html