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
