| Message ID | 20181003123328.GA29471@embeddedor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c24bfa8f21b59283580043dada19a6e943b6e426 |
| Headers | show |
| Series | spi: slave: Fix missing break in switch | expand |
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
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.
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
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
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.
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
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");
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(+)