[PATCH] spi: slave: Fix missing break in switch

[PATCH] spi: slave: Fix missing break in switch

[Gustavo A. R. Silva]

Apparently, this code does not actually fall through to the next case
because the machine restarts before it has a chance. However, for the
sake of maintenance and readability, we better add the missing break
statement.

Addresses-Coverity-ID: 1437892 ("Missing break in switch")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/spi/spi-slave-system-control.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-slave-system-control.c b/drivers/spi/spi-slave-system-control.c
index c0257e9..169f3d5 100644
--- a/drivers/spi/spi-slave-system-control.c
+++ b/drivers/spi/spi-slave-system-control.c
@@ -60,6 +60,7 @@ static void spi_slave_system_control_complete(void *arg)
 	case CMD_REBOOT:
 		dev_info(&priv->spi->dev, "Rebooting system...\n");
 		kernel_restart(NULL);
+		break;
 
 	case CMD_POWEROFF:
 		dev_info(&priv->spi->dev, "Powering off system...\n");
-- 
2.7.4

Re: [PATCH] spi: slave: Fix missing break in switch

[Geert Uytterhoeven]

Hi Gustavo,

On Wed, Oct 3, 2018 at 2:57 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
> Apparently, this code does not actually fall through to the next case
> because the machine restarts before it has a chance. However, for the
> sake of maintenance and readability, we better add the missing break
> statement.
>
> Addresses-Coverity-ID: 1437892 ("Missing break in switch")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/spi/spi-slave-system-control.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/spi/spi-slave-system-control.c b/drivers/spi/spi-slave-system-control.c
> index c0257e9..169f3d5 100644
> --- a/drivers/spi/spi-slave-system-control.c
> +++ b/drivers/spi/spi-slave-system-control.c
> @@ -60,6 +60,7 @@ static void spi_slave_system_control_complete(void *arg)
>         case CMD_REBOOT:
>                 dev_info(&priv->spi->dev, "Rebooting system...\n");
>                 kernel_restart(NULL);
> +               break;
>
>         case CMD_POWEROFF:
>                 dev_info(&priv->spi->dev, "Powering off system...\n");

Alternatively, kernel_restart() and friends could be marked __noreturn.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: slave: Fix missing break in switch

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Wed, Oct 03, 2018 at 04:46:45PM +0200, Geert Uytterhoeven wrote:
> On Wed, Oct 3, 2018 at 2:57 PM Gustavo A. R. Silva

> >         case CMD_REBOOT:
> >                 dev_info(&priv->spi->dev, "Rebooting system...\n");
> >                 kernel_restart(NULL);
> > +               break;

> Alternatively, kernel_restart() and friends could be marked __noreturn.

Yes, that seems more sensible though there's no harm in this patch even
with that.  It'd definitely avoid other issues in future.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: slave: Fix missing break in switch

[Gustavo A. R. Silva]

Hi,

On 10/3/18 5:01 PM, Mark Brown wrote:
> On Wed, Oct 03, 2018 at 04:46:45PM +0200, Geert Uytterhoeven wrote:
>> On Wed, Oct 3, 2018 at 2:57 PM Gustavo A. R. Silva
> 
>>>         case CMD_REBOOT:
>>>                 dev_info(&priv->spi->dev, "Rebooting system...\n");
>>>                 kernel_restart(NULL);
>>> +               break;
> 
>> Alternatively, kernel_restart() and friends could be marked __noreturn.
> 
> Yes, that seems more sensible though there's no harm in this patch even
> with that.  It'd definitely avoid other issues in future.
> 

I'll include the __noreturn in addition to the break statement.
I'll send v2 shortly.

Thank you both for the feedback.
--
Gustavo

Re: [PATCH] spi: slave: Fix missing break in switch

[Geert Uytterhoeven]

Hi Gustavo,

On Wed, Oct 3, 2018 at 5:05 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
> On 10/3/18 5:01 PM, Mark Brown wrote:
> > On Wed, Oct 03, 2018 at 04:46:45PM +0200, Geert Uytterhoeven wrote:
> >> On Wed, Oct 3, 2018 at 2:57 PM Gustavo A. R. Silva
> >
> >>>         case CMD_REBOOT:
> >>>                 dev_info(&priv->spi->dev, "Rebooting system...\n");
> >>>                 kernel_restart(NULL);
> >>> +               break;
> >
> >> Alternatively, kernel_restart() and friends could be marked __noreturn.
> >
> > Yes, that seems more sensible though there's no harm in this patch even
> > with that.  It'd definitely avoid other issues in future.
>
> I'll include the __noreturn in addition to the break statement.
> I'll send v2 shortly.

Please note that adding __noreturn is not a trivial task, due to the complex
call chains, and the different implementations on the various architectures
and platforms.  So that will be a big patch series.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: slave: Fix missing break in switch

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On Wed, Oct 03, 2018 at 05:05:04PM +0200, Gustavo A. R. Silva wrote:

> I'll include the __noreturn in addition to the break statement.
> I'll send v2 shortly.

No need for a v2, I already applied this - adding __noreturn seems like
a separate (although desirable) effort.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: slave: Fix missing break in switch

[Gustavo A. R. Silva]

On 10/3/18 5:10 PM, Geert Uytterhoeven wrote:
> Hi Gustavo,
> 
> On Wed, Oct 3, 2018 at 5:05 PM Gustavo A. R. Silva
> <gustavo@embeddedor.com> wrote:
>> On 10/3/18 5:01 PM, Mark Brown wrote:
>>> On Wed, Oct 03, 2018 at 04:46:45PM +0200, Geert Uytterhoeven wrote:
>>>> On Wed, Oct 3, 2018 at 2:57 PM Gustavo A. R. Silva
>>>
>>>>>         case CMD_REBOOT:
>>>>>                 dev_info(&priv->spi->dev, "Rebooting system...\n");
>>>>>                 kernel_restart(NULL);
>>>>> +               break;
>>>
>>>> Alternatively, kernel_restart() and friends could be marked __noreturn.
>>>
>>> Yes, that seems more sensible though there's no harm in this patch even
>>> with that.  It'd definitely avoid other issues in future.
>>
>> I'll include the __noreturn in addition to the break statement.
>> I'll send v2 shortly.
> 
> Please note that adding __noreturn is not a trivial task, due to the complex
> call chains, and the different implementations on the various architectures
> and platforms.  So that will be a big patch series.
> 

I see. In that case, it might be an interesting side project.

I appreciate the feedback, Geert.
--
Gustavo