Skip to contentSkip to conversation

scsi: NCR5380: Mark expected switch fall-through

PATCH In review Gustavo A. R. Silva 4 participants 4 messages Feb 28, 2019 – Mar 6, 2019

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 scsi: NCR5380: Mark expected switch fall-through

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This patch fixes the following warning:

In file included from drivers/scsi/dmx3191d.c:48:
drivers/scsi/NCR5380.c: In function ‘NCR5380_information_transfer’:
drivers/scsi/NCR5380.c:1933:9: warning: this statement may fall through [-Wimplicit-fallthrough=]

      if (!hostdata->connected)
         ^
drivers/scsi/NCR5380.c:1937:5: note: here
     default:
     ^~~~~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.

This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.

---
Changes in v2:
- Update commit log.
- Move code comment after the default label and
retain reason for fall-through in comment as
requested by Michael Schmitz.

 drivers/scsi/NCR5380.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/scsi/NCR5380.c +4 -5
index 01c23d27f290..985d1c053578 100644
@@ -1933,13 +1933,12 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					if (!hostdata->connected)
 						return;
 
-					/* Fall through to reject message */
-
+					/* Fall through - to reject message */
+				default:
 					/*
-					 * If we get something weird that we aren't expecting,
-					 * reject it.
+					 * If we get something weird that we
+					 * aren't expecting, reject it.
 					 */
