Re: [PATCH 1/4] clk: ti: Handle possible address in the node name

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Tony,

On Tue, Feb 13, 2024 at 11:59 AM Tony Lindgren <tony@atomide.com> wrote:
> In order to use #address-cells = <1> and start making use of the
> standard reg property, let's prepare things to ignore the possible
> address in the clock node name.
>
> Unless the clock-output-names property is used, the legacy clocks still
> fall back to matching the clock data based on the node name.
>
> We use cleanup.h to simplify the return path for freeing tmp.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Thanks for your patch, which is now commit 3516338543cafb65 ("clk: ti:
Handle possible address in the node name") in v6.9-rc1.
This causes an early boot crash on BeagleBone Black:

    ti_dt_clocks_register: failed to lookup clock node
clk-24mhz-clkctrl:0000:0, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
clk-24mhz-clkctrl:0000:0, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:30, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:19, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4-wkup-clkctrl:0008:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4ls-clkctrl:0074:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4ls-clkctrl:0078:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4ls-clkctrl:007c:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:27, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:22, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:24, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:20, ret=-517
    8<--- cut here ---
    Unable to handle kernel paging request at virtual address fffffffe when read
    [fffffffe] *pgd=9fdf6841, *pte=00000000, *ppte=00000000
    Internal error: Oops: 27 [#1] SMP ARM
    CPU: 0 PID: 0 Comm: swapper/0 Not tainted
6.8.0-rc1-boneblack-00001-g3516338543ca #119
    Hardware name: Generic AM33XX (Flattened Device Tree)
    PC is at clk_set_parent+0x2c/0x6c
    LR is at __lock_acquire+0x3f8/0x29d4
    pc : [<c06ecd4c>]    lr : [<c01b2b14>]    psr: a0000093
    sp : c1001fb0  ip : 00000000  fp : 00000000
    r10: c0f5da88  r9 : 00000000  r8 : 00000078
    r7 : ffffffff  r6 : c11ba000  r5 : fffffffe  r4 : c20a0700
    r3 : 00000000  r2 : c100c580  r1 : 00000001  r0 : c209ac00
    Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
    Control: 10c5387d  Table: 80004019  DAC: 00000051
    Register r0 information: slab kmalloc-192 start c209ac00 pointer
offset 0 size 192
    Register r1 information: non-paged memory
    Register r2 information: non-slab/vmalloc memory
    Register r3 information: NULL pointer
    Register r4 information: slab kmalloc-64 start c20a0700 pointer
offset 0 size 64
    Register r5 information: non-paged memory
    Register r6 information: non-slab/vmalloc memory
    Register r7 information: non-paged memory
    Register r8 information: non-paged memory
    Register r9 information: NULL pointer
    Register r10 information: non-slab/vmalloc memory
    Register r11 information: NULL pointer
    Register r12 information: NULL pointer
    Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
    Stack: (0xc1001fb0 to 0xc1002000)
    1fa0:                                     c20a0700 c10093c0
c11ba000 c0f2ebdc
    1fc0: dfdffc40 c0f11380 dfdffc40 c0f01074 ffffffff ffffffff
00000000 c0f006f0
    1fe0: 00000000 c0f5da88 00000000 00000e05 00000000 00000000
00000000 00000000
     clk_set_parent from am33xx_dt_clk_init+0x84/0xa4
     am33xx_dt_clk_init from omap_init_time_of+0x8/0x10
     omap_init_time_of from start_kernel+0x430/0x638
     start_kernel from 0x0
    Code: e3530000 1a00000e e3550000 e5940000 (15955000)
    ---[ end trace 0000000000000000 ]---
    Kernel panic - not syncing: Attempted to kill the idle task!
    ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---

Reverting the commit on top of a recent v6.11-rcX-based tree fixes
the issue.

BTW, bisecting this took a while as:
  1. The OMAP serial driver locks up when booted with "earlycon
     keep_bootcon",
  2. The TI SYSC sometimes crashes during early boot, too:

    Unhandled fault: external abort on non-linefetch (0x1008) at 0xe036d010
    [e036d010] *pgd=8276e811, *pte=47400653, *ppte=47400453
    Internal error: : 1008 [#1] SMP ARM
    Modules linked in:
    CPU: 0 PID: 33 Comm: kworker/u4:3 Not tainted
6.8.0-boneblack-05567-gaa7d6513d68b #78
    Hardware name: Generic AM33XX (Flattened Device Tree)
    Workqueue: events_unbound deferred_probe_work_func
    PC is at sysc_reset+0x118/0x210
    LR is at sysc_probe+0xe08/0x1440
    pc : [<c06d0ba8>]    lr : [<c06d1cd8>]    psr: 60000013

> --- a/drivers/clk/ti/clk.c
> +++ b/drivers/clk/ti/clk.c
> @@ -7,6 +7,7 @@
>   * Tero Kristo <t-kristo@ti.com>
>   */
>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -114,20 +115,26 @@ int ti_clk_setup_ll_ops(struct ti_clk_ll_ops *ops)
>
>  /*
>   * Eventually we could standardize to using '_' for clk-*.c files to follow the
> - * TRM naming and leave out the tmp name here.
> + * TRM naming.
>   */
>  static struct device_node *ti_find_clock_provider(struct device_node *from,
>                                                   const char *name)
>  {
> +       char *tmp __free(kfree) = NULL;
>         struct device_node *np;
>         bool found = false;
>         const char *n;
> -       char *tmp;
> +       char *p;
>
>         tmp = kstrdup_and_replace(name, '-', '_', GFP_KERNEL);
>         if (!tmp)
>                 return NULL;
>
> +       /* Ignore a possible address for the node name */
> +       p = strchr(tmp, '@');
> +       if (p)
> +               *p = '\0';
> +
>         /* Node named "clock" with "clock-output-names" */
>         for_each_of_allnodes_from(from, np) {
>                 if (of_property_read_string_index(np, "clock-output-names",
> @@ -140,7 +147,6 @@ static struct device_node *ti_find_clock_provider(struct device_node *from,
>                         break;
>                 }
>         }
> -       kfree(tmp);
>
>         if (found) {
>                 of_node_put(from);
> @@ -148,7 +154,7 @@ static struct device_node *ti_find_clock_provider(struct device_node *from,
>         }
>
>         /* Fall back to using old node name base provider name */
> -       return of_find_node_by_name(from, name);
> +       return of_find_node_by_name(from, tmp);
>  }
>
>  /**

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds