ASoC: dapm: fix module unload/reload failed issue by reset kcontrol's widget list
diff mbox

Message ID 1438074219-26408-1-git-send-email-yang.jie@intel.com
State New
Headers show

Commit Message

Jie Yang July 28, 2015, 9:03 a.m. UTC
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@intel.com>
---
 sound/soc/soc-dapm.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Takashi Iwai July 28, 2015, 9:07 a.m. UTC | #1
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@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.

BTW, how does the reference occur while unloading?  Or better to say:
isn't it the module refcount adjusted while accessing the resources?


thanks,

Takashi
Jie Yang July 28, 2015, 2:34 p.m. UTC | #2
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, July 28, 2015 5:07 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@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@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.


So, actually, here resetting data->widget to NULL may be what we really need,
and updating data->wlist (num_widgets and widgets[j]) is only the icing I wanted
on the cake, maybe I should remove it?

Thanks,
~Keyon

> 
> 
> thanks,
> 
> Takashi
Lars-Peter Clausen July 28, 2015, 8:40 p.m. UTC | #3
On 07/28/2015 04:34 PM, Jie, Yang wrote:
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Tuesday, July 28, 2015 5:07 PM
>> To: Jie, Yang
>> Cc: broonie@kernel.org; alsa-devel@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@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

- Lars
Jie Yang July 29, 2015, 5:15 a.m. UTC | #4
> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
> Sent: Wednesday, July 29, 2015 4:41 AM
> To: Jie, Yang; Takashi Iwai
> Cc: alsa-devel@alsa-project.org; broonie@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@suse.de]
> >> Sent: Tuesday, July 28, 2015 5:07 PM
> >> To: Jie, Yang
> >> Cc: broonie@kernel.org; alsa-devel@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@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

Patch
diff mbox

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;
+				data->widget = NULL;
+			}
+		}
+	}
+
+}
+
 /* free all dapm widgets and resources */
 static void dapm_free_widgets(struct snd_soc_dapm_context *dapm)
 {
@@ -2326,6 +2355,8 @@  static void dapm_free_widgets(struct snd_soc_dapm_context *dapm)
 		list_for_each_entry_safe(p, next_p, &w->sinks, list_source)
 			dapm_free_path(p);
 
+		dapm_update_kcontrols_of_freeing_widget(w);
+
 		kfree(w->kcontrols);
 		kfree(w->name);
 		kfree(w);