[Qemu-devel] [PATCH 0/6] arm devices: mark or remove implicit fallthroughs

[Qemu-devel] [PATCH 0/6] arm devices: mark or remove implicit fallthroughs

[Peter Maydell]

These patches either mark implicit fallthroughs in case statements
or (in a few cases) remove them by putting in an explicit 'break'
or 'return' rather than relying on the one in the following case.
There is no behaviour change for any of these patches, and in all
cases I've examined the code and am happy that the behaviour is
intentional and correct.

This isn't intended to be a comprehensive patchset (or even a
comprehensive patchset for the ARM related files); it's just a
set of half a dozen changes for files which I care about and where
the existing behaviour was definitely correct.

I'm not a fan of the code pattern used in the omap and pxa2xx
code of incrementing an index and then falling through to an
array access, but it doesn't seem sufficiently ugly to be worth
rewriting.

Peter Maydell (6):
  hw/arm_sysctl.c: Add missing 'break' statements
  hw/omap1.c: Add fallthrough markers and breaks
  hw/omap_dma, hw/omap_spi: Explicitly mark fallthroughs
  hw/pflash_cfi02.c: Mark deliberate fallthrough
  hw/smc91c111: Add explicit 'return' rather than relying on
    fallthrough
  hw/pxa2xx_timer: Explicitly mark fallthroughs

 hw/arm_sysctl.c   |    2 ++
 hw/omap1.c        |    3 +++
 hw/omap_dma.c     |   12 ++++++++++++
 hw/omap_spi.c     |   24 ++++++++++++++++++++++++
 hw/pflash_cfi02.c |    1 +
 hw/pxa2xx_timer.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/smc91c111.c    |    1 +
 7 files changed, 90 insertions(+)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/6] hw/arm_sysctl.c: Add missing 'break' statements

[Peter Maydell]

Add some break statements that were accidentally omitted
from some cases of arm_sysctl_write(). The omission was
harmless because in both cases the following case did
an immediate break, but adding the breaks explicitly
placates static analysers and avoids weird behaviour if
the following register is ever implemented as something
other than a no-op.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm_sysctl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
index a196fcc..6d99f9c 100644
--- a/hw/arm_sysctl.c
+++ b/hw/arm_sysctl.c
@@ -199,6 +199,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
     switch (offset) {
     case 0x08: /* LED */
         s->leds = val;
+        break;
     case 0x0c: /* OSC0 */
     case 0x10: /* OSC1 */
     case 0x14: /* OSC2 */
@@ -295,6 +296,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
             /* On VExpress this register is unimplemented and will RAZ/WI */
             break;
         }
+        break;
     case 0x54: /* CLCDSER */
     case 0x64: /* DMAPSR0 */
     case 0x68: /* DMAPSR1 */
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/6] hw/omap1.c: Add fallthrough markers and breaks

[Peter Maydell]

