[PATCH] ASoC: simple-card: fixup refcount_t underflow

[PATCH] ASoC: simple-card: fixup refcount_t underflow

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
merged simple-card and simple-scu-card. Then it had refcount
underflow bug. This patch fixup it.
We will get below error without this patch.

	OF: ERROR: Bad of_node_put() on /sound
	CPU: 3 PID: 237 Comm: kworker/3:1 Not tainted 5.0.0-rc6+ #1514
	Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
	Workqueue: events deferred_probe_work_func
	Call trace:
	 dump_backtrace+0x0/0x150
	 show_stack+0x24/0x30
	 dump_stack+0xb0/0xec
	 of_node_release+0xd0/0xd8
	 kobject_put+0x74/0xe8
	 of_node_put+0x24/0x30
	 __of_get_next_child+0x50/0x70
	 of_get_next_child+0x40/0x68
	 asoc_simple_card_probe+0x604/0x730
	 platform_drv_probe+0x58/0xa8
	 ...
Reported-by: Vicente Bergas <vicencb@gmail.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
Vicente, can you please test this patch ?

 sound/soc/generic/simple-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 37e001c..3fe3441 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -462,7 +462,7 @@ static int asoc_simple_card_parse_of(struct simple_card_data *priv)
 	conf_idx	= 0;
 	node = of_get_child_by_name(top, PREFIX "dai-link");
 	if (!node) {
-		node = dev->of_node;
+		node = of_node_get(top);
 		loop = 0;
 	}
 
-- 
2.7.4

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Daniel Baluta]

Hi,

This patch doesn't seem to apply on Mark's sound/for-next branch.

What is the tree you based this on?

thanks,
Daniel.
On Fri, Feb 15, 2019 at 8:43 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
> merged simple-card and simple-scu-card. Then it had refcount
> underflow bug. This patch fixup it.
> We will get below error without this patch.
>
>         OF: ERROR: Bad of_node_put() on /sound
>         CPU: 3 PID: 237 Comm: kworker/3:1 Not tainted 5.0.0-rc6+ #1514
>         Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
>         Workqueue: events deferred_probe_work_func
>         Call trace:
>          dump_backtrace+0x0/0x150
>          show_stack+0x24/0x30
>          dump_stack+0xb0/0xec
>          of_node_release+0xd0/0xd8
>          kobject_put+0x74/0xe8
>          of_node_put+0x24/0x30
>          __of_get_next_child+0x50/0x70
>          of_get_next_child+0x40/0x68
>          asoc_simple_card_probe+0x604/0x730
>          platform_drv_probe+0x58/0xa8
>          ...
> Reported-by: Vicente Bergas <vicencb@gmail.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> Vicente, can you please test this patch ?
>
>  sound/soc/generic/simple-card.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 37e001c..3fe3441 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -462,7 +462,7 @@ static int asoc_simple_card_parse_of(struct simple_card_data *priv)
>         conf_idx        = 0;
>         node = of_get_child_by_name(top, PREFIX "dai-link");
>         if (!node) {
> -               node = dev->of_node;
> +               node = of_node_get(top);
>                 loop = 0;
>         }
>
> --
> 2.7.4
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Daniel Baluta]

Ok, I think you are using an older code base.

Here is the patch that works for me:

--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -445,7 +445,7 @@ static int simple_for_each_link(struct simple_priv *priv,
        /* Check if it has dai-link */
        node = of_get_child_by_name(top, PREFIX "dai-link");
        if (!node) {
-               node = top;
+               node = of_node_get(top);
                is_top = 1;
        }

Not sure if is correct, though.

On Fri, Feb 15, 2019 at 3:57 PM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> Hi,
>
> This patch doesn't seem to apply on Mark's sound/for-next branch.
>
> What is the tree you based this on?
>
> thanks,
> Daniel.
> On Fri, Feb 15, 2019 at 8:43 AM Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> >
> >
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
> > merged simple-card and simple-scu-card. Then it had refcount
> > underflow bug. This patch fixup it.
> > We will get below error without this patch.
> >
> >         OF: ERROR: Bad of_node_put() on /sound
> >         CPU: 3 PID: 237 Comm: kworker/3:1 Not tainted 5.0.0-rc6+ #1514
> >         Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
> >         Workqueue: events deferred_probe_work_func
> >         Call trace:
> >          dump_backtrace+0x0/0x150
> >          show_stack+0x24/0x30
> >          dump_stack+0xb0/0xec
> >          of_node_release+0xd0/0xd8
> >          kobject_put+0x74/0xe8
> >          of_node_put+0x24/0x30
> >          __of_get_next_child+0x50/0x70
> >          of_get_next_child+0x40/0x68
> >          asoc_simple_card_probe+0x604/0x730
> >          platform_drv_probe+0x58/0xa8
> >          ...
> > Reported-by: Vicente Bergas <vicencb@gmail.com>
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> > Vicente, can you please test this patch ?
> >
> >  sound/soc/generic/simple-card.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> > index 37e001c..3fe3441 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -462,7 +462,7 @@ static int asoc_simple_card_parse_of(struct simple_card_data *priv)
> >         conf_idx        = 0;
> >         node = of_get_child_by_name(top, PREFIX "dai-link");
> >         if (!node) {
> > -               node = dev->of_node;
> > +               node = of_node_get(top);
> >                 loop = 0;
> >         }
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Mark Brown]

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

