| Message ID | 878syh8svy.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 19dd0777773ab17b4d97f7105e836867c0cdecb4 |
| Headers | show |
| Series | ASoC: simple-card: fixup refcount_t underflow | expand |
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
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
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?
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.
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.
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
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.
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; }