Re: [PATCH-for-8.2?] hw/arm/fsl-imx: Do not ignore Error argument

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Both i.MX25 and i.MX6 SoC models ignore the Error argument when
> setting the PHY number. Pick &error_abort which is the error
> used by the i.MX7 SoC (see commit 1f7197deb0 "ability to change
> the FEC PHY on i.MX7 processor").
>
> Fixes: 74c1330582 ("ability to change the FEC PHY on i.MX25 processor")
> Fixes: a9c167a3c4 ("ability to change the FEC PHY on i.MX6 processor")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/fsl-imx25.c | 3 ++-
>  hw/arm/fsl-imx6.c  | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> index 24c4374590..9aabbf7f58 100644
> --- a/hw/arm/fsl-imx25.c
> +++ b/hw/arm/fsl-imx25.c
> @@ -169,7 +169,8 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
> **errp)
>                                              epit_table[i].irq));
>      }
>  
> -    object_property_set_uint(OBJECT(&s->fec), "phy-num", s->phy_num, &err);

This is actually worse than "ignore the Error argument".  If this fails,
we continue with @err not null.  If we actually reach the next use of
@err...

> +    object_property_set_uint(OBJECT(&s->fec), "phy-num", s->phy_num,
> +                             &error_abort);
>      qdev_set_nic_properties(DEVICE(&s->fec), &nd_table[0]);
>  
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->fec), errp)) {
           return;
       }
       sysbus_mmio_map(SYS_BUS_DEVICE(&s->fec), 0, FSL_IMX25_FEC_ADDR);
       sysbus_connect_irq(SYS_BUS_DEVICE(&s->fec), 0,
                          qdev_get_gpio_in(DEVICE(&s->avic), 
FSL_IMX25_FEC_IRQ));

       if (!sysbus_realize(SYS_BUS_DEVICE(&s->rngc), errp)) {
           return;
       }
       
       [...]

       /* initialize 2 x 16 KB ROM */

... here, we pass a non-null @err to memory_region_init_rom().  Any
error will trip error_setv()'s assertion.

       memory_region_init_rom(&s->rom[0], OBJECT(dev), "imx25.rom0",
                              FSL_IMX25_ROM0_SIZE, &err);
       if (err) {
           error_propagate(errp, err);
           return;
       }

This is an instance of an anti-pattern: passing &err or errp without
checking for failure.  Three possible fixes:

1. Check for failure.

2. Pass &error_abort instead.  This is appropriate for programming
   errors.

3. Pass NULL instead.  This is appropriate when errors don't matter.
   Which is rare.

You go with 2., which looks good to me.

> diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
> index 4fa7f0b95e..7dc42cbfe6 100644
> --- a/hw/arm/fsl-imx6.c
> +++ b/hw/arm/fsl-imx6.c
> @@ -379,7 +379,8 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
> **errp)
>                                              spi_table[i].irq));
>      }
>  
> -    object_property_set_uint(OBJECT(&s->eth), "phy-num", s->phy_num, &err);
> +    object_property_set_uint(OBJECT(&s->eth), "phy-num", s->phy_num,
> +                             &error_abort);
>      qdev_set_nic_properties(DEVICE(&s->eth), &nd_table[0]);
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->eth), errp)) {
>          return;

Same.

Suggest to clarify the commit message like this:

  Both i.MX25 and i.MX6 SoC pass &err to object_property_set_uint()
  without checking for failure.  Running into another failure will trip
  error_setv()'s assertion.

  Pass &error_abort instead, like the i.MX7 SoC does (see commit
  1f7197deb0 "ability to change the FEC PHY on i.MX7 processor").

With something like that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>