diff mbox series

[v3,2/9] hw/{arm,ppc}: Resolve unreachable code

Message ID 20221016122737.93755-3-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series ppc/e500: Add support for two types of flash, cleanup | expand

Commit Message

Bernhard Beschow Oct. 16, 2022, 12:27 p.m. UTC 
pflash_cfi01_register() always returns with a non-NULL pointer (otherwise
it would crash internally). Therefore, the bodies of the if-statements
are unreachable.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/arm/gumstix.c     | 18 ++++++------------
 hw/arm/mainstone.c   | 13 +++++--------
 hw/arm/omap_sx1.c    | 22 ++++++++--------------
 hw/arm/versatilepb.c |  6 ++----
 hw/arm/z2.c          |  9 +++------
 hw/ppc/sam460ex.c    | 12 ++++--------
 6 files changed, 28 insertions(+), 52 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 17, 2022, 8:57 p.m. UTC | #1 
On 16/10/22 14:27, Bernhard Beschow wrote:
> pflash_cfi01_register() always returns with a non-NULL pointer (otherwise
> it would crash internally). Therefore, the bodies of the if-statements
> are unreachable.

This is true, pflash_cfi0X_register() use an hardcoded &error_fatal.

Shouldn't it be better to pass an Error* argument?

 From the pflash API perspective I don't see much value in
returning a PFlashCFI0X type instead of a simple DeviceState
(but this is another story...).

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/arm/gumstix.c     | 18 ++++++------------
>   hw/arm/mainstone.c   | 13 +++++--------
>   hw/arm/omap_sx1.c    | 22 ++++++++--------------
>   hw/arm/versatilepb.c |  6 ++----
>   hw/arm/z2.c          |  9 +++------
>   hw/ppc/sam460ex.c    | 12 ++++--------
>   6 files changed, 28 insertions(+), 52 deletions(-)
Bernhard Beschow Oct. 17, 2022, 9:48 p.m. UTC | #2 
Am 17. Oktober 2022 20:57:06 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 16/10/22 14:27, Bernhard Beschow wrote:
>> pflash_cfi01_register() always returns with a non-NULL pointer (otherwise
>> it would crash internally). Therefore, the bodies of the if-statements
>> are unreachable.
>
>This is true, pflash_cfi0X_register() use an hardcoded &error_fatal.
>
>Shouldn't it be better to pass an Error* argument?

You mean that the callers would pass &error_fatal instead, including the ones in this patch?

>
>From the pflash API perspective I don't see much value in
>returning a PFlashCFI0X type instead of a simple DeviceState
>(but this is another story...).

It comes in handy in the following patches when retrieving the memory region for memory_region_add_subregion() using pflash_cfi0X_get_memory(). What do you think about these next patches?

Furthermore, returning PFlashCFI0X can be downcasted which seems safer than upcasting from DeviceState.

Best regards,
Bernhard

>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/arm/gumstix.c     | 18 ++++++------------
>>   hw/arm/mainstone.c   | 13 +++++--------
>>   hw/arm/omap_sx1.c    | 22 ++++++++--------------
>>   hw/arm/versatilepb.c |  6 ++----
>>   hw/arm/z2.c          |  9 +++------
>>   hw/ppc/sam460ex.c    | 12 ++++--------
>>   6 files changed, 28 insertions(+), 52 deletions(-)
Peter Maydell Oct. 18, 2022, 9:25 a.m. UTC | #3 
On Mon, 17 Oct 2022 at 21:57, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 16/10/22 14:27, Bernhard Beschow wrote:
> > pflash_cfi01_register() always returns with a non-NULL pointer (otherwise
> > it would crash internally). Therefore, the bodies of the if-statements
> > are unreachable.
>
> This is true, pflash_cfi0X_register() use an hardcoded &error_fatal.
>
> Shouldn't it be better to pass an Error* argument?

The whole function is a legacy convenience-wrapper that's just
doing "create, configure, realize and wire up a device". New
code, and any callers that actually care about errors, should
just be directly creating and configuring the device anyway.
Almost all the callers are in old legacy board model code.

-- PMM
diff mbox series

Patch

diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index 3a4bc332c4..1296628ed9 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -65,12 +65,9 @@  static void connex_init(MachineState *machine)
         exit(1);
     }
 
-    if (!pflash_cfi01_register(0x00000000, "connext.rom", connex_rom,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               sector_len, 2, 0, 0, 0, 0, 0)) {
-        error_report("Error registering flash memory");
-        exit(1);
-    }
+    pflash_cfi01_register(0x00000000, "connext.rom", connex_rom,
+                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                          sector_len, 2, 0, 0, 0, 0, 0);
 
     /* Interrupt line of NIC is connected to GPIO line 36 */
     smc91c111_init(&nd_table[0], 0x04000300,
@@ -95,12 +92,9 @@  static void verdex_init(MachineState *machine)
         exit(1);
     }
 