On Fri, Feb 15, 2019 at 06:48:26PM +0200, Daniel Baluta wrote:

> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -445,7 +445,7 @@ static int simple_for_each_link(struct simple_priv *priv,
>         /* Check if it has dai-link */
>         node = of_get_child_by_name(top, PREFIX "dai-link");
>         if (!node) {
> -               node = top;
> +               node = of_node_get(top);
>                 is_top = 1;
>         }
> 
> Not sure if is correct, though.

That looks correct to me, of_get_child_by_name() takes a reference we'll
need to drop later so when we substitute in top we need to take a
reference as well as just assigning.  Can you or someone put together a
proper patch please?

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

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

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Mark Brown]

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

On Fri, Feb 15, 2019 at 06:21:04PM +0100, Takashi Iwai wrote:

> Actually it's not all.  As mentioned in my earlier post, there are
> more of refcount unbalance in the code.  Lots of them, really.

> Below is a patch I've fixed quickly over the whole scanning the sound
> tree.  Some might be incorrect, so more detailed review for each piece
> of change is needed...

Extra references aren't such a problem for practical systems since we
don't actually ever free any of the DT (there's no mainline way to use
overlays yet...), that's how none of this stuff ever gets noticed.  I'm
not surprised there's loads of missing frees TBH.  In contrast if we've
got extra frees they will get noticed and cause problems.

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

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

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Daniel Baluta]

On Fri, Feb 15, 2019 at 8:43 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Fri, 15 Feb 2019 18:31:08 +0100,
> Mark Brown wrote:
> >
> > On Fri, Feb 15, 2019 at 06:21:04PM +0100, Takashi Iwai wrote:
> >
> > > Actually it's not all.  As mentioned in my earlier post, there are
> > > more of refcount unbalance in the code.  Lots of them, really.
> >
> > > Below is a patch I've fixed quickly over the whole scanning the sound
> > > tree.  Some might be incorrect, so more detailed review for each piece
> > > of change is needed...
> >
> > Extra references aren't such a problem for practical systems since we
> > don't actually ever free any of the DT (there's no mainline way to use
> > overlays yet...), that's how none of this stuff ever gets noticed.  I'm
> > not surprised there's loads of missing frees TBH.  In contrast if we've
> > got extra frees they will get noticed and cause problems.
>
> Yeah, the rest are just leaks, and we have already tons of them :)
>
> The point is that the commit in question introduced multiple such
> issues, so they should be addressed as well.
>
> But, the biggest problem is that OF api is very hard to use right...

Hello Mark/Takashi,

Just sent this https://lkml.org/lkml/2019/2/16/52
to fix the problem that was bugging me and to
update the patch from Kuninori.

Feel free to send a patch for the rest of the refcount issues :).

Kuninori, if you consider necessary please add your Signed-off-by to the patch
as it is mostly your work but I didn't know exactly how to give you
credit for it :(.

thanks,
Daniel.

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Kuninori Morimoto]

Hi Takashi-san

> > commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
> > merged simple-card and simple-scu-card. Then it had refcount
> > underflow bug. This patch fixup it.
> > We will get below error without this patch.
> > 
> > 	OF: ERROR: Bad of_node_put() on /sound
> > 	CPU: 3 PID: 237 Comm: kworker/3:1 Not tainted 5.0.0-rc6+ #1514
> > 	Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
> > 	Workqueue: events deferred_probe_work_func
> > 	Call trace:
> > 	 dump_backtrace+0x0/0x150
> > 	 show_stack+0x24/0x30
> > 	 dump_stack+0xb0/0xec
> > 	 of_node_release+0xd0/0xd8
> > 	 kobject_put+0x74/0xe8
> > 	 of_node_put+0x24/0x30
> > 	 __of_get_next_child+0x50/0x70
> > 	 of_get_next_child+0x40/0x68
> > 	 asoc_simple_card_probe+0x604/0x730
> > 	 platform_drv_probe+0x58/0xa8
> > 	 ...
> > Reported-by: Vicente Bergas <vicencb@gmail.com>
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Please don't forget to put Fixes tag for a regression fix.

Oops, indeed. Thanks.
will add it on v2 patch.

> And, looking at the code in 5.0-rc, it seems still leaving the
> reference in some condition.  The 5.0-rc code corrected your patch
> looks like:
> 
> 	node = of_get_child_by_name(top, PREFIX "dai-link");
> 	if (!node) {
> 		node = of_node_get(top);
> 		loop = 0;
> 	}
> 
> 	do  {
> 		....
> 		node = of_get_next_child(top, node);
> 	} while (loop && node);
> 
> So when loop=0, it aborts the loop at the first iteration.  If node is
> non-NULL at that point, it still keeps the refcount taken in
> of_get_next_child().
> 
> That is, you'd need to call of_node_put(node) after the loop.

