Re: [PATCH] soc/tegra: refactor soc_is_tegra()

[Frank Lee]

On Thu, Nov 22, 2018 at 6:49 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Thu, Nov 22, 2018 at 11:45 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >
> > On Wed, Nov 21, 2018 at 3:40 PM Yangtao Li <tiny.windzz@xxxxxxxxx> wrote:
> > >
> > > of_find_node_by_path() acquires a reference to the node returned by
> > > it and that reference needs to be dropped by its caller.soc_is_tegra()
> > > doesn't do that, so fix it.Call of_machine_is_compatible() to refactor
> > > soc_is_tegra() whcih automatically manages the reference count.
> > >
> > > Signed-off-by: Yangtao Li <tiny.windzz@xxxxxxxxx>
> > > ---
>
> Anders ran into a crash after this patch, on a non-tegra qemu platform:
>
> [    0.055907] ASID allocator initialised with 32768 entries
> [    0.063381] rcu: Hierarchical SRCU implementation.
> [    0.137238] Unable to handle kernel paging request at virtual
> address ffff00000935f0c0
> [    0.137274] Mem abort info:
> [    0.137291]   ESR = 0x96000007
> [    0.137320]   Exception class = DABT (current EL), IL = 32 bits
> [    0.137337]   SET = 0, FnV = 0
> [    0.137352]   EA = 0, S1PTW = 0
> [    0.137370] Data abort info:
> [    0.137387]   ISV = 0, ISS = 0x00000007
> [    0.137401]   CM = 0, WnR = 0
> [    0.137456] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
> [    0.137479] [ffff00000935f0c0] pgd=00000000bdfff003,
> pud=00000000bdffe003, pmd=00000000bdffa003, pte=0000000000000000
> [    0.137644] Internal error: Oops: 96000007 [#1] PREEMPT SMP
> [    0.137766] Modules linked in:
> [    0.137950] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 4.20.0-rc3-next-20181122-00006-g38d8a1f80349-dirty #2
> [    0.137975] Hardware name: linux,dummy-virt (DT)
> [    0.138071] pstate: 00000085 (nzcv daIf -PAN -UAO)
> [    0.138110] pc : __of_device_is_compatible+0x30/0x138
> [    0.138132] lr : of_device_is_compatible+0x40/0x68
> [    0.138147] sp : ffff00000804bc80
> [    0.138167] x29: ffff00000804bc80 x28: 0000000000000000
> [    0.138207] x27: 0000000000000000 x26: 0000000000000000
> [    0.138224] x25: ffff80007dfed2a8 x24: ffff000009301000
> [    0.138239] x23: 0000000000000000 x22: 0000000000000000
> [    0.138254] x21: ffff00000935f0c0 x20: ffff00000935f0c0
> [    0.138269] x19: 0000000000000000 x18: 0000000000000400
> [    0.138284] x17: 0000000000000000 x16: 0000000000000000
> [    0.138298] x15: 0000000000000400 x14: 0000000000000400
> [    0.138313] x13: 0000000000000000 x12: 0000000000000020
> [    0.138327] x11: 0000000000000008 x10: 0101010101010101
> [    0.138357] x9 : 6862726efffefeff x8 : 7f7f7f7f7f7f7f7f
> [    0.138373] x7 : fefefefefefeff2e x6 : 0080808080808080
> [    0.138387] x5 : 0000000000000002 x4 : 0000000000000001
> [    0.138402] x3 : 0000000000000000 x2 : 0000000000000000
> [    0.138416] x1 : ffff00000935f0c0 x0 : ffff80007dfed2a8
> [    0.138475] Process swapper/0 (pid: 1, stack limit = 0x(____ptrval____))
> [    0.138540] Call trace:
> [    0.138607]  __of_device_is_compatible+0x30/0x138
> [    0.138632]  of_device_is_compatible+0x40/0x68
> [    0.138654]  of_machine_is_compatible+0x34/0x68
> [    0.138672]  soc_is_tegra+0x2c/0x40
> [    0.138689]  tegra_flowctrl_init+0x28/0x108
> [    0.138706]  do_one_initcall+0x5c/0x178
> [    0.138722]  kernel_init_freeable+0xd0/0x240
> [    0.138741]  kernel_init+0x10/0x108
> [    0.138755]  ret_from_fork+0x10/0x18
> [    0.138913] Code: b4000861 f90013b5 aa0103f5 52800013 (39400021)
> [    0.139229] ---[ end trace d4d0fc77e9b04fa6 ]---
> [    0.139448] note: swapper/0[1] exited with preempt_count 1
> [    0.140598] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b
> [    0.140767] ---[ end Kernel panic - not syncing: Attempted to kill
> init! exitcode=0x0000000b ]---
>
> I'm not completely sure what's wrong with the patch, but I assume
> it never worked on non-tegra machines.
>
> > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> > > index cd8f41351add..0b40700b672a 100644
> > > --- a/drivers/soc/tegra/common.c
> > > +++ b/drivers/soc/tegra/common.c
> > > @@ -22,11 +22,13 @@ static const struct of_device_id tegra_machine_match[] = {
> > >
> > >  bool soc_is_tegra(void)
> > >  {
> > > -       struct device_node *root;
> > > +       struct of_device_id *match = tegra_machine_match;
> > >
> > > -       root = of_find_node_by_path("/");
> > > -       if (!root)
> > > -               return false;
> > > +       while(match->compatible){
> > > +               if(of_machine_is_compatible(match->compatible))
> > > +                       return true;
> > > +               match++;
> > > +       }
> > >
> > > -       return of_match_node(tegra_machine_match, root) != NULL;
> > > +       return false;
>
> I would also note that this is a rather inefficient way to check
> for a particular platform, as we to do the string search through
> the DT for each entry in the table now instead of doing it once.
>
>        Arnd
Hi Jon:

Do you think we should stick to the new way or go back to the way before?

Yours,
    Yangtao