All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] soc/tegra: refactor soc_is_tegra()
@ 2018-11-21 14:12 Yangtao Li
  2018-11-21 14:34   ` Jon Hunter
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Yangtao Li @ 2018-11-21 14:12 UTC (permalink / raw)
  To: thierry.reding, jonathanh; +Cc: linux-tegra, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:12 [PATCH] soc/tegra: refactor soc_is_tegra() Yangtao Li
@ 2018-11-21 14:34   ` Jon Hunter
  2018-11-21 15:28 ` Thierry Reding
  2018-11-22 10:45 ` Arnd Bergmann
  2 siblings, 0 replies; 13+ messages in thread
From: Jon Hunter @ 2018-11-21 14:34 UTC (permalink / raw)
  To: Yangtao Li, thierry.reding; +Cc: linux-tegra, linux-kernel


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
@ 2018-11-21 14:34   ` Jon Hunter
  0 siblings, 0 replies; 13+ messages in thread
From: Jon Hunter @ 2018-11-21 14:34 UTC (permalink / raw)
  To: Yangtao Li, thierry.reding; +Cc: linux-tegra, linux-kernel


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:34   ` Jon Hunter
  (?)
@ 2018-11-21 14:43   ` Thierry Reding
  2018-11-21 14:47     ` Frank Lee
  -1 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2018-11-21 14:43 UTC (permalink / raw)
  To: Jon Hunter; +Cc: Yangtao Li, linux-tegra, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:43   ` Thierry Reding
@ 2018-11-21 14:47     ` Frank Lee
  2018-11-21 14:50         ` Jon Hunter
  0 siblings, 1 reply; 13+ messages in thread
From: Frank Lee @ 2018-11-21 14:47 UTC (permalink / raw)
  To: Thierry Reding; +Cc: jonathanh, linux-tegra, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:47     ` Frank Lee
@ 2018-11-21 14:50         ` Jon Hunter
  0 siblings, 0 replies; 13+ messages in thread
From: Jon Hunter @ 2018-11-21 14:50 UTC (permalink / raw)
  To: Frank Lee, Thierry Reding; +Cc: linux-tegra, linux-kernel


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
@ 2018-11-21 14:50         ` Jon Hunter
  0 siblings, 0 replies; 13+ messages in thread
From: Jon Hunter @ 2018-11-21 14:50 UTC (permalink / raw)
  To: Frank Lee, Thierry Reding; +Cc: linux-tegra, linux-kernel


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:50         ` Jon Hunter
  (?)
@ 2018-11-21 14:56         ` Thierry Reding
  2018-11-21 14:57           ` Frank Lee
  -1 siblings, 1 reply; 13+ messages in thread
From: Thierry Reding @ 2018-11-21 14:56 UTC (permalink / raw)
  To: Jon Hunter; +Cc: Frank Lee, linux-tegra, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:56         ` Thierry Reding
@ 2018-11-21 14:57           ` Frank Lee
  0 siblings, 0 replies; 13+ messages in thread
From: Frank Lee @ 2018-11-21 14:57 UTC (permalink / raw)
  To: Thierry Reding; +Cc: jonathanh, linux-tegra, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:12 [PATCH] soc/tegra: refactor soc_is_tegra() Yangtao Li
  2018-11-21 14:34   ` Jon Hunter
@ 2018-11-21 15:28 ` Thierry Reding
  2018-11-22 10:45 ` Arnd Bergmann
  2 siblings, 0 replies; 13+ messages in thread
From: Thierry Reding @ 2018-11-21 15:28 UTC (permalink / raw)
  To: Yangtao Li; +Cc: jonathanh, linux-tegra, linux-kernel

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-21 14:12 [PATCH] soc/tegra: refactor soc_is_tegra() Yangtao Li
  2018-11-21 14:34   ` Jon Hunter
  2018-11-21 15:28 ` Thierry Reding
@ 2018-11-22 10:45 ` Arnd Bergmann
  2018-11-22 10:49   ` Arnd Bergmann
  2 siblings, 1 reply; 13+ messages in thread
From: Arnd Bergmann @ 2018-11-22 10:45 UTC (permalink / raw)
  To: tiny.windzz
  Cc: Thierry Reding, Jonathan Hunter,
	open list:TEGRA ARCHITECTURE SUPPORT, Linux Kernel Mailing List,
	Anders Roxell

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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-22 10:45 ` Arnd Bergmann
@ 2018-11-22 10:49   ` Arnd Bergmann
  2018-11-22 11:02     ` Frank Lee
  0 siblings, 1 reply; 13+ messages in thread
From: Arnd Bergmann @ 2018-11-22 10:49 UTC (permalink / raw)
  To: tiny.windzz
  Cc: Thierry Reding, Jonathan Hunter,
	open list:TEGRA ARCHITECTURE SUPPORT, Linux Kernel Mailing List,
	Anders Roxell

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] soc/tegra: refactor soc_is_tegra()
  2018-11-22 10:49   ` Arnd Bergmann
@ 2018-11-22 11:02     ` Frank Lee
  0 siblings, 0 replies; 13+ messages in thread
From: Frank Lee @ 2018-11-22 11:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Thierry Reding, jonathanh, linux-tegra, linux-kernel, anders.roxell

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-11-22 11:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 14:12 [PATCH] soc/tegra: refactor soc_is_tegra() Yangtao Li
2018-11-21 14:34 ` Jon Hunter
2018-11-21 14:34   ` Jon Hunter
2018-11-21 14:43   ` Thierry Reding
2018-11-21 14:47     ` Frank Lee
2018-11-21 14:50       ` Jon Hunter
2018-11-21 14:50         ` Jon Hunter
2018-11-21 14:56         ` Thierry Reding
2018-11-21 14:57           ` Frank Lee
2018-11-21 15:28 ` Thierry Reding
2018-11-22 10:45 ` Arnd Bergmann
2018-11-22 10:49   ` Arnd Bergmann
2018-11-22 11:02     ` Frank Lee

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.