Explicitly mark cases where we are deliberately falling
through to the following code. In one case we insert a
'break' instead of falling through to a 'break', as this
seems slightly clearer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/omap1.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/omap1.c b/hw/omap1.c
index e85f2e2..cc19893 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -529,6 +529,7 @@ static uint64_t omap_ulpd_pm_read(void *opaque, hwaddr addr,
     case 0x28:	/* Reserved */
     case 0x2c:	/* Reserved */
         OMAP_BAD_REG(addr);
+        /* fall through */
     case 0x00:	/* COUNTER_32_LSB */
     case 0x04:	/* COUNTER_32_MSB */
     case 0x08:	/* COUNTER_HIGH_FREQ_LSB */
@@ -633,6 +634,7 @@ static void omap_ulpd_pm_write(void *opaque, hwaddr addr,
     case 0x28:	/* Reserved */
     case 0x2c:	/* Reserved */
         OMAP_BAD_REG(addr);
+        /* fall through */
     case 0x24:	/* SETUP_ANALOG_CELL3_ULPD1 */
     case 0x38:	/* COUNTER_32_FIQ */
     case 0x48:	/* LOCL_TIME */
@@ -1089,6 +1091,7 @@ static void omap_mpui_write(void *opaque, hwaddr addr,
     /* Not in OMAP310 */
     case 0x14:	/* DSP_STATUS */
         OMAP_RO_REG(addr);
+        break;
     case 0x18:	/* DSP_BOOT_CONFIG */
     case 0x1c:	/* DSP_MPUI_CONFIG */
         break;
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/6] hw/omap_dma, hw/omap_spi: Explicitly mark fallthroughs

[Peter Maydell]

Explicitly mark the fallthroughs as intentional in the code
pattern where we gradually increment an index before falling
into the code to read/write that array entry:
  case THINGY_3: idx++;
  case THINGY_2: idx++;
  case THINGY_1: idx++;
  case THINGY_0: return s->thingy[idx];

This makes static analysers happy.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/omap_dma.c |   12 ++++++++++++
 hw/omap_spi.c |   24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/hw/omap_dma.c b/hw/omap_dma.c
index aec5874..0c878b6 100644
--- a/hw/omap_dma.c
+++ b/hw/omap_dma.c
@@ -1709,19 +1709,25 @@ static uint64_t omap_dma4_read(void *opaque, hwaddr addr,
 
     case 0x14:	/* DMA4_IRQSTATUS_L3 */
         irqn ++;
+        /* fall through */
     case 0x10:	/* DMA4_IRQSTATUS_L2 */
         irqn ++;
+        /* fall through */
     case 0x0c:	/* DMA4_IRQSTATUS_L1 */
         irqn ++;
+        /* fall through */
     case 0x08:	/* DMA4_IRQSTATUS_L0 */
         return s->irqstat[irqn];
 
     case 0x24:	/* DMA4_IRQENABLE_L3 */
         irqn ++;
+        /* fall through */
     case 0x20:	/* DMA4_IRQENABLE_L2 */
         irqn ++;
+        /* fall through */
     case 0x1c:	/* DMA4_IRQENABLE_L1 */
         irqn ++;
+        /* fall through */
     case 0x18:	/* DMA4_IRQENABLE_L0 */
         return s->irqen[irqn];
 
@@ -1856,10 +1862,13 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
     switch (addr) {
     case 0x14:	/* DMA4_IRQSTATUS_L3 */
         irqn ++;
+        /* fall through */
     case 0x10:	/* DMA4_IRQSTATUS_L2 */
         irqn ++;
+        /* fall through */
     case 0x0c:	/* DMA4_IRQSTATUS_L1 */
         irqn ++;
+        /* fall through */
     case 0x08:	/* DMA4_IRQSTATUS_L0 */
         s->irqstat[irqn] &= ~value;
         if (!s->irqstat[irqn])
@@ -1868,10 +1877,13 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
 
     case 0x24:	/* DMA4_IRQENABLE_L3 */
         irqn ++;
+        /* fall through */
     case 0x20:	/* DMA4_IRQENABLE_L2 */
         irqn ++;
+        /* fall through */
     case 0x1c:	/* DMA4_IRQENABLE_L1 */
         irqn ++;
+        /* fall through */
     case 0x18:	/* DMA4_IRQENABLE_L0 */
         s->irqen[irqn] = value;
         return;
diff --git a/hw/omap_spi.c b/hw/omap_spi.c
index 42d5149..8ff01ed 100644
--- a/hw/omap_spi.c
+++ b/hw/omap_spi.c
@@ -167,32 +167,47 @@ static uint64_t omap_mcspi_read(void *opaque, hwaddr addr,
         return s->control;
 
     case 0x68: ch ++;
+        /* fall through */
     case 0x54: ch ++;
+        /* fall through */
     case 0x40: ch ++;
+        /* fall through */
     case 0x2c:	/* MCSPI_CHCONF */
         return s->ch[ch].config;
 
     case 0x6c: ch ++;
+        /* fall through */
     case 0x58: ch ++;
+        /* fall through */
     case 0x44: ch ++;
+        /* fall through */
     case 0x30:	/* MCSPI_CHSTAT */
         return s->ch[ch].status;
 
     case 0x70: ch ++;
+        /* fall through */
     case 0x5c: ch ++;
+        /* fall through */
     case 0x48: ch ++;
+        /* fall through */
     case 0x34:	/* MCSPI_CHCTRL */
         return s->ch[ch].control;
 
     case 0x74: ch ++;
+        /* fall through */
     case 0x60: ch ++;
+        /* fall through */
     case 0x4c: ch ++;
+        /* fall through */
     case 0x38:	/* MCSPI_TX */
         return s->ch[ch].tx;
 
     case 0x78: ch ++;
+        /* fall through */
     case 0x64: ch ++;
+        /* fall through */
     case 0x50: ch ++;
+        /* fall through */
     case 0x3c:	/* MCSPI_RX */
         s->ch[ch].status &= ~(1 << 0);			/* RXS */
         ret = s->ch[ch].rx;
@@ -269,8 +284,11 @@ static void omap_mcspi_write(void *opaque, hwaddr addr,
         break;
 
     case 0x68: ch ++;
+        /* fall through */
     case 0x54: ch ++;
+        /* fall through */
     case 0x40: ch ++;
+        /* fall through */
     case 0x2c:	/* MCSPI_CHCONF */
         if ((value ^ s->ch[ch].config) & (3 << 14))	/* DMAR | DMAW */
             omap_mcspi_dmarequest_update(s->ch + ch);
@@ -283,8 +301,11 @@ static void omap_mcspi_write(void *opaque, hwaddr addr,
         break;
 
     case 0x70: ch ++;
+        /* fall through */
     case 0x5c: ch ++;
+        /* fall through */
     case 0x48: ch ++;
+        /* fall through */
     case 0x34:	/* MCSPI_CHCTRL */
         if (value & ~s->ch[ch].control & 1) {		/* EN */
             s->ch[ch].control |= 1;
@@ -294,8 +315,11 @@ static void omap_mcspi_write(void *opaque, hwaddr addr,
         break;
 
     case 0x74: ch ++;
+        /* fall through */
     case 0x60: ch ++;
+        /* fall through */
     case 0x4c: ch ++;
+        /* fall through */
     case 0x38:	/* MCSPI_TX */
         s->ch[ch].tx = value;
         s->ch[ch].status &= ~(1 << 1);			/* TXS */
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/6] hw/pflash_cfi02.c: Mark deliberate fallthrough

[Peter Maydell]

Mark the deliberate fallthrough where we treat the case of
an attempt to read flash when it is an unknown command
state as if it were a normal read.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pflash_cfi02.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index cfb91cb..d66c0ca 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -157,6 +157,7 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
         pfl->wcycle = 0;
         pfl->cmd = 0;
+        /* fall through to the read code */
     case 0x80:
         /* We accept reads during second unlock sequence... */
     case 0x00:
-- 
1.7.9.5

[Qemu-devel] [PATCH 5/6] hw/smc91c111: Add explicit 'return' rather than relying on fallthrough

[Peter Maydell]

Add an explicit 'return' statement to a case in smc91c111_readb
rather than relying on fallthrough to the following case's
return statement, for code clarity and to placate static analysers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/smc91c111.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/smc91c111.c b/hw/smc91c111.c
index a34698f..b605069 100644
--- a/hw/smc91c111.c
+++ b/hw/smc91c111.c
@@ -442,6 +442,7 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
             return;
         case 12: /* Early receive.  */
             s->ercv = value & 0x1f;
+            return;
         case 13:
             /* Ignore.  */
             return;
-- 
1.7.9.5

[Qemu-devel] [PATCH 6/6] hw/pxa2xx_timer: Explicitly mark fallthroughs

[Peter Maydell]

Explicitly mark the fallthroughs as intentional in the code
pattern where we gradually increment an index before falling
into the code to read/write that array entry:
    case THINGY_3: idx++;
    case THINGY_2: idx++;
    case THINGY_1: idx++;
    case THINGY_0: return s->thingy[idx];

This makes static analysers happy.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pxa2xx_timer.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/hw/pxa2xx_timer.c b/hw/pxa2xx_timer.c
index 32c1872..5c9d2e8 100644
--- a/hw/pxa2xx_timer.c
+++ b/hw/pxa2xx_timer.c
@@ -157,17 +157,27 @@ static uint64_t pxa2xx_timer_read(void *opaque, hwaddr offset,
 
     switch (offset) {
     case OSMR3:  tm ++;
+        /* fall through */
     case OSMR2:  tm ++;
+        /* fall through */
     case OSMR1:  tm ++;
+        /* fall through */
     case OSMR0:
         return s->timer[tm].value;
     case OSMR11: tm ++;
+        /* fall through */
     case OSMR10: tm ++;
+        /* fall through */
     case OSMR9:  tm ++;
+        /* fall through */
     case OSMR8:  tm ++;
+        /* fall through */
     case OSMR7:  tm ++;
+        /* fall through */
     case OSMR6:  tm ++;
+        /* fall through */
     case OSMR5:  tm ++;
+        /* fall through */
     case OSMR4:
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
@@ -176,12 +186,19 @@ static uint64_t pxa2xx_timer_read(void *opaque, hwaddr offset,
         return s->clock + muldiv64(qemu_get_clock_ns(vm_clock) -
                         s->lastload, s->freq, get_ticks_per_sec());
     case OSCR11: tm ++;
+        /* fall through */
     case OSCR10: tm ++;
+        /* fall through */
     case OSCR9:  tm ++;
+        /* fall through */
     case OSCR8:  tm ++;
+        /* fall through */
     case OSCR7:  tm ++;
+        /* fall through */
     case OSCR6:  tm ++;
+        /* fall through */
     case OSCR5:  tm ++;
+        /* fall through */
     case OSCR4:
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
@@ -207,12 +224,19 @@ static uint64_t pxa2xx_timer_read(void *opaque, hwaddr offset,
     case OWER:
         return s->reset3;
     case OMCR11: tm ++;
+        /* fall through */
     case OMCR10: tm ++;
+        /* fall through */
     case OMCR9:  tm ++;
+        /* fall through */
     case OMCR8:  tm ++;
+        /* fall through */
     case OMCR7:  tm ++;
+        /* fall through */
     case OMCR6:  tm ++;
+        /* fall through */
     case OMCR5:  tm ++;
+        /* fall through */
     case OMCR4:
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
@@ -235,19 +259,29 @@ static void pxa2xx_timer_write(void *opaque, hwaddr offset,
 
     switch (offset) {
     case OSMR3:  tm ++;
+        /* fall through */
     case OSMR2:  tm ++;
+        /* fall through */
     case OSMR1:  tm ++;
+        /* fall through */
     case OSMR0:
         s->timer[tm].value = value;
         pxa2xx_timer_update(s, qemu_get_clock_ns(vm_clock));
         break;
     case OSMR11: tm ++;
+        /* fall through */
     case OSMR10: tm ++;
+        /* fall through */
     case OSMR9:  tm ++;
+        /* fall through */
     case OSMR8:  tm ++;
+        /* fall through */
     case OSMR7:  tm ++;
+        /* fall through */
     case OSMR6:  tm ++;
+        /* fall through */
     case OSMR5:  tm ++;
+        /* fall through */
     case OSMR4:
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
@@ -261,12 +295,19 @@ static void pxa2xx_timer_write(void *opaque, hwaddr offset,
         pxa2xx_timer_update(s, s->lastload);
         break;
     case OSCR11: tm ++;
+        /* fall through */
     case OSCR10: tm ++;
+        /* fall through */
     case OSCR9:  tm ++;
+        /* fall through */
     case OSCR8:  tm ++;
+        /* fall through */
     case OSCR7:  tm ++;
+        /* fall through */
     case OSCR6:  tm ++;
+        /* fall through */
     case OSCR5:  tm ++;
+        /* fall through */
     case OSCR4:
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
@@ -291,8 +332,11 @@ static void pxa2xx_timer_write(void *opaque, hwaddr offset,
         s->reset3 = value;
         break;
     case OMCR7:  tm ++;
+        /* fall through */
     case OMCR6:  tm ++;
+        /* fall through */
     case OMCR5:  tm ++;
+        /* fall through */
     case OMCR4:
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
@@ -306,8 +350,11 @@ static void pxa2xx_timer_write(void *opaque, hwaddr offset,
         }
         break;
     case OMCR11: tm ++;
+        /* fall through */
     case OMCR10: tm ++;
+        /* fall through */
     case OMCR9:  tm ++;
+        /* fall through */
     case OMCR8:  tm += 4;
         if (!pxa2xx_timer_has_tm4(s))
             goto badreg;
-- 
1.7.9.5