Patchwork checkpatch: prefer dev_<level>( to dev_printk(KERN_<LEVEL>

login
register
mail settings
Submitter Joe Perches
Date Jan. 3, 2013, 12:06 a.m.
Message ID <1357171594.25181.19.camel@joe-AO722>
Download mbox | patch
Permalink /patch/209164/
State Not Applicable
Headers show

Comments

Joe Perches - Jan. 3, 2013, 12:06 a.m.
Add YA check to printk style.

dev_<level> uses are functions and generate smaller
object code than dev_printk(KERN_<LEVEL>.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)



--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Jan. 3, 2013, 12:34 a.m.
On Wed, Jan 2, 2013 at 6:06 PM, Joe Perches <joe@perches.com> wrote:
> Add YA check to printk style.
>
> dev_<level> uses are functions and generate smaller
> object code than dev_printk(KERN_<LEVEL>.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  scripts/checkpatch.pl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4d2c7df..f50b32d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2430,6 +2430,16 @@ sub process {
>                              "Prefer pr_warn(... to pr_warning(...\n" . $herecurr);
>                 }
>
> +               if ($line =~ /\bdev_printk\s*\(\s*KERN_([A-Z]+)/) {
> +                       my $orig = $1;
> +                       my $level = lc($orig);
> +                       $level = "warn" if ($level eq "warning");
> +                       my $level2 = $level;
> +                       $level2 = "dbg" if ($level eq "debug");
> +                       WARN("PREFER_DEV_LEVEL",
> +                            "Prefer dev_$level2(... to dev_printk(KERN_$orig, ...\n" . $herecurr);

This suggests dev_dbg() instead of dev_printk(KERN_DEBUG), doesn't it?
 Those aren't equivalent (dev_printk() always does the printk, but in
many cases dev_dbg() does not, depending on CONFIG_DEBUG,
CONFIG_DYNAMIC_DEBUG, etc.)

> +               }
> +
>  # function brace can't be on same line, except for #defines of do while,
>  # or if closed on same line
>                 if (($line=~/$Type\s*$Ident\(.*\).*\s{/) and
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 3, 2013, 12:39 a.m.
On Wed, 2013-01-02 at 18:34 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 2, 2013 at 6:06 PM, Joe Perches <joe@perches.com> wrote:
> > Add YA check to printk style.
> >
> > dev_<level> uses are functions and generate smaller
> > object code than dev_printk(KERN_<LEVEL>.
[]
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -2430,6 +2430,16 @@ sub process {
> >                              "Prefer pr_warn(... to pr_warning(...\n" . $herecurr);
> >                 }
> >
> > +               if ($line =~ /\bdev_printk\s*\(\s*KERN_([A-Z]+)/) {
> > +                       my $orig = $1;
> > +                       my $level = lc($orig);
> > +                       $level = "warn" if ($level eq "warning");
> > +                       my $level2 = $level;
> > +                       $level2 = "dbg" if ($level eq "debug");
> > +                       WARN("PREFER_DEV_LEVEL",
> > +                            "Prefer dev_$level2(... to dev_printk(KERN_$orig, ...\n" . $herecurr);
> 
> This suggests dev_dbg() instead of dev_printk(KERN_DEBUG), doesn't it?
>  Those aren't equivalent (dev_printk() always does the printk, but in
> many cases dev_dbg() does not, depending on CONFIG_DEBUG,
> CONFIG_DYNAMIC_DEBUG, etc.)

True, I think the suggested change is appropriate though.

If people really want debug output in a specific file,
then they should #define DEBUG and/or enable dynamic debug.

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Whitcroft - Jan. 3, 2013, 10:08 a.m.
On Wed, Jan 02, 2013 at 04:06:34PM -0800, Joe Perches wrote:
> Add YA check to printk style.
> 
> dev_<level> uses are functions and generate smaller
> object code than dev_printk(KERN_<LEVEL>.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  scripts/checkpatch.pl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4d2c7df..f50b32d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2430,6 +2430,16 @@ sub process {
>  			     "Prefer pr_warn(... to pr_warning(...\n" . $herecurr);
>  		}
>  
> +		if ($line =~ /\bdev_printk\s*\(\s*KERN_([A-Z]+)/) {
> +			my $orig = $1;
> +			my $level = lc($orig);
> +			$level = "warn" if ($level eq "warning");
> +			my $level2 = $level;
> +			$level2 = "dbg" if ($level eq "debug");

Is there some sublty I am not seeing here such that level2 is necessary?
As far as I can see the two above could be the below to the same effect?

			$level2= "dbg" if ($level eq "debug");

(With the obvious change to the print of course)

-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 3, 2013, 4:28 p.m.
On Thu, 2013-01-03 at 10:08 +0000, Andy Whitcroft wrote:
> On Wed, Jan 02, 2013 at 04:06:34PM -0800, Joe Perches wrote:
> > Add YA check to printk style.
> > 
> > dev_<level> uses are functions and generate smaller
> > object code than dev_printk(KERN_<LEVEL>.
[]
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > +		if ($line =~ /\bdev_printk\s*\(\s*KERN_([A-Z]+)/) {
> > +			my $orig = $1;
> > +			my $level = lc($orig);
> > +			$level = "warn" if ($level eq "warning");
> > +			my $level2 = $level;
> > +			$level2 = "dbg" if ($level eq "debug");
> 
> Is there some sublty I am not seeing here such that level2 is necessary?

No, your vision is 20/20, it's not necessary.

It was a copy/paste from the pr_<level> conversion above it
where 2 styles to convert to (dev_dbg and pr_debug) are used.

Here there is just one form and your review is appreciated.
I'll submit a V2 later.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4d2c7df..f50b32d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2430,6 +2430,16 @@  sub process {
 			     "Prefer pr_warn(... to pr_warning(...\n" . $herecurr);
 		}
 
+		if ($line =~ /\bdev_printk\s*\(\s*KERN_([A-Z]+)/) {
+			my $orig = $1;
+			my $level = lc($orig);
+			$level = "warn" if ($level eq "warning");
+			my $level2 = $level;
+			$level2 = "dbg" if ($level eq "debug");
+			WARN("PREFER_DEV_LEVEL",
+			     "Prefer dev_$level2(... to dev_printk(KERN_$orig, ...\n" . $herecurr);
+		}
+
 # function brace can't be on same line, except for #defines of do while,
 # or if closed on same line
 		if (($line=~/$Type\s*$Ident\(.*\).*\s{/) and