Re: [RFC PATCH v2 58/78] hw/ppc: add fallthrough pseudo-keyword

[Cédric Le Goater]

On 10/13/23 10:32, Harsh Prateek Bora wrote:
> On 10/13/23 13:27, Emmanouil Pitsidianakis wrote:
> > In preparation of raising -Wimplicit-fallthrough to 5, replace all
> > fall-through comments with the fallthrough attribute pseudo-keyword.
> >
> > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
> One question below, may be Cedric knows better who introduced initial code.
>
> > ---
> >   hw/ppc/pnv_bmc.c      | 2 +-
> >   hw/ppc/spapr_events.c | 1 +
> >   2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
> > index 99f1e8d7f9..9bff7d03cb 100644
> > --- a/hw/ppc/pnv_bmc.c
> > +++ b/hw/ppc/pnv_bmc.c
> > @@ -172,69 +172,69 @@ static int hiomap_erase(PnvPnor *pnor, uint32_t offset, 
> > uint32_t size)
> >   static void hiomap_cmd(IPMIBmcSim *ibs, uint8_t *cmd, unsigned int cmd_len,
> >                          RspBuffer *rsp)
> >   {
> >       PnvPnor *pnor = PNV_PNOR(object_property_get_link(OBJECT(ibs), "pnor",
> >                                                         &error_abort));
> >       uint32_t pnor_size = pnor->size;
> >       uint32_t pnor_addr = PNOR_SPI_OFFSET;
> >       bool readonly = false;
> >       rsp_buffer_push(rsp, cmd[2]);
> >       rsp_buffer_push(rsp, cmd[3]);
> >       switch (cmd[2]) {
> >       case HIOMAP_C_MARK_DIRTY:
> >       case HIOMAP_C_FLUSH:
> >       case HIOMAP_C_ACK:
> >           break;
> >       case HIOMAP_C_ERASE:
> >           if (hiomap_erase(pnor, blocks_to_bytes(cmd[5] << 8 | cmd[4]),
> >                           blocks_to_bytes(cmd[7] << 8 | cmd[6]))) {
> >               rsp_buffer_set_error(rsp, IPMI_CC_UNSPECIFIED);
> >           }
> >           break;
> >       case HIOMAP_C_GET_INFO:
> >           rsp_buffer_push(rsp, 2);  /* Version 2 */
> >           rsp_buffer_push(rsp, BLOCK_SHIFT); /* block size */
> >           rsp_buffer_push(rsp, 0);  /* Timeout */
> >           rsp_buffer_push(rsp, 0);  /* Timeout */
> >           break;
> >       case HIOMAP_C_GET_FLASH_INFO:
> >           rsp_buffer_push(rsp, bytes_to_blocks(pnor_size) & 0xFF);
> >           rsp_buffer_push(rsp, bytes_to_blocks(pnor_size) >> 8);
> >           rsp_buffer_push(rsp, 0x01);  /* erase size */
> >           rsp_buffer_push(rsp, 0x00);  /* erase size */
> >           break;
> >       case HIOMAP_C_CREATE_READ_WINDOW:
> >           readonly = true;
> > -        /* Fall through */
> > +        fallthrough;
> >       case HIOMAP_C_CREATE_WRITE_WINDOW:
> >           memory_region_set_readonly(&pnor->mmio, readonly);
> >           memory_region_set_enabled(&pnor->mmio, true);
> >           rsp_buffer_push(rsp, bytes_to_blocks(pnor_addr) & 0xFF);
> >           rsp_buffer_push(rsp, bytes_to_blocks(pnor_addr) >> 8);
> >           rsp_buffer_push(rsp, bytes_to_blocks(pnor_size) & 0xFF);
> >           rsp_buffer_push(rsp, bytes_to_blocks(pnor_size) >> 8);
> >           rsp_buffer_push(rsp, 0x00); /* offset */
> >           rsp_buffer_push(rsp, 0x00); /* offset */
> >           break;
> >       case HIOMAP_C_CLOSE_WINDOW:
> >           memory_region_set_enabled(&pnor->mmio, false);
> >           break;
> >       case HIOMAP_C_DEVICE_NAME:
> >       case HIOMAP_C_RESET:
> >       case HIOMAP_C_LOCK:
>
> Do we need a break here ?
> Otherwise above 3 case statements doesnt add any value.

Indeed. These came from ca661fae81d3 ("ppc/pnv: Add HIOMAP commands").
The RESET command is implemented in skiboot. DEVICE_NAME and LOCK
seem not. Something to check.

Thanks,

C.

> >       default:
> >           qemu_log_mask(LOG_GUEST_ERROR, "HIOMAP: unknown command %02X\n", 
> > cmd[2]);
> >           break;
> >       }
> >   }
> >   #define HIOMAP   0x5a
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 4508e40814..9d51746daf 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -411,25 +411,26 @@ static const SpaprEventSource *
> >   rtas_event_log_to_source(SpaprMachineState *spapr, int log_type)
> >   {
> >       const SpaprEventSource *source;
> >       g_assert(spapr->event_sources);
> >       switch (log_type) {
> >       case RTAS_LOG_TYPE_HOTPLUG:
> >           source = spapr_event_sources_get_source(spapr->event_sources,
> >                                                   EVENT_CLASS_HOT_PLUG);
> >           if (spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT)) {
> >               g_assert(source->enabled);
> >               break;
> >           }
> >           /* fall through back to epow for legacy hotplug interrupt source */
> > +        fallthrough;
> >       case RTAS_LOG_TYPE_EPOW:
> >           source = spapr_event_sources_get_source(spapr->event_sources,
> >                                                   EVENT_CLASS_EPOW);
> >           break;
> >       default:
> >           source = NULL;
> >       }
> >       return source;
> >   }