[072/141] can: peak_usb: Fix fall-through warnings for Clang

Message ID aab7cf16bf43cc7c3e9c9930d2dae850c1d07a3c.1605896059.git.gustavoars@kernel.org
State New
Headers show
Series
  • Fix fall-through warnings for Clang
Related show

Commit Message

Gustavo A. R. Silva Nov. 20, 2020, 6:34 p.m.
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning
by explicitly adding a break statement instead of letting the code fall
through to the next case.

Link: https://github.com/KSPP/linux/issues/115
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Marc Kleine-Budde Nov. 21, 2020, 1:17 p.m. | #1
On 11/20/20 7:34 PM, Gustavo A. R. Silva wrote:
> In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning

> by explicitly adding a break statement instead of letting the code fall

> through to the next case.

> 

> Link: https://github.com/KSPP/linux/issues/115

> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

> ---

>  drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> index c2764799f9ef..fd65a155be3b 100644

> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> @@ -299,6 +299,8 @@ static void peak_usb_write_bulk_callback(struct urb *urb)

>  		if (net_ratelimit())

>  			netdev_err(netdev, "Tx urb aborted (%d)\n",

>  				   urb->status);

> +		break;

> +

>  	case -EPROTO:

>  	case -ENOENT:

>  	case -ECONNRESET:

> 


What about moving the default to the end if the case, which is more common anyways:

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index 204ccb27d6d9..e8977dd10902 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
                netif_trans_update(netdev);
                break;
 
-       default:
-               if (net_ratelimit())
-                       netdev_err(netdev, "Tx urb aborted (%d)\n",
-                                  urb->status);
        case -EPROTO:
        case -ENOENT:
        case -ECONNRESET:
        case -ESHUTDOWN:
-
                break;
+
+       default:
+               if (net_ratelimit())
+                       netdev_err(netdev, "Tx urb aborted (%d)\n",
+                                  urb->status);
        }
 
        /* should always release echo skb and corresponding context */


Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>


Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Joe Perches Nov. 21, 2020, 7:50 p.m. | #2
On Sat, 2020-11-21 at 14:17 +0100, Marc Kleine-Budde wrote:
> On 11/20/20 7:34 PM, Gustavo A. R. Silva wrote:

> > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning

> > by explicitly adding a break statement instead of letting the code fall

> > through to the next case.

> > 

> > Link: https://github.com/KSPP/linux/issues/115

> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

[]
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

[]
> > @@ -299,6 +299,8 @@ static void peak_usb_write_bulk_callback(struct urb *urb)

> >  		if (net_ratelimit())

> >  			netdev_err(netdev, "Tx urb aborted (%d)\n",

> >  				   urb->status);

> > +		break;

> > +

> >  	case -EPROTO:

> >  	case -ENOENT:

> >  	case -ECONNRESET:

> > 

> 

> What about moving the default to the end if the case, which is more common anyways:

> 

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

[]
> @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)

>                 netif_trans_update(netdev);

>                 break;

>  

> 

> -       default:

> -               if (net_ratelimit())

> -                       netdev_err(netdev, "Tx urb aborted (%d)\n",

> -                                  urb->status);

>         case -EPROTO:

>         case -ENOENT:

>         case -ECONNRESET:

>         case -ESHUTDOWN:

> -

>                 break;

> +

> +       default:

> +               if (net_ratelimit())

> +                       netdev_err(netdev, "Tx urb aborted (%d)\n",

> +                                  urb->status);


That's fine and is more generally used style but this
default: case should IMO also end with a break;

+		break;

>         }
Marc Kleine-Budde Nov. 21, 2020, 11:04 p.m. | #3
On 11/21/20 8:50 PM, Joe Perches wrote:
>> What about moving the default to the end if the case, which is more common anyways:

>>

>> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> []

>> @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)

>>                 netif_trans_update(netdev);

>>                 break;

>>  

>>

>> -       default:

>> -               if (net_ratelimit())

>> -                       netdev_err(netdev, "Tx urb aborted (%d)\n",

>> -                                  urb->status);

>>         case -EPROTO:

>>         case -ENOENT:

>>         case -ECONNRESET:

>>         case -ESHUTDOWN:

>> -

>>                 break;

>> +

>> +       default:

>> +               if (net_ratelimit())

>> +                       netdev_err(netdev, "Tx urb aborted (%d)\n",

>> +                                  urb->status);

> 

> That's fine and is more generally used style but this

> default: case should IMO also end with a break;

> 

> +		break;


I don't mind.

process/coding-style.rst is not totally clear about the break after the default,
if this is the lase one the switch statement.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Joe Perches Nov. 22, 2020, 2:46 a.m. | #4
On Sun, 2020-11-22 at 00:04 +0100, Marc Kleine-Budde wrote:
> On 11/21/20 8:50 PM, Joe Perches wrote:

> > > What about moving the default to the end if the case, which is more common anyways:

> > > 

> > > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

> > []

> > > @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)

> > >                 netif_trans_update(netdev);

> > >                 break;

> > >  

> > > 

> > > -       default:

> > > -               if (net_ratelimit())

> > > -                       netdev_err(netdev, "Tx urb aborted (%d)\n",

> > > -                                  urb->status);

> > >         case -EPROTO:

> > >         case -ENOENT:

> > >         case -ECONNRESET:

> > >         case -ESHUTDOWN:

> > > -

> > >                 break;

> > > +

> > > +       default:

> > > +               if (net_ratelimit())

> > > +                       netdev_err(netdev, "Tx urb aborted (%d)\n",

> > > +                                  urb->status);

> > 

> > That's fine and is more generally used style but this

> > default: case should IMO also end with a break;

> > 

> > +		break;

> 

> I don't mind.

> 

> process/coding-style.rst is not totally clear about the break after the default,

> if this is the lase one the switch statement.


deprecated.rst has:

All switch/case blocks must end in one of:

* break;
* fallthrough;
* continue;
* goto <label>;
* return [expression];

I suppose that could be moved into coding-style along with
maybe a change to "all switch/case/default blocks"

Patch

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
index c2764799f9ef..fd65a155be3b 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
@@ -299,6 +299,8 @@  static void peak_usb_write_bulk_callback(struct urb *urb)
 		if (net_ratelimit())
 			netdev_err(netdev, "Tx urb aborted (%d)\n",
 				   urb->status);
+		break;
+
 	case -EPROTO:
 	case -ENOENT:
 	case -ECONNRESET: