watchdog: pcwd_pci: mark expected switch fall-through

PATCH In review Gustavo A. R. Silva 4 participants 9 messages Nov 3, 2017 – Nov 5, 2017 Watchdog device drivers

lore

Share & tools

lore

Gustavo A. R. Silva

author

2017-11-03 14:49 UTC #0 lore raw

PATCH 1/1 watchdog: pcwd_pci: mark expected switch fall-through

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

Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/pcwd_pci.c +1 -1

index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 
 	case WDIOC_GETTIMEOUT:

Signature · 1 line

2.7.4

Wim Van Sebroeck

2017-11-03 15:04 UTC #1 lore raw

Hi Gustavo,

Quoted 27 lines

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/pcwd_pci.c +1 -1
index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 
 	case WDIOC_GETTIMEOUT:
Signature · 1 line
2.7.4

Shouldn't the /* fall through */ come after the } ?

Kind regards,
Wim.

Gustavo A. R. Silva

author

2017-11-03 15:47 UTC #2 lore raw

Hi Wim,

Quoting Wim Van Sebroeck <[email protected]>:

Quoted 26 lines

Hi Gustavo,
Quoted 24 lines
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/pcwd_pci.c +1 -1
index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file,  
 unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 

Quoted 7 lines

Quoted 4 lines
case WDIOC_GETTIMEOUT:
--
2.7.4
Shouldn't the /* fall through */ come after the } ?

Yep, you are right.

I'll fix that.

Thank you!
--
Gustavo A. R. Silva

Quoted 2 lines

Kind regards,
Wim.

Gustavo A. R. Silva

author

2017-11-03 16:02 UTC #3 lore raw

PATCH v2 1/1 watchdog: pcwd_pci: mark expected switch fall-through

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

Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
Place the "fall through" comment outside the unconditional code block.

 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/pcwd_pci.c +1 -1

index c0d07ee..1f78f09 100644
@@ -545,8 +545,8 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
 	}
+		/* fall through */
 
 	case WDIOC_GETTIMEOUT:
 		return put_user(heartbeat, p);

Signature · 1 line

2.7.4

Guenter Roeck

maintainer Reviewed-by on v2 1/1

2017-11-05 15:11 UTC #4 lore raw

Quoted 7 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>

Reviewed-by Guenter Roeck <[email protected]>

Quoted 21 lines

---
Changes in v2:
Place the "fall through" comment outside the unconditional code block.

 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/pcwd_pci.c +1 -1

index c0d07ee..1f78f09 100644
@@ -545,8 +545,8 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
 	}
+		/* fall through */
 
 	case WDIOC_GETTIMEOUT:
 		return put_user(heartbeat, p);

Guenter Roeck

maintainer

2017-11-03 15:54 UTC #5 lore raw

In reply to Wim Van Sebroeck

Quoted 25 lines · Wim Van Sebroeck

Hi Gustavo,
Quoted 23 lines
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/pcwd_pci.c +1 -1
index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 

Quoted 7 lines

Quoted 4 lines
drivers/watchdog/pcwd_pci.c +0 -0
 	case WDIOC_GETTIMEOUT:
Signature · 1 line
2.7.4
Shouldn't the /* fall through */ come after the } ?

Good question. This is an unconditional code block needed to declare a local variable within the case statement. What is correct in that situation ?

Guenter

Gustavo A. R. Silva

author

2017-11-03 15:57 UTC #6 lore raw

Quoting Guenter Roeck <[email protected]>:

Quoted 34 lines · Wim Van Sebroeck

Quoted 26 lines
Hi Gustavo,
drivers/watchdog/pcwd_pci.c static long pcipcwd_ioctl(struct file *file,
Quoted 16 lines
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/pcwd_pci.c +0 -0
index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file,  
unsigned int cmd,
Quoted 7 lines
drivers/watchdog/pcwd_pci.c +1 -1
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 
Quoted 7 lines
Quoted 4 lines
drivers/watchdog/pcwd_pci.c +0 -1
 	case WDIOC_GETTIMEOUT:
--
2.7.4
Shouldn't the /* fall through */ come after the } ?

Quoted 4 lines

Good question. This is an unconditional code block needed to declare a local variable within the case statement. What is correct in that situation ?

I think it is correct to place the comment outside the code block.

I'll send a patch shortly.

Thanks
--
Gustavo A. R. Silva

Joe Perches

2017-11-03 16:02 UTC #7 lore raw

In reply to Guenter Roeck

Quoted 34 lines · Guenter Roeck

Quoted 25 lines · Wim Van Sebroeck
Hi Gustavo,
Quoted 23 lines
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/pcwd_pci.c +1 -1
index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 
Quoted 7 lines
Quoted 4 lines
drivers/watchdog/pcwd_pci.c +0 -0
 	case WDIOC_GETTIMEOUT:
Signature · 1 line
2.7.4
Shouldn't the /* fall through */ come after the } ?

Quoted 3 lines

Good question. This is an unconditional code block needed to declare a local variable within the case statement. What is correct in that situation ?

I think it'd be clearer to avoid the trivial fallthrough optimization/complexity and just directly use

		return put_user(new_heartbeat, p);

as heartbeat and new_heartbeat are now the same value here.

Guenter Roeck

maintainer

2017-11-03 16:15 UTC #8 lore raw

Quoted 39 lines · Joe Perches

Quoted 34 lines · Guenter Roeck
Quoted 25 lines · Wim Van Sebroeck
Hi Gustavo,
Quoted 23 lines
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Notice that in this particular case I replaced "Fall" with a proper "fall through" comment, which is what GCC is expecting to find.
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/watchdog/pcwd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/watchdog/pcwd_pci.c +1 -1
index c0d07ee..c882252 100644
@@ -545,7 +545,7 @@ static long pcipcwd_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		pcipcwd_keepalive();
-		/* Fall */
+		/* fall through */
 	}
 
Quoted 7 lines
Quoted 4 lines
drivers/watchdog/pcwd_pci.c +0 -0
 	case WDIOC_GETTIMEOUT:
Signature · 1 line
2.7.4
Shouldn't the /* fall through */ come after the } ?
Quoted 3 lines
Good question. This is an unconditional code block needed to declare a local variable within the case statement. What is correct in that situation ?

Quoted 7 lines

I think it'd be clearer to avoid the trivial fallthrough optimization/complexity and just directly use
		return put_user(new_heartbeat, p);
as heartbeat and new_heartbeat are now the same value here.

I don't think it really matters. What would matter would be for someone to convert he driver to use the watchdog subsystem.

Guenter