Ah.. yes..

> And, this leads me checking the whole code, and I'm afraid that we
> have many similar unblanced refcounts.  For example, a tricky one is
> for_each_child_of_node() loop.  When you abort at the middle of the
> loop, you'd need to unreference the remaining node.  But we return or
> abort immediately without unreferencing in many places.
> 
> Hrm...

Yes, I had noticed this refcounts issue.
The code can be very confusable for this refcounts...

Best regards
---
Kuninori Morimoto

Re: [PATCH] ASoC: simple-card: fixup refcount_t underflow

[Kuninori Morimoto]

Hi Daniel

> Hello Mark/Takashi,
> 
> Just sent this https://lkml.org/lkml/2019/2/16/52
> to fix the problem that was bugging me and to
> update the patch from Kuninori.
> 
> Feel free to send a patch for the rest of the refcount issues :).
> 
> Kuninori, if you consider necessary please add your Signed-off-by to the patch
> as it is mostly your work but I didn't know exactly how to give you
> credit for it :(.

As Takashi mentioned, I think we need extra of_node_put(),
but it can be additional patch (?)
Anyway, thank you for your patch.
Yeah, I want to add my Signed-off-by (or Acked-by ?) on it.
I will post such mail.

Applied "ASoC: simple-card: fixup refcount_t underflow" to the asoc tree

[Mark Brown]

The patch

   ASoC: simple-card: fixup refcount_t underflow

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 19dd0777773ab17b4d97f7105e836867c0cdecb4 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Fri, 15 Feb 2019 15:31:29 +0900
Subject: [PATCH] ASoC: simple-card: fixup refcount_t underflow

commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
merged simple-card and simple-scu-card. Then it had refcount
underflow bug. This patch fixup it.
We will get below error without this patch.

	OF: ERROR: Bad of_node_put() on /sound
	CPU: 3 PID: 237 Comm: kworker/3:1 Not tainted 5.0.0-rc6+ #1514
	Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
	Workqueue: events deferred_probe_work_func
	Call trace:
	 dump_backtrace+0x0/0x150
	 show_stack+0x24/0x30
	 dump_stack+0xb0/0xec
	 of_node_release+0xd0/0xd8
	 kobject_put+0x74/0xe8
	 of_node_put+0x24/0x30
	 __of_get_next_child+0x50/0x70
	 of_get_next_child+0x40/0x68
	 asoc_simple_card_probe+0x604/0x730
	 platform_drv_probe+0x58/0xa8
	 ...
Reported-by: Vicente Bergas <vicencb@gmail.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/generic/simple-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 37e001cf9cd1..3fe34417ec89 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -462,7 +462,7 @@ static int asoc_simple_card_parse_of(struct simple_card_data *priv)
 	conf_idx	= 0;
 	node = of_get_child_by_name(top, PREFIX "dai-link");
 	if (!node) {
-		node = dev->of_node;
+		node = of_node_get(top);
 		loop = 0;
 	}
 
-- 
2.20.1

Applied "ASoC: simple-card: fixup refcount_t underflow" to the asoc tree

[Mark Brown]

The patch

   ASoC: simple-card: fixup refcount_t underflow

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 19dd0777773ab17b4d97f7105e836867c0cdecb4 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Fri, 15 Feb 2019 15:31:29 +0900
Subject: [PATCH] ASoC: simple-card: fixup refcount_t underflow

commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
merged simple-card and simple-scu-card. Then it had refcount
underflow bug. This patch fixup it.
We will get below error without this patch.

	OF: ERROR: Bad of_node_put() on /sound
	CPU: 3 PID: 237 Comm: kworker/3:1 Not tainted 5.0.0-rc6+ #1514
	Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT)
	Workqueue: events deferred_probe_work_func
	Call trace:
	 dump_backtrace+0x0/0x150
	 show_stack+0x24/0x30
	 dump_stack+0xb0/0xec
	 of_node_release+0xd0/0xd8
	 kobject_put+0x74/0xe8
	 of_node_put+0x24/0x30
	 __of_get_next_child+0x50/0x70
	 of_get_next_child+0x40/0x68
	 asoc_simple_card_probe+0x604/0x730
	 platform_drv_probe+0x58/0xa8
	 ...
Reported-by: Vicente Bergas <vicencb@gmail.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/generic/simple-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 37e001cf9cd1..3fe34417ec89 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -462,7 +462,7 @@ static int asoc_simple_card_parse_of(struct simple_card_data *priv)
 	conf_idx	= 0;
 	node = of_get_child_by_name(top, PREFIX "dai-link");
 	if (!node) {
-		node = dev->of_node;
+		node = of_node_get(top);
 		loop = 0;
 	}
 
-- 
2.20.1