| Message ID | 1530798684-23009-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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.
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"); }
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(-)