-    if (!pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               sector_len, 2, 0, 0, 0, 0, 0)) {
-        error_report("Error registering flash memory");
-        exit(1);
-    }
+    pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom,
+                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                          sector_len, 2, 0, 0, 0, 0, 0);
 
     /* Interrupt line of NIC is connected to GPIO line 99 */
     smc91c111_init(&nd_table[0], 0x04000300,
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index 8454b65458..40f708f2d3 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -130,14 +130,11 @@  static void mainstone_common_init(MemoryRegion *address_space_mem,
     /* There are two 32MiB flash devices on the board */
     for (i = 0; i < 2; i ++) {
         dinfo = drive_get(IF_PFLASH, 0, i);
-        if (!pflash_cfi01_register(mainstone_flash_base[i],
-                                   i ? "mainstone.flash1" : "mainstone.flash0",
-                                   MAINSTONE_FLASH,
-                                   dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                                   sector_len, 4, 0, 0, 0, 0, 0)) {
-            error_report("Error registering flash memory");
-            exit(1);
-        }
+        pflash_cfi01_register(mainstone_flash_base[i],
+                              i ? "mainstone.flash1" : "mainstone.flash0",
+                              MAINSTONE_FLASH,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              sector_len, 4, 0, 0, 0, 0, 0);
     }
 
     mst_irq = sysbus_create_simple("mainstone-fpga", MST_FPGA_PHYS,
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index 57829b3744..820652265b 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -153,13 +153,10 @@  static void sx1_init(MachineState *machine, const int version)
 
     fl_idx = 0;
     if ((dinfo = drive_get(IF_PFLASH, 0, fl_idx)) != NULL) {
-        if (!pflash_cfi01_register(OMAP_CS0_BASE,
-                                   "omap_sx1.flash0-1", flash_size,
-                                   blk_by_legacy_dinfo(dinfo),
-                                   sector_size, 4, 0, 0, 0, 0, 0)) {
-            fprintf(stderr, "qemu: Error registering flash memory %d.\n",
-                           fl_idx);
-        }
+        pflash_cfi01_register(OMAP_CS0_BASE,
+                              "omap_sx1.flash0-1", flash_size,
+                              blk_by_legacy_dinfo(dinfo),
+                              sector_size, 4, 0, 0, 0, 0, 0);
         fl_idx++;
     }
 
@@ -175,13 +172,10 @@  static void sx1_init(MachineState *machine, const int version)
         memory_region_add_subregion(address_space,
                                 OMAP_CS1_BASE + flash1_size, &cs[1]);
 
-        if (!pflash_cfi01_register(OMAP_CS1_BASE,
-                                   "omap_sx1.flash1-1", flash1_size,
-                                   blk_by_legacy_dinfo(dinfo),
-                                   sector_size, 4, 0, 0, 0, 0, 0)) {
-            fprintf(stderr, "qemu: Error registering flash memory %d.\n",
-                           fl_idx);
-        }
+        pflash_cfi01_register(OMAP_CS1_BASE,
+                              "omap_sx1.flash1-1", flash1_size,
+                              blk_by_legacy_dinfo(dinfo),
+                              sector_size, 4, 0, 0, 0, 0, 0);
         fl_idx++;
     } else {
         memory_region_init_io(&cs[1], NULL, &static_ops, &cs1val,
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index ecc1f6cf74..43172d72ea 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -385,13 +385,11 @@  static void versatile_init(MachineState *machine, int board_id)
     /* 0x34000000 NOR Flash */
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    if (!pflash_cfi01_register(VERSATILE_FLASH_ADDR, "versatile.flash",
+    pflash_cfi01_register(VERSATILE_FLASH_ADDR, "versatile.flash",
                           VERSATILE_FLASH_SIZE,
                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                           VERSATILE_FLASH_SECT_SIZE,
-                          4, 0x0089, 0x0018, 0x0000, 0x0, 0)) {
-        fprintf(stderr, "qemu: Error registering flash memory.\n");
-    }
+                          4, 0x0089, 0x0018, 0x0000, 0x0, 0);
 
     versatile_binfo.ram_size = machine->ram_size;
     versatile_binfo.board_id = board_id;
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 9c1e876207..082ccc557e 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -311,12 +311,9 @@  static void z2_init(MachineState *machine)
     mpu = pxa270_init(address_space_mem, z2_binfo.ram_size, machine->cpu_type);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    if (!pflash_cfi01_register(Z2_FLASH_BASE, "z2.flash0", Z2_FLASH_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               sector_len, 4, 0, 0, 0, 0, 0)) {
-        error_report("Error registering flash memory");
-        exit(1);
-    }
+    pflash_cfi01_register(Z2_FLASH_BASE, "z2.flash0", Z2_FLASH_SIZE,
+                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                          sector_len, 4, 0, 0, 0, 0, 0);
 
     /* setup keypad */
     pxa27x_register_keypad(mpu->kp, map, 0x100);
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 850bb3b817..8089dd015b 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -111,14 +111,10 @@  static int sam460ex_load_uboot(void)
     DriveInfo *dinfo;
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    if (!pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
-                               "sam460ex.flash", FLASH_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1)) {
-        error_report("Error registering flash memory");
-        /* XXX: return an error instead? */
-        exit(1);
-    }
+    pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
+                          "sam460ex.flash", FLASH_SIZE,
+                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
 
     if (!dinfo) {
         /*error_report("No flash image given with the 'pflash' parameter,"