-				default:
 					if (tmp == EXTENDED_MESSAGE)
 						scmd_printk(KERN_INFO, cmd,
 						            "rejecting unknown extended message code %02x, length %d\n",
Signature · 1 line
2.21.0
#1 lore raw
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This switch case is already marked. So I think the patch description should state that this patch is actually a workaround for a gcc deficiency which prevents it from locating the marker.

Quoted 35 lines

This patch fixes the following warning:

In file included from drivers/scsi/dmx3191d.c:48:
drivers/scsi/NCR5380.c: In function ?NCR5380_information_transfer?:
drivers/scsi/NCR5380.c:1933:9: warning: this statement may fall through [-Wimplicit-fallthrough=]

      if (!hostdata->connected)
         ^
drivers/scsi/NCR5380.c:1937:5: note: here
     default:
     ^~~~~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.

This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.

---
Changes in v2:
- Update commit log.
- Move code comment after the default label and
retain reason for fall-through in comment as
requested by Michael Schmitz.

 drivers/scsi/NCR5380.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/scsi/NCR5380.c +0 -0
index 01c23d27f290..985d1c053578 100644

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1936

Quoted 6 lines
drivers/scsi/NCR5380.c +1 -2
@@ -1933,13 +1933,12 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					if (!hostdata->connected)
 						return;
 
-					/* Fall through to reject message */
-
+					/* Fall through - to reject message */

This new hyphen is wrong and harms readability for humans.

I did confirm that gcc can be appeased by the use of a hyphen but not by correct grammar such as "Fall through to reject message" or "Fall through. Reject message."

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1940

Quoted 6 lines
drivers/scsi/NCR5380.c +3 -2
+				default:
 					/*
-					 * If we get something weird that we aren't expecting,
-					 * reject it.
+					 * If we get something weird that we
+					 * aren't expecting, reject it.

This reformatting isn't relevant to this patch. The comments can be improved however (see below).

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1942

Quoted 2 lines
drivers/scsi/NCR5380.c +0 -1
 					 */
-				default:

Moving the 'default' keyword closer to the 'fall through' comment makes sense to me -- I could understand if gcc had simple, unambiguous rules for annotations.

Do compilers and static analysers agree as to what a correctly annotated switch label should look like? If not, we would have to try to mangle code and comments in such a way that might satisfy all of the failings in all of the tools.

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1944

Quoted 4 lines
drivers/scsi/NCR5380.c +0 -0
 					if (tmp == EXTENDED_MESSAGE)
 						scmd_printk(KERN_INFO, cmd,
 						            "rejecting unknown extended message code %02x, length %d\n",

Here's an alternative patch, which has the virtue that a simple heuristic will work. This patch does not require that other static analysis tools will follow gcc's weird rules about hyphens. (I assume they don't but I didn't check.)

drivers/scsi/NCR5380.c +4 -4
index 7fed9bb72784..fe0535affc14 100644
@@ -1932,13 +1932,13 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					if (!hostdata->connected)
 						return;
 
-					/* Fall through to reject message */
-
+					/* Reject message */
+					/* Fall through */
+				default:
 					/*
 					 * If we get something weird that we aren't expecting,
-					 * reject it.
+					 * log it.
 					 */
-				default:
 					if (tmp == EXTENDED_MESSAGE)
 						scmd_printk(KERN_INFO, cmd,
 						            "rejecting unknown extended message code %02x, length %d\n",
maintainer
#2 lore raw

Finn's version looks fine to me.

Cheers,

    Michael

Quoted 108 lines · Finn Thain
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

This switch case is already marked. So I think the patch description should state that this patch is actually a workaround for a gcc deficiency which prevents it from locating the marker.

Quoted 35 lines

This patch fixes the following warning:

In file included from drivers/scsi/dmx3191d.c:48:
drivers/scsi/NCR5380.c: In function ?NCR5380_information_transfer?:
drivers/scsi/NCR5380.c:1933:9: warning: this statement may fall through [-Wimplicit-fallthrough=]

       if (!hostdata->connected)
          ^
drivers/scsi/NCR5380.c:1937:5: note: here
      default:
      ^~~~~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find.

This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough.

---
Changes in v2:
- Update commit log.
- Move code comment after the default label and
retain reason for fall-through in comment as
requested by Michael Schmitz.

  drivers/scsi/NCR5380.c | 9 ++++-----
  1 file changed, 4 insertions(+), 5 deletions(-)
drivers/scsi/NCR5380.c +0 -0
index 01c23d27f290..985d1c053578 100644

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1936

Quoted 6 lines
drivers/scsi/NCR5380.c +1 -2
@@ -1933,13 +1933,12 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
  					if (!hostdata->connected)
  						return;
 
-					/* Fall through to reject message */
-
+					/* Fall through - to reject message */

This new hyphen is wrong and harms readability for humans.

I did confirm that gcc can be appeased by the use of a hyphen but not by correct grammar such as "Fall through to reject message" or "Fall through. Reject message."

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance)

Quoted 6 lines
drivers/scsi/NCR5380.c +3 -2
+				default:
  					/*
-					 * If we get something weird that we aren't expecting,
-					 * reject it.
+					 * If we get something weird that we
+					 * aren't expecting, reject it.

This reformatting isn't relevant to this patch. The comments can be improved however (see below).

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1941

Quoted 2 lines
drivers/scsi/NCR5380.c +0 -1
  					 */
-				default:

Moving the 'default' keyword closer to the 'fall through' comment makes sense to me -- I could understand if gcc had simple, unambiguous rules for annotations.

Do compilers and static analysers agree as to what a correctly annotated switch label should look like? If not, we would have to try to mangle code and comments in such a way that might satisfy all of the failings in all of the tools.

drivers/scsi/NCR5380.c static void NCR5380_information_transfer(struct Scsi_Host *instance) · line 1944

Quoted 4 lines
drivers/scsi/NCR5380.c +0 -0
  					if (tmp == EXTENDED_MESSAGE)
  						scmd_printk(KERN_INFO, cmd,
  						            "rejecting unknown extended message code %02x, length %d\n",

Here's an alternative patch, which has the virtue that a simple heuristic will work. This patch does not require that other static analysis tools will follow gcc's weird rules about hyphens. (I assume they don't but I didn't check.)

drivers/scsi/NCR5380.c +4 -4
index 7fed9bb72784..fe0535affc14 100644
@@ -1932,13 +1932,13 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
  					if (!hostdata->connected)
  						return;
 
-					/* Fall through to reject message */
-
+					/* Reject message */
+					/* Fall through */
+				default:
  					/*
  					 * If we get something weird that we aren't expecting,
-					 * reject it.
+					 * log it.
  					 */
-				default:
  					if (tmp == EXTENDED_MESSAGE)
  						scmd_printk(KERN_INFO, cmd,
  						            "rejecting unknown extended message code %02x, length %d\n",
#3 lore raw
In reply to Finn Thain

Hi Finn,

Please submit this as a formal patch. Thanks!

Quoted 27 lines

Here's an alternative patch, which has the virtue that a simple heuristic will work. This patch does not require that other static analysis tools will follow gcc's weird rules about hyphens. (I assume they don't but I didn't check.)

drivers/scsi/NCR5380.c +4 -4
index 7fed9bb72784..fe0535affc14 100644
@@ -1932,13 +1932,13 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					if (!hostdata->connected)
 						return;
 
-					/* Fall through to reject message */
-
+					/* Reject message */
+					/* Fall through */
+				default:
 					/*
 					 * If we get something weird that we aren't expecting,
-					 * reject it.
+					 * log it.
 					 */
-				default:
 					if (tmp == EXTENDED_MESSAGE)
 						scmd_printk(KERN_INFO, cmd,
 						            "rejecting unknown extended message code %02x, length %d\n",
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering