Use of of_parse_phandle()/of_node_put()

Use of of_parse_phandle()/of_node_put()

[Kumar, Anil]

[-- Attachment #1.1: Type: text/plain, Size: 939 bytes --]

Hi  Peter,

Just trying to understand.

In omap-twl4030.c file probe function :-

 dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
                if (!dai_node) {
                        dev_err(&pdev->dev, "McBSP node is not provided\n");
                        return -EINVAL;
                }

Here  "of_parse_phandle()"  is used to get  "of_device" node pointer.
of_parse_phandle() suggest to use of_node_put()  on it when done.

It looks when code request for an "of_device" node, kernel  maintains "refcount" for this.
It check "refcount"  before giving pointer of of_device node and  WARN_ON()
in case of refcount  > 0 and increase it on success.

Should this code need to use of_node_put() on the requested "of_device" when done
so that this can be get  again ?

I am taking reference of Linux-next kernel.

I'm sorry if this question is very vague or  missing something.

Thanks,
Anil







[-- Attachment #1.2: Type: text/html, Size: 2342 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Use of of_parse_phandle()/of_node_put()

[Peter Ujfalusi]

On 02/13/2013 07:04 AM, Kumar, Anil wrote:
> Hi  Peter,
>  
> Just trying to understand.
>  
> In omap-twl4030.c file probe function :-
>  
> dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
>                 if (!dai_node) {
>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>                         return -EINVAL;
>                 }
>  
> Here  “of_parse_phandle()”  is used to get  “of_device” node pointer.
> of_parse_phandle() suggest to use of_node_put()  on it when done.
>  
> It looks when code request for an “of_device” node, kernel  maintains
> “refcount” for this.
> It check “refcount”  before giving pointer of of_device node and  WARN_ON()
> in case of refcount  > 0 and increase it on success.
>  
> Should this code need to use of_node_put() on the requested “of_device” when done
> so that this can be get  again ?

Hrm, one thing or sure we should not call it of_node_put() while we have the
card loaded since the node is used runtime by the core.

However when we unload the machine driver it might be needed, but not sure
about this. None of the existing machine drivers doing it (tegra, samsung,
omap, etc).
But if it is needed it might be better to be done by the core?

>  
> I am taking reference of Linux-next kernel.
>  
> I'm sorry if this question is very vague or  missing something.
>  
> Thanks,
> Anil
>  
>  
>  
>  
>  
>  


-- 
Péter

Re: Use of of_parse_phandle()/of_node_put()

[Anil Kumar]

Hi,

On Wed, Feb 13, 2013 at 9:22 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 02/13/2013 07:04 AM, Kumar, Anil wrote:
>> Hi  Peter,
>>
>> Just trying to understand.
>>
>> In omap-twl4030.c file probe function :-
>>
>> dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
>>                 if (!dai_node) {
>>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>>                         return -EINVAL;
>>                 }
>>
>> Here  “of_parse_phandle()”  is used to get  “of_device” node pointer.
>> of_parse_phandle() suggest to use of_node_put()  on it when done.
>>
>> It looks when code request for an “of_device” node, kernel  maintains
>> “refcount” for this.
>> It check “refcount”  before giving pointer of of_device node and  WARN_ON()
>> in case of refcount  > 0 and increase it on success.
>>
>> Should this code need to use of_node_put() on the requested “of_device” when done
>> so that this can be get  again ?
>
> Hrm, one thing or sure we should not call it of_node_put() while we have the
> card loaded since the node is used runtime by the core.
>
> However when we unload the machine driver it might be needed, but not sure
> about this. None of the existing machine drivers doing it (tegra, samsung,
> omap, etc).
> But if it is needed it might be better to be done by the core?
>

It looks issue is different here.
I have done some testing here with patch[1]. In this patch i tried to
get same "of_device" node pointer
again and found refcount for this dt node is 1 [Result].

As of_parse_phandle() says it "returns the device_node pointer with
refcount incremented".
But why refcount  value is 1 [Result] again ?

-----------------------------8---------------------
Patch[1]:-
diff --git a/sound/soc/omap/omap-twl4030.c b/sound/soc/omap/omap-twl4030.c
index fd98509..0828a5c 100644
--- a/sound/soc/omap/omap-twl4030.c
+++ b/sound/soc/omap/omap-twl4030.c
@@ -297,6 +297,16 @@ static int omap_twl4030_probe(struct platform_device *pdev)
                        dev_err(&pdev->dev, "McBSP node is not provided\n");
                        return -EINVAL;
                }
+               printk(KERN_ERR"refcount 0x%x",
atomic_read(&dai_node->kref.refcount));
+
+               dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
+                if (!dai_node) {
+                        dev_err(&pdev->dev, "McBSP node is not provided\n");
+                        return -EINVAL;
+                }
+               printk(KERN_ERR"refcount 0x%x",
atomic_read(&dai_node->kref.refcount));
+
+
                omap_twl4030_dai_links[0].cpu_dai_name  = NULL;
                omap_twl4030_dai_links[0].cpu_of_node = dai_node;

[Result]:-
root@DevKit8000:/# insmod snd-soc-omap-twl4030.ko
[   95.718109] refcount 0x1
[   95.720611] refcount 0x1 [   95.818054] omap-twl4030 sound.20:
twl4030-hifi <-> 49022000.mcbsp mapping ok


Thanks,
Anil
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Use of of_parse_phandle()/of_node_put()

[Anil Kumar]

Hi Peter,

On Wed, Feb 13, 2013 at 10:10 PM, Anil Kumar <anilk4.v@gmail.com> wrote:
> Hi,
>
> On Wed, Feb 13, 2013 at 9:22 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> On 02/13/2013 07:04 AM, Kumar, Anil wrote:
>>> Hi  Peter,
>>>
>>> Just trying to understand.
>>>
>>> In omap-twl4030.c file probe function :-
>>>
>>> dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
>>>                 if (!dai_node) {
>>>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>>>                         return -EINVAL;
>>>                 }
>>>
>>> Here  “of_parse_phandle()”  is used to get  “of_device” node pointer.
>>> of_parse_phandle() suggest to use of_node_put()  on it when done.
>>>
>>> It looks when code request for an “of_device” node, kernel  maintains
>>> “refcount” for this.
>>> It check “refcount”  before giving pointer of of_device node and  WARN_ON()
>>> in case of refcount  > 0 and increase it on success.
>>>
>>> Should this code need to use of_node_put() on the requested “of_device” when done
>>> so that this can be get  again ?
>>
>> Hrm, one thing or sure we should not call it of_node_put() while we have the
>> card loaded since the node is used runtime by the core.
>>
>> However when we unload the machine driver it might be needed, but not sure
>> about this. None of the existing machine drivers doing it (tegra, samsung,
>> omap, etc).
>> But if it is needed it might be better to be done by the core?
>>

Sorry i forgot to mention.

Yes, I think soc core may have something like[1] to release cpu_of_node.

[1]
------------8---------------
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b7e84a7..9000f4a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1044,6 +1044,13 @@ static void soc_remove_dai_links(struct
snd_soc_card *card)
                        soc_remove_link_dais(card, dai, order);
        }

+       /* release cpu_of_node */
+       if(card->dai_link) {
+               int i;
+               for(i = 0; i < card->num_links; i++)
+                       of_node_put(card->dai_link[i].cpu_of_node);
+       }
+

Thanks,
Anil

>
> It looks issue is different here.
> I have done some testing here with patch[1]. In this patch i tried to
> get same "of_device" node pointer
> again and found refcount for this dt node is 1 [Result].
>
> As of_parse_phandle() says it "returns the device_node pointer with
> refcount incremented".
> But why refcount  value is 1 [Result] again ?
>
> -----------------------------8---------------------
> Patch[1]:-
> diff --git a/sound/soc/omap/omap-twl4030.c b/sound/soc/omap/omap-twl4030.c
> index fd98509..0828a5c 100644
> --- a/sound/soc/omap/omap-twl4030.c
> +++ b/sound/soc/omap/omap-twl4030.c
> @@ -297,6 +297,16 @@ static int omap_twl4030_probe(struct platform_device *pdev)
>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>                         return -EINVAL;
>                 }
> +               printk(KERN_ERR"refcount 0x%x",
> atomic_read(&dai_node->kref.refcount));
> +
> +               dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
> +                if (!dai_node) {
> +                        dev_err(&pdev->dev, "McBSP node is not provided\n");
> +                        return -EINVAL;
> +                }
> +               printk(KERN_ERR"refcount 0x%x",
> atomic_read(&dai_node->kref.refcount));
> +
> +
>                 omap_twl4030_dai_links[0].cpu_dai_name  = NULL;
>                 omap_twl4030_dai_links[0].cpu_of_node = dai_node;
>
> [Result]:-
> root@DevKit8000:/# insmod snd-soc-omap-twl4030.ko
> [   95.718109] refcount 0x1
> [   95.720611] refcount 0x1 [   95.818054] omap-twl4030 sound.20:
> twl4030-hifi <-> 49022000.mcbsp mapping ok
>
>
> Thanks,
> Anil
> [...]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Use of of_parse_phandle()/of_node_put()

[Peter Ujfalusi]

On 02/13/2013 05:40 PM, Anil Kumar wrote:
> It looks issue is different here.
> I have done some testing here with patch[1]. In this patch i tried to
> get same "of_device" node pointer
> again and found refcount for this dt node is 1 [Result].
> 
> As of_parse_phandle() says it "returns the device_node pointer with
> refcount incremented".
> But why refcount  value is 1 [Result] again ?

Documentation/kref.txt

> 
> -----------------------------8---------------------
> Patch[1]:-
> diff --git a/sound/soc/omap/omap-twl4030.c b/sound/soc/omap/omap-twl4030.c
> index fd98509..0828a5c 100644
> --- a/sound/soc/omap/omap-twl4030.c
> +++ b/sound/soc/omap/omap-twl4030.c
> @@ -297,6 +297,16 @@ static int omap_twl4030_probe(struct platform_device *pdev)
>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>                         return -EINVAL;
>                 }
> +               printk(KERN_ERR"refcount 0x%x",
> atomic_read(&dai_node->kref.refcount));
> +
> +               dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
> +                if (!dai_node) {
> +                        dev_err(&pdev->dev, "McBSP node is not provided\n");
> +                        return -EINVAL;
> +                }
> +               printk(KERN_ERR"refcount 0x%x",
> atomic_read(&dai_node->kref.refcount));
> +
> +
>                 omap_twl4030_dai_links[0].cpu_dai_name  = NULL;
>                 omap_twl4030_dai_links[0].cpu_of_node = dai_node;
> 
> [Result]:-
> root@DevKit8000:/# insmod snd-soc-omap-twl4030.ko
> [   95.718109] refcount 0x1
> [   95.720611] refcount 0x1 [   95.818054] omap-twl4030 sound.20:
> twl4030-hifi <-> 49022000.mcbsp mapping ok
> 
> 
> Thanks,
> Anil
> [...]
> 


-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Use of of_parse_phandle()/of_node_put()

[Peter Ujfalusi]

On 02/13/2013 06:02 PM, Anil Kumar wrote:
> ------------8---------------
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index b7e84a7..9000f4a 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1044,6 +1044,13 @@ static void soc_remove_dai_links(struct
> snd_soc_card *card)
>                         soc_remove_link_dais(card, dai, order);
>         }
> 
> +       /* release cpu_of_node */
> +       if(card->dai_link) {
> +               int i;
> +               for(i = 0; i < card->num_links; i++)
> +                       of_node_put(card->dai_link[i].cpu_of_node);

and the same for codec_of_node and platform_of_node for that matter.

Mark: what do you think? Does it make sense to do this in the core or should
we let the machine drivers to take care of this?

> +       }
> +
> 

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html