| Message ID | 1438074219-26408-1-git-send-email-yang.jie@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> -----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
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
> -----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
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);
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(+)