[alsa-devel] [PATCH] ASoC: dapm: fix module unload/reload failed issue by reset kcontrol's widget list
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; + 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);
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-n...
- 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 referencehere
//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
participants (4)
-
Jie Yang -
Jie, Yang
-
Lars-Peter Clausen -
Takashi Iwai