[alsa-devel] [PATCH] ASoC: dapm: fix module unload/reload failed issue by reset kcontrol's widget list

[Jie, Yang]

> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars at metafoo.de]
> Sent: Wednesday, July 29, 2015 4:41 AM
> To: Jie, Yang; Takashi Iwai
> Cc: alsa-devel at alsa-project.org; broonie at kernel.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ASoC: dapm: fix module unload/reload
> failed issue by reset kcontrol's widget list
> 
> On 07/28/2015 04:34 PM, Jie, Yang wrote:
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai at suse.de]
> >> Sent: Tuesday, July 28, 2015 5:07 PM
> >> To: Jie, Yang
> >> Cc: broonie at kernel.org; alsa-devel at alsa-project.org; Girdwood, Liam R
> >> Subject: Re: [PATCH] ASoC: dapm: fix module unload/reload failed
> >> issue by reset kcontrol's widget list
> >>
> >> On Tue, 28 Jul 2015 11:03:39 +0200,
> >> Jie Yang wrote:
> >>>
> >>> We need reset the related kcontrol's widget list during the freeing
> >>> of a widget, otherwise, the widget list may be outdated and
> >>> reference to it may introduce errors(they usually occurs during
> >>> driver modules unloading/reloading).
> >>>
> >>> Here adding a func dapm_update_kcontrols_of_freeing_widget and call
> >>> it at dapm_update_kcontrols_of_freeing_widget, to fix those issues.
> >>>
> >>> Signed-off-by: Jie Yang <yang.jie at intel.com>
> >>> ---
> >>>   sound/soc/soc-dapm.c | 31 +++++++++++++++++++++++++++++++
> >>>   1 file changed, 31 insertions(+)
> >>>
> >>> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
> >>> aa327c9..1ad8144 100644
> >>> --- a/sound/soc/soc-dapm.c
> >>> +++ b/sound/soc/soc-dapm.c
> >>> @@ -2305,6 +2305,35 @@ static void dapm_free_path(struct
> >> snd_soc_dapm_path *path)
> >>>   	kfree(path);
> >>>   }
> >>>
> >>> +/* update all dapm kcontrols that related to a widget which being
> >>> +freed*/ static void dapm_update_kcontrols_of_freeing_widget(struct
> >>> +snd_soc_dapm_widget * w) {
> >>> +	struct dapm_kcontrol_data *data = NULL;
> >>> +	int i, j, n;
> >>> +
> >>> +	if (w && w->kcontrols) {
> >>> +		for (i = 0; i < w->num_kcontrols; i++) {
> >>> +			if (w->kcontrols[i] == NULL)
> >>> +				continue;
> >>> +			data = snd_kcontrol_chip(w->kcontrols[i]);
> >>> +			if (data == NULL)
> >>> +				continue;
> >>> +			for (j = 0, n = 0; j < data->wlist->num_widgets; j++) {
> >>> +				if (data->wlist->widgets[j] == w) {
> >>> +					data->wlist->widgets[j] = NULL;
> >>> +					n++;
> >>> +				}
> >>> +			}
> >>> +
> >>> +			if (n) {
> >>> +				data->wlist->num_widgets -= n;
> >>
> >> This looks buggy.  You didn't rearrange widgets[] but just put a
> >> hole, yet decreasing the number.
> >
> > You are right, but since krealloc() cannot shrink the allocated size,
> > maybe we should free those related kcontrols here?
> >
> >>
> >> BTW, how does the reference occur while unloading?  Or better to say:
> >> isn't it the module refcount adjusted while accessing the resources?
> >
> >
> > soc_cleanup_card_resources
> > 	snd_soc_dapm_free
> > 		dapm_free_widgets
> > 	snd_card_free
> > 		//removing kcontrols, calling private dapm_kcontrol_free()
> > 			kfree(data->widget->name);//the widget here may
> be
> > 					//already freed! So the reference
> here
> > 					//may cause error.
> 
> Hi,
> 
> that free was wrong no matter what, since the widget itself frees the name.
> So even if widget is valid this would result in a double free.
> 
> The issue was fixed in this commit:
> http://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit?h=for-
> next&id=e18077b6e5dfe26e9fbbdc1fd1085a1701c24bea
 
Thanks, Lars, your fixing also fix my module unloading failed issue.

Then my patch may be not needed any more, the outdated data->wlist and 
data->widget(num_widgets and widgets[j]) looks not so critical, for they may
be freed totally at the subsequent kcontrol private removing.

Thanks,
~Keyon

> 
> - Lars