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);
+    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)) {
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;
-- 
2.41.0

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>

On Mon, 20 Nov 2023 at 11:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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>
> ---



Applied to target-arm.next, thanks.

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 20 Nov 2023 at 11:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> 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>
>> ---
>
>
>
> Applied to target-arm.next, thanks.

With or without my commit message clarification?

Hi Markus,

On 21/11/23 07:40, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Mon, 20 Nov 2023 at 11:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> 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>
>>> ---
>>
>>
>>
>> Applied to target-arm.next, thanks.
> 
> With or without my commit message clarification?

I didn't get your email on this patch, but per the other
ones on similar fixes:
https://lore.kernel.org/all/87cyw3mu4r.fsf@pond.sub.org/
https://lore.kernel.org/all/87il5vlemo.fsf@pond.sub.org/
I assume you want:

   Both i.MX25 and i.MX6 SoC models ignore the Error argument when
   setting the PHY number with object_property_set_uint(). If this
   @errp argument is set, its following use via sysbus_realize()
   might potentially triggers an assertion in error_setv().

   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").

If that is OK with you, Peter, do you mind updating the description?

Thanks both!

Phil.