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

[Peter Maydell]

On Mon, 17 Oct 2022 at 21:57, Philippe Mathieu-Daudé  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 _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

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

[Philippe Mathieu-Daudé]

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 _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 
---
  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(-)

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

[Bernhard Beschow]

Am 17. Oktober 2022 20:57:06 UTC schrieb "Philippe Mathieu-Daudé" 
:
>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 _fatal.
>
>Shouldn't it be better to pass an Error* argument?

You mean that the callers would pass _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 
>> ---
>>   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(-)

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

[Bernhard Beschow]

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 
---
 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(-)

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(0x, "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(0x, "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(_table[0], 0x04000300,
@@ -95,12 +92,9 @@ static void verdex_init(MachineState *machine)
 exit(1);
 }
 
-if (!pflash_cfi01_register(0x, "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(0x, "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(_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, [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 {