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

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

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.

>       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;
>   }