| Message ID | 20161212175654.ydc7rx3edqoacnp7@lostoracle.net |
|---|---|
| State | New |
| Headers | show |
On Mon, 2016-12-12 at 09:56 -0800, Nick Desaulniers wrote: > A quick cleanup that passes scripts/checkpatch.pl -f <file>. You might use the --strict option for acpi files. > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c [] > @@ -103,9 +103,8 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx) > > if (!mwait_supported[cstate_type]) { > mwait_supported[cstate_type] = 1; > - printk(KERN_DEBUG > - "Monitor-Mwait will be used to enter C-%d " > - "state\n", cx->type); > + pr_debug("Monitor-Mwait will be used to enter C-%d state\n", > + cx->type); It's generally better not to convert these printk(KERN_DEBUG uses. There are behavior differences between printk(KERN_DEBUG ...); and pr_debug(...); The first will always be emitted as long as the console level is appropriate. The second depends on a #define DEBUG before it gets emitted or a kernel with CONFIG_DYNAMIC_DEBUG enabled and this entry specifically enabled in the control file. -- 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 Mon 2016-12-12 10:39:15, Joe Perches wrote: > On Mon, 2016-12-12 at 09:56 -0800, Nick Desaulniers wrote: > > A quick cleanup that passes scripts/checkpatch.pl -f <file>. > > You might use the --strict option for acpi files. Please... don't encourage people more, we have enough cleanup patches as is. > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > [] > > @@ -103,9 +103,8 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx) > > > > if (!mwait_supported[cstate_type]) { > > mwait_supported[cstate_type] = 1; > > - printk(KERN_DEBUG > > - "Monitor-Mwait will be used to enter C-%d " > > - "state\n", cx->type); > > + pr_debug("Monitor-Mwait will be used to enter C-%d state\n", > > + cx->type); > > It's generally better not to convert > these printk(KERN_DEBUG uses. > > There are behavior differences between > printk(KERN_DEBUG ...); > and > pr_debug(...); > > The first will always be emitted as long > as the console level is appropriate. > > The second depends on a #define DEBUG > before it gets emitted or a kernel > with CONFIG_DYNAMIC_DEBUG enabled and > this entry specifically enabled in the > control file. Hmm. Perhaps pr_debug should be called pr_c_debug() or something? This is rather nice trap. Anyway with that fixed, Acked-by: Pavel Machek <pavel@ucw.cz> -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, 2016-12-12 at 23:22 +0100, Pavel Machek wrote: > On Mon 2016-12-12 10:39:15, Joe Perches wrote: > > On Mon, 2016-12-12 at 09:56 -0800, Nick Desaulniers wrote: > > > A quick cleanup that passes scripts/checkpatch.pl -f <file>. [] > > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c [] > > It's generally better not to convert > > these printk(KERN_DEBUG uses. > > > > There are behavior differences between > > printk(KERN_DEBUG ...); > > and > > pr_debug(...); > > > > The first will always be emitted as long > > as the console level is appropriate. > > > > The second depends on a #define DEBUG > > before it gets emitted or a kernel > > with CONFIG_DYNAMIC_DEBUG enabled and > > this entry specifically enabled in the > > control file. > > Hmm. Perhaps pr_debug should be called pr_c_debug() or something? This > is rather nice trap. Yeah, I've suggested veriants like pr_always_debug (from 2009) http://lkml.iu.edu/hypermail/linux/kernel/0910.0/00399.html -- 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 Mon 2016-12-12 14:32:12, Joe Perches wrote: > On Mon, 2016-12-12 at 23:22 +0100, Pavel Machek wrote: > > On Mon 2016-12-12 10:39:15, Joe Perches wrote: > > > On Mon, 2016-12-12 at 09:56 -0800, Nick Desaulniers wrote: > > > > A quick cleanup that passes scripts/checkpatch.pl -f <file>. > [] > > > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > [] > > > It's generally better not to convert > > > these printk(KERN_DEBUG uses. > > > > > > There are behavior differences between > > > printk(KERN_DEBUG ...); > > > and > > > pr_debug(...); > > > > > > The first will always be emitted as long > > > as the console level is appropriate. > > > > > > The second depends on a #define DEBUG > > > before it gets emitted or a kernel > > > with CONFIG_DYNAMIC_DEBUG enabled and > > > this entry specifically enabled in the > > > control file. > > > > Hmm. Perhaps pr_debug should be called pr_c_debug() or something? This > > is rather nice trap. > > Yeah, I've suggested veriants like pr_always_debug (from 2009) > http://lkml.iu.edu/hypermail/linux/kernel/0910.0/00399.html I'd very much like to see it other way around. pr_err is equivalent to printk(KERN_ERR) pr_warn is equivalent to printk(KERN_WARN) pr_debug _NOT_ beging equivalent to printk(KERN_DEBUG) is a trap :-(. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, 2016-12-13 at 00:08 +0100, Pavel Machek wrote: > On Mon 2016-12-12 14:32:12, Joe Perches wrote: > > On Mon, 2016-12-12 at 23:22 +0100, Pavel Machek wrote: > > > On Mon 2016-12-12 10:39:15, Joe Perches wrote: > > > > On Mon, 2016-12-12 at 09:56 -0800, Nick Desaulniers wrote: > > > > > A quick cleanup that passes scripts/checkpatch.pl -f <file>. > > > > [] > > > > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > > > > [] > > > > It's generally better not to convert > > > > these printk(KERN_DEBUG uses. > > > > > > > > There are behavior differences between > > > > printk(KERN_DEBUG ...); > > > > and > > > > pr_debug(...); > > > > > > > > The first will always be emitted as long > > > > as the console level is appropriate. > > > > > > > > The second depends on a #define DEBUG > > > > before it gets emitted or a kernel > > > > with CONFIG_DYNAMIC_DEBUG enabled and > > > > this entry specifically enabled in the > > > > control file. > > > > > > Hmm. Perhaps pr_debug should be called pr_c_debug() or something? This > > > is rather nice trap. > > > > Yeah, I've suggested veriants like pr_always_debug (from 2009) > > http://lkml.iu.edu/hypermail/linux/kernel/0910.0/00399.html > > I'd very much like to see it other way around. > > pr_err is equivalent to printk(KERN_ERR) > pr_warn is equivalent to printk(KERN_WARN) That bus left the station more than a decade ago. > pr_debug _NOT_ beging equivalent to printk(KERN_DEBUG) is a trap :-(. true -- 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
> Please... don't encourage people more, we have enough cleanup patches > as is. I recognize that this patch is relatively inconsequential, but it is my first patch to the Linux kernel, and is teaching me how to wrangle my email client and about the development work flow. I plan to write a blog post about my experience to help encourage more people to contribute. So I do really appreciate all of the feedback and patience for a relatively small patch. Thanks for your time. v3 incoming (will start a new thread). ~Nick -- 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 Mon, 2016-12-12 at 15:20 -0800, Nick Desaulniers wrote: > > Please... don't encourage people more, we have enough cleanup patches > > as is. > > I recognize that this patch is relatively inconsequential, but it is my > first patch to the Linux kernel, and is teaching me how to wrangle my > email client and about the development work flow. I plan to write a > blog post about my experience to help encourage more people to > contribute. So I do really appreciate all of the feedback and patience > for a relatively small patch. Thanks for your time. v3 incoming (will > start a new thread). Please create your first patch against something in drivers/staging/ Use the kernel newbies list and resources too https://kernelnewbies.org/FirstKernelPatch . -- 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
Joe Perches <joe@perches.com> writes: > On Mon, 2016-12-12 at 23:22 +0100, Pavel Machek wrote: >> On Mon 2016-12-12 10:39:15, Joe Perches wrote: >> > On Mon, 2016-12-12 at 09:56 -0800, Nick Desaulniers wrote: >> > > A quick cleanup that passes scripts/checkpatch.pl -f <file>. > [] >> > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > [] >> > It's generally better not to convert >> > these printk(KERN_DEBUG uses. >> > >> > There are behavior differences between >> > printk(KERN_DEBUG ...); >> > and >> > pr_debug(...); >> > >> > The first will always be emitted as long >> > as the console level is appropriate. >> > >> > The second depends on a #define DEBUG >> > before it gets emitted or a kernel >> > with CONFIG_DYNAMIC_DEBUG enabled and >> > this entry specifically enabled in the >> > control file. >> >> Hmm. Perhaps pr_debug should be called pr_c_debug() or something? This >> is rather nice trap. > > Yeah, I've suggested veriants like pr_always_debug (from 2009) > http://lkml.iu.edu/hypermail/linux/kernel/0910.0/00399.html The ability to strip the kernel from all debugging messages, or to keep them and dynamically enabling the interesting ones, are both important features *on top of* printk(KERN_DEBUG ...); If you add pr_c_debug() or whatever, then you'll only create a use case for another level of "strip this out". Back to square one. If this is a case of "my debug message is too important to let the user strip it from the kernel", then just use pr_info(). If not, then live with the additional debug level features leaving the control in the hands of the user. Personally, I want to be able to do dynamic debugging without having to manually filter out any unconditional debug messages. Please don't mess that up. There are more than enough levels for unconditional messages. we can afford to reserve KERN_DEBUG for the dynamic debug conditional ones. Thanks. Bjørn -- 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 Mon 2016-12-12 15:22:22, Joe Perches wrote: > On Mon, 2016-12-12 at 15:20 -0800, Nick Desaulniers wrote: > > > Please... don't encourage people more, we have enough cleanup patches > > > as is. > > > > I recognize that this patch is relatively inconsequential, but it is my > > first patch to the Linux kernel, and is teaching me how to wrangle my > > email client and about the development work flow. I plan to write a > > blog post about my experience to help encourage more people to > > contribute. So I do really appreciate all of the feedback and patience > > for a relatively small patch. Thanks for your time. v3 incoming (will > > start a new thread). > > Please create your first patch against something in drivers/staging/ > > Use the kernel newbies list and resources too > https://kernelnewbies.org/FirstKernelPatch > . Actually.. the ACPI cleanup is fine. You did well :-). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Dec 13, 2016 at 08:00:01PM +0100, Pavel Machek wrote: > Actually.. the ACPI cleanup is fine. You did well :-). > Pavel Cool, so (forgive the naive question) what happens next? Maybe I'm too used to the immediate gratification from Github of having a Pull Request get merged. Is this something that you have cherry picked into your tree, then will ask Linus to pull from at the end of the merge window? Do you have a tree that public that I am able to view? On git.kernel.org, there seems to be one tree with your name on it but it seems to be related to the (Nokia?) n900 and it's latest updated branch is from 9 weeks ago. Or is there more approvals I have to get to get the patch merged? Thanks, ~Nick -- 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 Fri, Dec 23, 2016 at 4:19 AM, Nick Desaulniers <nick.desaulniers@gmail.com> wrote: > On Tue, Dec 13, 2016 at 08:00:01PM +0100, Pavel Machek wrote: >> Actually.. the ACPI cleanup is fine. You did well :-). >> Pavel > > Cool, so (forgive the naive question) what happens next? Maybe I'm too > used to the immediate gratification from Github of having a Pull Request > get merged. Is this something that you have cherry picked into your > tree, then will ask Linus to pull from at the end of the merge window? > > Do you have a tree that public that I am able to view? On > git.kernel.org, there seems to be one tree with your name on it but it > seems to be related to the (Nokia?) n900 and it's latest updated branch > is from 9 weeks ago. > > Or is there more approvals I have to get to get the patch merged? Somebody has to decide that your patch is worth merging and take it. For ACPI-related patches that usually is me, but I haven't looked at it yet. I'll do that next week and let you know if all goes well. Thanks, Rafael -- 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 2016-12-22 19:19:43, Nick Desaulniers wrote: > On Tue, Dec 13, 2016 at 08:00:01PM +0100, Pavel Machek wrote: > > Actually.. the ACPI cleanup is fine. You did well :-). > > Pavel > > Cool, so (forgive the naive question) what happens next? Maybe I'm too > used to the immediate gratification from Github of having a Pull Request > get merged. Is this something that you have cherry picked into your > tree, then will ask Linus to pull from at the end of the merge window? > Do you have a tree that public that I am able to view? On > git.kernel.org, there seems to be one tree with your name on it but it > seems to be related to the (Nokia?) n900 and it's latest updated branch > is from 9 weeks ago. I don't maintain a tree for power management stuff. Rafael does. There are tricks that can be done for easy but not trivial fixes. Ouch and if you want code that needs clean ups / fixes... just let me know :-). > Or is there more approvals I have to get to get the patch merged? No, you did well, just wait. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
From 47d3bcb76ef89ddbe74c8d8aacee1c0c6203a766 Mon Sep 17 00:00:00 2001 From: Nick Desaulniers <nick.desaulniers@gmail.com> Date: Mon, 12 Dec 2016 09:50:23 -0800 Subject: [PATCH v2] ACPI: small formatting fixes A quick cleanup that passes scripts/checkpatch.pl -f <file>. Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com> --- arch/x86/kernel/acpi/cstate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c index af15f44..37d40a3 100644 --- a/arch/x86/kernel/acpi/cstate.c +++ b/arch/x86/kernel/acpi/cstate.c @@ -12,7 +12,6 @@ #include <linux/sched.h> #include <acpi/processor.h> -#include <asm/acpi.h> #include <asm/mwait.h> #include <asm/special_insns.h> @@ -89,7 +88,8 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx) retval = 0; /* If the HW does not support any sub-states in this C-state */ if (num_cstate_subtype == 0) { - pr_warn(FW_BUG "ACPI MWAIT C-state 0x%x not supported by HW (0x%x)\n", cx->address, edx_part); + pr_warn(FW_BUG "ACPI MWAIT C-state 0x%x not supported by HW (0x%x)\n", + cx->address, edx_part); retval = -1; goto out; } @@ -103,9 +103,8 @@ static long acpi_processor_ffh_cstate_probe_cpu(void *_cx) if (!mwait_supported[cstate_type]) { mwait_supported[cstate_type] = 1; - printk(KERN_DEBUG - "Monitor-Mwait will be used to enter C-%d " - "state\n", cx->type); + pr_debug("Monitor-Mwait will be used to enter C-%d state\n", + cx->type); } snprintf(cx->desc, ACPI_CX_DESC_LEN, "ACPI FFH INTEL MWAIT 0x%x", @@ -166,6 +165,7 @@ EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter); static int __init ffh_cstate_init(void) { struct cpuinfo_x86 *c = &boot_cpu_data; + if (c->x86_vendor != X86_VENDOR_INTEL) return -1; -- 2.9.3 -- 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