Checkpatch query

Checkpatch query

[Lee Jones]

Hi Andy, Joe,

When running checkpatch on drivers/mfd/tps80031.c I recieve the
following warning:

> WARNING: Possible switch case/default not preceeded by break or
>          fallthrough comment
> #337: FILE: drivers/mfd/tps80031.c:337:
> +       case TPS80031_BACKUP_REG:
> 
> total: 0 errors, 1 warnings, 573 lines checked

... but we use switch statement fall through all the time when
identifying different types of registers used with Regmap.  Placing a
fall-through comment between them all sounds very messy to me.

Also, the warning only fires on 'case's which do not use '...'
notation, which seems a little odd.

Anyway, I'm not sure placing a 'fall through' comment makes the code
any cleaner or reduces possible failure rate in any way.  Is there any
chance that this check can be removed?

> static bool rd_wr_reg_id0(struct device *dev, unsigned int reg)
> {
> 	switch (reg) {
> 	case TPS80031_SMPS1_CFG_FORCE ... TPS80031_SMPS2_CFG_VOLTAGE:
> 		return true;
> 	default:
> 		return false;
> 	}
> }
> 
> static bool rd_wr_reg_id1(struct device *dev, unsigned int reg)
> {
> 	switch (reg) {
> 	case TPS80031_SECONDS_REG ... TPS80031_RTC_RESET_STATUS_REG:
> 	case TPS80031_VALIDITY0 ... TPS80031_VALIDITY7:
> 	case TPS80031_PHOENIX_START_CONDITION ... TPS80031_KEY_PRESS_DUR_CFG:
> 	case TPS80031_SMPS4_CFG_TRANS ... TPS80031_SMPS3_CFG_VOLTAGE:
> 	case TPS80031_BROADCAST_ADDR_ALL ... TPS80031_BROADCAST_ADDR_CLK_RST:
> 	case TPS80031_VANA_CFG_TRANS ... TPS80031_LDO7_CFG_VOLTAGE:
> 	case TPS80031_REGEN1_CFG_TRANS ... TPS80031_TMP_CFG_STATE:
> 	case TPS80031_PREQ1_RES_ASS_A ... TPS80031_PREQ3_RES_ASS_C:
> 	case TPS80031_SMPS_OFFSET ... TPS80031_BATDEBOUNCING:
> 	case TPS80031_CFG_INPUT_PUPD1 ... TPS80031_CFG_SMPS_PD:
> 	case TPS80031_BACKUP_REG:
> 		return true;
> 	default:
> 		return false;
> 	}
> }
> 
> static bool is_volatile_reg_id1(struct device *dev, unsigned int reg)
> {
> 	switch (reg) {
> 	case TPS80031_SMPS4_CFG_TRANS ... TPS80031_SMPS3_CFG_VOLTAGE:
> 	case TPS80031_VANA_CFG_TRANS ... TPS80031_LDO7_CFG_VOLTAGE:
> 	case TPS80031_REGEN1_CFG_TRANS ... TPS80031_TMP_CFG_STATE:
> 	case TPS80031_PREQ1_RES_ASS_A ... TPS80031_PREQ3_RES_ASS_C:
> 	case TPS80031_SMPS_OFFSET ... TPS80031_BATDEBOUNCING:
> 	case TPS80031_CFG_INPUT_PUPD1 ... TPS80031_CFG_SMPS_PD:
> 		return true;
> 	default:
> 		return false;
> 	}
> }



-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Checkpatch query

[Joe Perches]

On Mon, 2014-07-21 at 12:39 +0100, Lee Jones wrote:
> Hi Andy, Joe,

Hi Lee.

> When running checkpatch on drivers/mfd/tps80031.c I recieve the
> following warning:
> 
> > WARNING: Possible switch case/default not preceeded by break or
> >          fallthrough comment
> > #337: FILE: drivers/mfd/tps80031.c:337:
> > +       case TPS80031_BACKUP_REG:
> > 
> > total: 0 errors, 1 warnings, 573 lines checked
> 
> ... but we use switch statement fall through all the time when
> identifying different types of registers used with Regmap.  Placing a
> fall-through comment between them all sounds very messy to me.

Yes, it can be messy, but it's found some
actual bugs/defects.

> Also, the warning only fires on 'case's which do not use '...'
> notation, which seems a little odd.

That's a checkpatch defect.  Thanks for the report.

> Anyway, I'm not sure placing a 'fall through' comment makes the code
> any cleaner or reduces possible failure rate in any way.  Is there any
> chance that this check can be removed?

