[PATCH] soc/tegra: refactor soc_is_tegra()

[PATCH] soc/tegra: refactor soc_is_tegra()

[Yangtao Li]

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@gmail.com>
---
 drivers/soc/tegra/common.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

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;
 }
-- 
2.17.0

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

[Jon Hunter]

On 21/11/2018 14:12, Yangtao Li 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@gmail.com>
> ---
>  drivers/soc/tegra/common.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> 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;
>  }

Ugh ... sorry, I thought that of_machine_is_compatible() looped through
the matches. OK, let's stick with your initial fix.

Cheers
Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

On Wed, Nov 21, 2018 at 02:34:57PM +0000, Jon Hunter wrote:
> 
> On 21/11/2018 14:12, Yangtao Li 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@gmail.com>
> > ---
> >  drivers/soc/tegra/common.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > 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;
> >  }
> 
> Ugh ... sorry, I thought that of_machine_is_compatible() looped through
> the matches. OK, let's stick with your initial fix.

Actually I prefer this one. Even if it is slightly more verbose, I think
it's much clearer what's actually going on. Also this hides all of the
OF node reference counting in a core function, so it's worth the extra
line, in my opinion.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Frank Lee]

On Wed, Nov 21, 2018 at 10:43 PM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Wed, Nov 21, 2018 at 02:34:57PM +0000, Jon Hunter wrote:
> >
> > On 21/11/2018 14:12, Yangtao Li 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@gmail.com>
> > > ---
> > >  drivers/soc/tegra/common.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > 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;
> > >  }
> >
> > Ugh ... sorry, I thought that of_machine_is_compatible() looped through
> > the matches. OK, let's stick with your initial fix.
>
> Actually I prefer this one. Even if it is slightly more verbose, I think
> it's much clearer what's actually going on. Also this hides all of the
> OF node reference counting in a core function, so it's worth the extra
> line, in my opinion.
>
> Thierry
Hi Jon：

I like both, how aboout you?

Yours，
    Yangtao

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

[Jon Hunter]

On 21/11/2018 14:47, Frank Lee wrote:
> On Wed, Nov 21, 2018 at 10:43 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
>>
>> On Wed, Nov 21, 2018 at 02:34:57PM +0000, Jon Hunter wrote:
>>>
>>> On 21/11/2018 14:12, Yangtao Li 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@gmail.com>
>>>> ---
>>>>  drivers/soc/tegra/common.c | 12 +++++++-----
>>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> 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;
>>>>  }
>>>
>>> Ugh ... sorry, I thought that of_machine_is_compatible() looped through
>>> the matches. OK, let's stick with your initial fix.
>>
>> Actually I prefer this one. Even if it is slightly more verbose, I think
>> it's much clearer what's actually going on. Also this hides all of the
>> OF node reference counting in a core function, so it's worth the extra
>> line, in my opinion.
>>
>> Thierry
> Hi Jon：
> 
> I like both, how aboout you?

Yes fine with me too. However, looks like there is some formatting that
needs to be fixed up above. Please make sure you run checkpatch.pl on
your patches. Otherwise ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Thierry, pick up this version if you prefer.

Cheers
Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2566 bytes --]

On Wed, Nov 21, 2018 at 02:50:54PM +0000, Jon Hunter wrote:
> 
> On 21/11/2018 14:47, Frank Lee wrote:
> > On Wed, Nov 21, 2018 at 10:43 PM Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> >>
> >> On Wed, Nov 21, 2018 at 02:34:57PM +0000, Jon Hunter wrote:
> >>>
> >>> On 21/11/2018 14:12, Yangtao Li 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@gmail.com>
> >>>> ---
> >>>>  drivers/soc/tegra/common.c | 12 +++++++-----
> >>>>  1 file changed, 7 insertions(+), 5 deletions(-)
> >>>>
> >>>> 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;
> >>>>  }
> >>>
> >>> Ugh ... sorry, I thought that of_machine_is_compatible() looped through
> >>> the matches. OK, let's stick with your initial fix.
> >>
> >> Actually I prefer this one. Even if it is slightly more verbose, I think
> >> it's much clearer what's actually going on. Also this hides all of the
> >> OF node reference counting in a core function, so it's worth the extra
> >> line, in my opinion.
> >>
> >> Thierry
> > Hi Jon：
> > 
> > I like both, how aboout you?
> 
> Yes fine with me too. However, looks like there is some formatting that
> needs to be fixed up above. Please make sure you run checkpatch.pl on
> your patches. Otherwise ...
> 
> Acked-by: Jon Hunter <jonathanh@nvidia.com>
> 
> Thierry, pick up this version if you prefer.

Yeah, I noticed the formatting issues, but I can take care of them while
applying.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Frank Lee]

On Wed, Nov 21, 2018 at 10:56 PM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Wed, Nov 21, 2018 at 02:50:54PM +0000, Jon Hunter wrote:
> >
> > On 21/11/2018 14:47, Frank Lee wrote:
> > > On Wed, Nov 21, 2018 at 10:43 PM Thierry Reding
> > > <thierry.reding@gmail.com> wrote:
> > >>
> > >> On Wed, Nov 21, 2018 at 02:34:57PM +0000, Jon Hunter wrote:
> > >>>
> > >>> On 21/11/2018 14:12, Yangtao Li 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@gmail.com>
> > >>>> ---
> > >>>>  drivers/soc/tegra/common.c | 12 +++++++-----
> > >>>>  1 file changed, 7 insertions(+), 5 deletions(-)
> > >>>>
> > >>>> 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;
> > >>>>  }
> > >>>
> > >>> Ugh ... sorry, I thought that of_machine_is_compatible() looped through
> > >>> the matches. OK, let's stick with your initial fix.
> > >>
> > >> Actually I prefer this one. Even if it is slightly more verbose, I think
> > >> it's much clearer what's actually going on. Also this hides all of the
> > >> OF node reference counting in a core function, so it's worth the extra
> > >> line, in my opinion.
> > >>
> > >> Thierry
> > > Hi Jon：
> > >
> > > I like both, how aboout you?
> >
> > Yes fine with me too. However, looks like there is some formatting that
> > needs to be fixed up above. Please make sure you run checkpatch.pl on
> > your patches. Otherwise ...
> >
> > Acked-by: Jon Hunter <jonathanh@nvidia.com>
> >
> > Thierry, pick up this version if you prefer.
>
> Yeah, I noticed the formatting issues, but I can take care of them while
> applying.
>
> Thanks,
> Thierry
Thierry，

Thanks.  :-)

Yours,
    Yangtao

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Wed, Nov 21, 2018 at 09:12:04AM -0500, Yangtao Li 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@gmail.com>
> ---
>  drivers/soc/tegra/common.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Applied to for-4.21/soc, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Arnd Bergmann]

On Wed, Nov 21, 2018 at 3:40 PM Yangtao Li <tiny.windzz@gmail.com> 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@gmail.com>
> ---
>  drivers/soc/tegra/common.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> 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;
>  }
> --
> 2.17.0
>

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

[Arnd Bergmann]

On Thu, Nov 22, 2018 at 11:45 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Nov 21, 2018 at 3:40 PM Yangtao Li <tiny.windzz@gmail.com> 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@gmail.com>
> > ---

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

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

[Frank Lee]

On Thu, Nov 22, 2018 at 6:49 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Nov 22, 2018 at 11:45 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Wed, Nov 21, 2018 at 3:40 PM Yangtao Li <tiny.windzz@gmail.com> 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@gmail.com>
> > > ---
>
> 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