[alsa-devel] [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 at gmail.com>
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx at 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