You could use the --ignore= cmd-line option or add it to
a .checkpatch.conf file.

$ ./scripts/checkpatch.pl -f drivers/mfd/tps80031.c  --ignore=missing_break
WARNING: Possible unnecessary 'out of memory' message
#435: FILE: drivers/mfd/tps80031.c:435:
+	if (!tps80031) {
+		dev_err(&client->dev, "Malloc failed for tps80031\n");

total: 0 errors, 1 warnings, 573 lines checked

NOTE: Ignored message types: MISSING_BREAK

drivers/mfd/tps80031.c has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

[PATCH] checkpatch: Allow case labels with ranges

[Joe Perches]

The MISSING_BREAK / fallthrough test does not support
gcc's extension for case labels with ranges.

Add it.

Signed-off-by: Joe Perches <joe@perches.com>
Reported-by: Lee Jones <lee.jones@linaro.org>
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dc72a9b..2258497 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4767,7 +4767,7 @@ sub process {
 		}
 
 # check for case / default statements not preceeded by break/fallthrough/switch
-		if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*|default):/) {
+		if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*(?:\.\.\.\s*(?:$Ident|$Constant)\s*)?|default):/) {
 			my $has_break = 0;
 			my $has_statement = 0;
 			my $count = 0;
@@ -4778,7 +4778,7 @@ sub process {
 				my $fline = $lines[$prevline - 1];
 				last if ($fline =~ /^\@\@/);
 				next if ($fline =~ /^\-/);
-				next if ($fline =~ /^.(?:\s*(?:case\s+(?:$Ident|$Constant)[\s$;]*|default):[\s$;]*)*$/);
+				next if ($fline =~ /^.(?:\s*(?:case\s+(?:$Ident|$Constant(?:\s*\.\.\.\s*(?:$Ident|$Constant))?)[\s$;]*|default):[\s$;]*)*$/);
 				$has_break = 1 if ($rline =~ /fall[\s_-]*(through|thru)/i);
 				next if ($fline =~ /^.[\s$;]*$/);
 				$has_statement = 1;

Re: [PATCH] checkpatch: Allow case labels with ranges

[Lee Jones]

On Mon, 21 Jul 2014, Joe Perches wrote:

> The MISSING_BREAK / fallthrough test does not support
> gcc's extension for case labels with ranges.
> 
> Add it.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> Reported-by: Lee Jones <lee.jones@linaro.org>
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

[...]

Yep, that's much worse [1] - I wish I never mentioned it. ;)
 
Acked-by: Lee Jones <lee.jones@linaro.org>

[1]

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#330: FILE: drivers/mfd/tps80031.c:330:
+	case TPS80031_SMPS4_CFG_TRANS ... TPS80031_SMPS3_CFG_VOLTAGE:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#331: FILE: drivers/mfd/tps80031.c:331:
+	case TPS80031_BROADCAST_ADDR_ALL ... TPS80031_BROADCAST_ADDR_CLK_RST:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#332: FILE: drivers/mfd/tps80031.c:332:
+	case TPS80031_VANA_CFG_TRANS ... TPS80031_LDO7_CFG_VOLTAGE:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#333: FILE: drivers/mfd/tps80031.c:333:
+	case TPS80031_REGEN1_CFG_TRANS ... TPS80031_TMP_CFG_STATE:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#334: FILE: drivers/mfd/tps80031.c:334:
+	case TPS80031_PREQ1_RES_ASS_A ... TPS80031_PREQ3_RES_ASS_C:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#335: FILE: drivers/mfd/tps80031.c:335:
+	case TPS80031_SMPS_OFFSET ... TPS80031_BATDEBOUNCING:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#336: FILE: drivers/mfd/tps80031.c:336:
+	case TPS80031_CFG_INPUT_PUPD1 ... TPS80031_CFG_SMPS_PD:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#351: FILE: drivers/mfd/tps80031.c:351:
+	case TPS80031_PREQ1_RES_ASS_A ... TPS80031_PREQ3_RES_ASS_C:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#352: FILE: drivers/mfd/tps80031.c:352:
+	case TPS80031_SMPS_OFFSET ... TPS80031_BATDEBOUNCING:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#353: FILE: drivers/mfd/tps80031.c:353:
+	case TPS80031_CFG_INPUT_PUPD1 ... TPS80031_CFG_SMPS_PD:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#366: FILE: drivers/mfd/tps80031.c:366:
+	case TPS80031_TOGGLE1 ... TPS80031_VIBMODE:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#367: FILE: drivers/mfd/tps80031.c:367:
+	case TPS80031_PWM1ON ... TPS80031_PWM2OFF:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#368: FILE: drivers/mfd/tps80031.c:368:
+	case TPS80031_FG_REG_00 ... TPS80031_FG_REG_11:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#369: FILE: drivers/mfd/tps80031.c:369:
+	case TPS80031_INT_STS_A ... TPS80031_INT_MSK_STS_C:

WARNING: Possible switch case/default not preceeded by break or fallthrough comment
#370: FILE: drivers/mfd/tps80031.c:370:
+	case TPS80031_CONTROLLER_CTRL2 ... TPS80031_LED_PWM_CTRL2:

total: 0 errors, 15 warnings, 574 lines checked

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] checkpatch: Allow case labels with ranges

[Joe Perches]

On Mon, 2014-07-21 at 14:21 +0100, Lee Jones wrote:
> On Mon, 21 Jul 2014, Joe Perches wrote:
> > The MISSING_BREAK / fallthrough test does not support
> > gcc's extension for case labels with ranges.
[]
> Yep, that's much worse [1] - I wish I never mentioned it. ;)

I'm glad you did actually.

The code itself:

bool foo(int bar)
{
	case (bar) {
	case 1:
	case 2:
	case 4:
		return true;
	default:
		return false;
}

should not emit any error.

I'll fix it soon.

Re: [PATCH] checkpatch: Allow case labels with ranges

[Lee Jones]

On Mon, 21 Jul 2014, Joe Perches wrote:

> On Mon, 2014-07-21 at 14:21 +0100, Lee Jones wrote:
> > On Mon, 21 Jul 2014, Joe Perches wrote:
> > > The MISSING_BREAK / fallthrough test does not support
> > > gcc's extension for case labels with ranges.
> []
> > Yep, that's much worse [1] - I wish I never mentioned it. ;)
> 
> I'm glad you did actually.
> 
> The code itself:
> 
> bool foo(int bar)
> {
> 	case (bar) {
> 	case 1:
> 	case 2:
> 	case 4:
> 		return true;
> 	default:
> 		return false;
> }
> 
> should not emit any error.

That's why I submitted the report.

> I'll fix it soon.

Brilliant, thanks Joe!

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] checkpatch: Fix false positive MISSING_BREAK warnings with --file

[Joe Perches]

Using --file mode can give false positives with MISSING_BREAK
fall-through warnings on simple but long multiple consecutive
case statements.

Look for all lines before a case statement for a switch
or a statement when using --file mode.

Fix a misspelling of preceded while there.

Signed-off-by: Joe Perches <joe@perches.com>
Reported-by: Lee Jones <lee.jones@linaro.org>
---

On Mon, 2014-07-21 at 14:30 -0700, Andrew Morton wrote:
> On Mon, 21 Jul 2014 07:28:49 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > I'll fix it soon.
> 
> wanna do s/preceeded/preceded/g while you're in there?

Sure.  It might be a few days though as
a generic fix seems non-trivial.

There's a trivial --file fix, but I'm not
sure the patch one is appropriate.

In the mean time, the case range label patch
should be OK to commit.

 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2258497..74afc7c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4766,13 +4766,13 @@ sub process {
 			}
 		}
 
-# check for case / default statements not preceeded by break/fallthrough/switch
+# check for case / default statements not preceded by break/fallthrough/switch
 		if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*(?:\.\.\.\s*(?:$Ident|$Constant)\s*)?|default):/) {
 			my $has_break = 0;
 			my $has_statement = 0;
 			my $count = 0;
 			my $prevline = $linenr;
-			while ($prevline > 1 && $count < 3 && !$has_break) {
+			while ($prevline > 1 && ($file || $count < 3) && !$has_break) {
 				$prevline--;
 				my $rline = $rawlines[$prevline - 1];
 				my $fline = $lines[$prevline - 1];

Re: [PATCH] checkpatch: Fix false positive MISSING_BREAK warnings with --file

[Lee Jones]

On Mon, 21 Jul 2014, Joe Perches wrote:
> Using --file mode can give false positives with MISSING_BREAK
> fall-through warnings on simple but long multiple consecutive
> case statements.
> 
> Look for all lines before a case statement for a switch
> or a statement when using --file mode.
> 
> Fix a misspelling of preceded while there.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> Reported-by: Lee Jones <lee.jones@linaro.org>

Solution works for me.

> total: 0 errors, 0 warnings, 574 lines checked
> 
> drivers/mfd/tps80031.c has no obvious style problems and is ready
> for submission.

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog