[Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[Guenter Roeck]

Failing to set serial port clock frequencies is not fatal. Add a comment
into the code explaining this, and typecast the function to void to
silence static analyzers.

Cc: BALATON Zoltan <balaton@eik.bme.hu>
Cc: Pilippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/ppc/sam460ex.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 7eed2ec..b004cda 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -324,7 +324,10 @@ static int sam460ex_load_device_tree(hwaddr addr,
     /* set serial port clocks */
     offset = fdt_node_offset_by_compatible(fdt, -1, "ns16550");
     while (offset >= 0) {
-        fdt_setprop_cell(fdt, offset, "clock-frequency", UART_FREQ);
+        /* Failure to set serial port clocks is not fatal, so just ignore
+         * errors when trying to do so.
+         */
+        (void)fdt_setprop_cell(fdt, offset, "clock-frequency", UART_FREQ);
         offset = fdt_node_offset_by_compatible(fdt, offset, "ns16550");
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[Paolo Bonzini]

On 05/07/2018 15:51, Guenter Roeck wrote:
> +        /* Failure to set serial port clocks is not fatal, so just ignore
> +         * errors when trying to do so.
> +         */
> +        (void)fdt_setprop_cell(fdt, offset, "clock-frequency", UART_FREQ);
>          offset = fdt_node_offset_by_compatible(fdt, offset, "ns16550");

Ok, but why would it even fail?  Maybe it's not this case, but even if
it's not fatal for the OS, generating different device trees silently
seems like a recipe for Heisenbugs.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Thu, Jul 05, 2018 at 04:06:28PM +0200, Paolo Bonzini wrote:
> On 05/07/2018 15:51, Guenter Roeck wrote:
> > +        /* Failure to set serial port clocks is not fatal, so just ignore
> > +         * errors when trying to do so.
> > +         */
> > +        (void)fdt_setprop_cell(fdt, offset, "clock-frequency", UART_FREQ);
> >          offset = fdt_node_offset_by_compatible(fdt, offset, "ns16550");
> 
> Ok, but why would it even fail?  Maybe it's not this case, but even if
> it's not fatal for the OS, generating different device trees silently
> seems like a recipe for Heisenbugs.

Yes, quite.  Especially since the most likely errors I can see here
would actually indicate something has already gone horribly wrong with
the device tree construction, so a missing clock-frequency is the
least of our troubles.

I think using _FDT() here would be a better approach.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 662 bytes --]

On 06/07/2018 03:18, David Gibson wrote:
>> Ok, but why would it even fail?  Maybe it's not this case, but even if
>> it's not fatal for the OS, generating different device trees silently
>> seems like a recipe for Heisenbugs.
> Yes, quite.  Especially since the most likely errors I can see here
> would actually indicate something has already gone horribly wrong with
> the device tree construction, so a missing clock-frequency is the
> least of our troubles.
> 
> I think using _FDT() here would be a better approach.

Or qemu_fdt_setprop_cell, which is there exactly for this reason.
Volunteers needed to report it in checkpatch! :)

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[Guenter Roeck]

On Fri, Jul 06, 2018 at 02:41:18PM +0200, Paolo Bonzini wrote:
> On 06/07/2018 03:18, David Gibson wrote:
> >> Ok, but why would it even fail?  Maybe it's not this case, but even if
> >> it's not fatal for the OS, generating different device trees silently
> >> seems like a recipe for Heisenbugs.
> > Yes, quite.  Especially since the most likely errors I can see here
> > would actually indicate something has already gone horribly wrong with
> > the device tree construction, so a missing clock-frequency is the
> > least of our troubles.
> > 
> > I think using _FDT() here would be a better approach.
> 
> Or qemu_fdt_setprop_cell, which is there exactly for this reason.
> Volunteers needed to report it in checkpatch! :)
> 
Sure, except it uses different parameters. Why don't you come up with
an implementation that is acceptable to you ? I was asked earlier to add
the comment, which I did, only to now be told that it is insufficient.
This can go on forever. At this point, I'll be happy to send a revert
request.

Guenter

Re: [Qemu-devel] [PATCH] sam460ex: Add comment explaining ignored errors from fdt operations

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Sat, Jul 07, 2018 at 02:23:25PM -0700, Guenter Roeck wrote:
> On Fri, Jul 06, 2018 at 02:41:18PM +0200, Paolo Bonzini wrote:
> > On 06/07/2018 03:18, David Gibson wrote:
> > >> Ok, but why would it even fail?  Maybe it's not this case, but even if
> > >> it's not fatal for the OS, generating different device trees silently
> > >> seems like a recipe for Heisenbugs.
> > > Yes, quite.  Especially since the most likely errors I can see here
> > > would actually indicate something has already gone horribly wrong with
> > > the device tree construction, so a missing clock-frequency is the
> > > least of our troubles.
> > > 
> > > I think using _FDT() here would be a better approach.
> > 
> > Or qemu_fdt_setprop_cell, which is there exactly for this reason.
> > Volunteers needed to report it in checkpatch! :)
> > 
> Sure, except it uses different parameters. Why don't you come up with
> an implementation that is acceptable to you ? I was asked earlier to add
> the comment, which I did, only to now be told that it is insufficient.
> This can go on forever. At this point, I'll be happy to send a revert
> request.

Well, we asked for the comment because we were told that ignoring the
error was intentional.  When the comment was made, it became clear
that wasn't really the case.

Really, I should have caught this when I first merged the code,
though.  The error handling throughout this function is kind of bogus,
so I'm about to add my own patch to fix it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]