A quick cleanup that passes scripts/checkpatch.pl -f <file>.
Signed-off-by: Nick Desaulniers <[email protected]>
---
arch/x86/kernel/acpi/cstate.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index af15f44..ed52aec 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -1,7 +1,7 @@
/*
* Copyright (C) 2005 Intel Corporation
- * Venkatesh Pallipadi <[email protected]>
- * - Added _PDC for SMP C-states on Intel CPUs
+ * Venkatesh Pallipadi <[email protected]>
+ * - Added _PDC for SMP C-states on Intel CPUs
*/
#include <linux/kernel.h>
@@ -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>
@@ -50,8 +49,8 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
* P4, Core and beyond CPUs
*/
if (c->x86_vendor == X86_VENDOR_INTEL &&
- (c->x86 > 0xf || (c->x86 == 6 && c->x86_model >= 0x0f)))
- flags->bm_control = 0;
+ (c->x86 > 0xf || (c->x86 == 6 && c->x86_model >= 0x0f)))
+ flags->bm_control = 0;
}
EXPORT_SYMBOL(acpi_processor_power_init_bm_check);
@@ -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",
@@ -159,13 +158,14 @@ void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
mwait_idle_with_hints(percpu_entry->states[cx->index].eax,
- percpu_entry->states[cx->index].ecx);
+ percpu_entry->states[cx->index].ecx);
}
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
Hi!
> A quick cleanup that passes scripts/checkpatch.pl -f <file>.
>
> Signed-off-by: Nick Desaulniers <[email protected]>
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -1,7 +1,7 @@
> /*
> * Copyright (C) 2005 Intel Corporation
> - * Venkatesh Pallipadi <[email protected]>
> - * - Added _PDC for SMP C-states on Intel CPUs
> + * Venkatesh Pallipadi <[email protected]>
> + * - Added _PDC for SMP C-states on Intel CPUs
> */
>
> #include <linux/kernel.h>
No.
> @@ -50,8 +49,8 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
> * P4, Core and beyond CPUs
> */
> if (c->x86_vendor == X86_VENDOR_INTEL &&
> - (c->x86 > 0xf || (c->x86 == 6 && c->x86_model >= 0x0f)))
> - flags->bm_control = 0;
> + (c->x86 > 0xf || (c->x86 == 6 && c->x86_model >= 0x0f)))
> + flags->bm_control = 0;
> }
> EXPORT_SYMBOL(acpi_processor_power_init_bm_check);
>
No.
> @@ -159,13 +158,14 @@ void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
>
> percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> mwait_idle_with_hints(percpu_entry->states[cx->index].eax,
> - percpu_entry->states[cx->index].ecx);
> + percpu_entry->states[cx->index].ecx);
> }
No.
Rest is good.
Pavel
--
(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 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.
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 <[email protected]>
--
(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
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
> 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
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
.
Joe Perches <[email protected]> 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
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
On Fri, Dec 23, 2016 at 4:19 AM, Nick Desaulniers
<[email protected]> 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