Hi Samuel,
On Thu, 17 Nov 2022 at 21:00, Samuel Holland <sam...@sholland.org> wrote:
>
> Hi Simon,
>
> On 11/7/22 17:35, Simon Glass wrote:
> > Hi Samuel,
> >
> > On Mon, 31 Oct 2022 at 13:27, Simon Glass <s...@chromium.org> wrote:
> >>
> >> On Sun, 30 Oct 2022 at 21:41, Samuel Holland <sam...@sholland.org> wrote:
> >>>
> >>> reg must contain enough cells for the entire next address/size pair
> >>> after skipping `index` pairs. The previous code allows an out-of-bounds
> >>> read when na + ns > 1.
> >>>
> >>> Fixes: 69b41388ba45 ("dm: core: Add a new api to get indexed device
> >>> address")
> >>> Signed-off-by: Samuel Holland <sam...@sholland.org>
> >>> ---
> >>>
> >>> drivers/core/fdtaddr.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Reviewed-by: Simon Glass <s...@chromium.org>
> >
> > Somehow this break PPC in CI:
> >
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/524776
> >
> > Can you please take a look?
>
> The cause is the call to lists_bind_fdt() inside serial_check_stdout(),
> which sets the UART's parent device to gd->dm_root, when the real parent
> should be a simple-bus node with a ranges property.
>
> Then when we go to probe the UART, devfdt_get_addr_index() does:
>
> int parent = dev_of_offset(dev->parent);
> // ...
> na = fdt_address_cells(gd->fdt_blob, parent);
>
> So devfdt_get_addr_index() sees the wrong number of address/size cells
> (2 instead of 1) and the check fails. This only worked previously
> because the check in devfdt_get_addr_index() would always succeed for
> index == 0.
>
> This patch fixes things, by setting the UART's parent device correctly:
>
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -623,9 +623,7 @@
> .priv_auto = sizeof(struct ns16550),
> .probe = ns16550_serial_probe,
> .ops = &ns16550_serial_ops,
> -#if !CONFIG_IS_ENABLED(OF_CONTROL)
> .flags = DM_FLAG_PRE_RELOC,
> -#endif
We should put the dm tag in the device tree node instead.
> };
>
> DM_DRIVER_ALIAS(ns16550_serial, ti_da830_uart)
>
> Or maybe devfdt_get_addr_index() should be looking at the FDT node
> parent, instead of the DM parent. This also fixes things:
>
> --- a/drivers/core/fdtaddr.c
> +++ b/drivers/core/fdtaddr.c
> @@ -22,7 +22,7 @@
> {
> #if CONFIG_IS_ENABLED(OF_REAL)
> int offset = dev_of_offset(dev);
> - int parent = dev_of_offset(dev->parent);
> + int parent = fdt_parent_offset(gd->fdt_blob, offset);
That is slow, best avoided.
> fdt_addr_t addr;
>
> if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
Regards,
Simon
