[alsa-devel] [PATCH v2 0/2] Add snd_card_disconnect_sync() helper
Hi,
here is a refreshed series containing ALSA core changes Morimoto-san posted. Basically the code is same but it's split, and more comments and an error message are added. I tested lightly with the existing hot-pluggable device and no regression was seen.
The commits are found in topic/card-disconnect branch of sound git tree, too. Mark, please pull it to yours when applying the further patches relying on this new helper function.
thanks,
Takashi
===
Takashi Iwai (2): ALSA: add snd_card_disconnect_sync() ALSA: pcm: Forcibly stop at disconnect callback
include/sound/core.h | 2 ++ sound/core/init.c | 32 ++++++++++++++++++++++++++++++++ sound/core/pcm.c | 4 ++++ 3 files changed, 38 insertions(+)
In case of user unbind ALSA driver during playing back / capturing, each driver needs to stop and remove it correctly. One note here is that we can't cancel from remove function in such case, because unbind operation doesn't check return value from remove function. So, we *must* stop and remove in this case.
For this purpose, we need to sync (= wait) until the all top-level operations are canceled at remove function. For example, snd_card_free() processes the disconnection procedure at first, then waits for the completion. That's how the hot-unplug works safely. It's implemented, at least, in the top-level driver removal.
Now for the lower level driver, we need a similar strategy. Notify to the toplevel for hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
This patch adds snd_card_disconnect_sync(), and driver can use it from remove function.
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/core.h | 2 ++ sound/core/init.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)
diff --git a/include/sound/core.h b/include/sound/core.h index 4104a9d1001f..5f181b875c2f 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -133,6 +133,7 @@ struct snd_card { struct device card_dev; /* cardX object for sysfs */ const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ bool registered; /* card_dev is registered? */ + wait_queue_head_t remove_sleep;
#ifdef CONFIG_PM unsigned int power_state; /* power state */ @@ -240,6 +241,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, struct snd_card **card_ret);
int snd_card_disconnect(struct snd_card *card); +void snd_card_disconnect_sync(struct snd_card *card); int snd_card_free(struct snd_card *card); int snd_card_free_when_closed(struct snd_card *card); void snd_card_set_id(struct snd_card *card, const char *id); diff --git a/sound/core/init.c b/sound/core/init.c index 32ebe2f6bc59..168ae03d3a1c 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -255,6 +255,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, #ifdef CONFIG_PM init_waitqueue_head(&card->power_sleep); #endif + init_waitqueue_head(&card->remove_sleep);
device_initialize(&card->card_dev); card->card_dev.parent = parent; @@ -452,6 +453,35 @@ int snd_card_disconnect(struct snd_card *card) } EXPORT_SYMBOL(snd_card_disconnect);
+/** + * snd_card_disconnect_sync - disconnect card and wait until files get closed + * @card: card object to disconnect + * + * This calls snd_card_disconnect() for disconnecting all belonging components + * and waits until all pending files get closed. + * It assures that all accesses from user-space finished so that the driver + * can release its resources gracefully. + */ +void snd_card_disconnect_sync(struct snd_card *card) +{ + int err; + + err = snd_card_disconnect(card); + if (err < 0) { + dev_err(card->dev, + "snd_card_disconnect error (%d), skipping sync\n", + err); + return; + } + + spin_lock_irq(&card->files_lock); + wait_event_lock_irq(card->remove_sleep, + list_empty(&card->files_list), + card->files_lock); + spin_unlock_irq(&card->files_lock); +} +EXPORT_SYMBOL_GPL(snd_card_disconnect_sync); + static int snd_card_do_free(struct snd_card *card) { #if IS_ENABLED(CONFIG_SND_MIXER_OSS) @@ -957,6 +987,8 @@ int snd_card_file_remove(struct snd_card *card, struct file *file) break; } } + if (list_empty(&card->files_list)) + wake_up_all(&card->remove_sleep); spin_unlock(&card->files_lock); if (!found) { dev_err(card->dev, "card file remove problem (%p)\n", file);
Hi,
On Oct 11 2017 19:16, Takashi Iwai wrote:
In case of user unbind ALSA driver during playing back / capturing, each driver needs to stop and remove it correctly. One note here is that we can't cancel from remove function in such case, because unbind operation doesn't check return value from remove function. So, we *must* stop and remove in this case.
For this purpose, we need to sync (= wait) until the all top-level operations are canceled at remove function. For example, snd_card_free() processes the disconnection procedure at first, then waits for the completion. That's how the hot-unplug works safely. It's implemented, at least, in the top-level driver removal.
Now for the lower level driver, we need a similar strategy. Notify to the toplevel for hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
This patch adds snd_card_disconnect_sync(), and driver can use it from remove function.
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 2 ++ sound/core/init.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)
I have a question for this patch. If this patch is applied and the lower-level driver call this function in its .remove callback, after all references to an instance of snd_card by file instances corresponding to each of ALSA character devices are released, the lower-level driver must explicitly releases the last reference to the instance of snd_card. On ALSA SoC part, which context perform this important task? In my opinion, this patch easily allows leak of kobject. Do you have any safety?
At least, it's better for us to add enough comments to inform it to developers.
Regards
Takashi Sakamoto
On Wed, 11 Oct 2017 13:22:12 +0200, Takashi Sakamoto wrote:
Hi,
On Oct 11 2017 19:16, Takashi Iwai wrote:
In case of user unbind ALSA driver during playing back / capturing, each driver needs to stop and remove it correctly. One note here is that we can't cancel from remove function in such case, because unbind operation doesn't check return value from remove function. So, we *must* stop and remove in this case.
For this purpose, we need to sync (= wait) until the all top-level operations are canceled at remove function. For example, snd_card_free() processes the disconnection procedure at first, then waits for the completion. That's how the hot-unplug works safely. It's implemented, at least, in the top-level driver removal.
Now for the lower level driver, we need a similar strategy. Notify to the toplevel for hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
This patch adds snd_card_disconnect_sync(), and driver can use it from remove function.
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 2 ++ sound/core/init.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)
I have a question for this patch. If this patch is applied and the lower-level driver call this function in its .remove callback, after all references to an instance of snd_card by file instances corresponding to each of ALSA character devices are released, the lower-level driver must explicitly releases the last reference to the instance of snd_card.
Not really. This is only about files user-space opened. It has nothing to do with the refcount of objects / resources. The refcount sync is done by card->release_completion instead, which is sync'ed in snd_card_free().
On ALSA SoC part, which context perform this important task?
The "lower-level" might be confusing here; in the context, it's rather meant as middle layer (component) that can be unbound freely. Thus, for non-ASoC codes, this hasn't been a big problem, so far, since the helper modules can't be unbound. (Maybe the legacy AC97 codec driver needs something, and HD-audio has already some not-so-perfect workaround.)
In my opinion, this patch easily allows leak of kobject. Do you have any safety?
There isn't anything different from the current state wrt kobjects. The only merit of calling this in the remove callback is that it can assure that the all actions are stopped, by disconnecting the devices. It doesn't free resources by itself.
At least, it's better for us to add enough comments to inform it to developers.
Well, I thought it well informed about the disconnection procedure. Maybe the term "lower level driver" needs to be more explicit.
thanks,
Takashi
On Wed, 11 Oct 2017 14:19:52 +0200, Takashi Iwai wrote:
At least, it's better for us to add enough comments to inform it to developers.
Well, I thought it well informed about the disconnection procedure. Maybe the term "lower level driver" needs to be more explicit.
I updated the patch description and added a couple of notes for explaining your concerns.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: add snd_card_disconnect_sync()
In case of user unbind ALSA driver during playing back / capturing, each driver needs to stop and remove it correctly. One note here is that we can't cancel from remove function in such case, because unbind operation doesn't check return value from remove function. So, we *must* stop and remove in this case.
For this purpose, we need to sync (= wait) until the all top-level operations are canceled at remove function. For example, snd_card_free() processes the disconnection procedure at first, then waits for the completion. That's how the hot-unplug works safely. It's implemented, at least, in the top-level driver removal.
Now for the lower level driver, we need a similar strategy. Notify to the toplevel for hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
This patch adds snd_card_disconnect_sync(), and driver can use it from remove function.
Note: the "lower level" driver here refers to a middle layer driver (e.g. ASoC components) that can be unbound freely during operation. Most of legacy ALSA helper drivers don't have such a problem because they can't be unbound.
Note#2: snd_card_disconnect_sync() merely calls snd_card_disconnect() and syncs with closing all pending files. It takes only the files opened by user-space into account, and doesn't care about object refcounts. (The latter is handled by snd_card_free() completion call, BTW.) Also, the function doesn't free resources by itself.
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/core.h | 2 ++ sound/core/init.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)
diff --git a/include/sound/core.h b/include/sound/core.h index 4104a9d1001f..5f181b875c2f 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -133,6 +133,7 @@ struct snd_card { struct device card_dev; /* cardX object for sysfs */ const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ bool registered; /* card_dev is registered? */ + wait_queue_head_t remove_sleep;
#ifdef CONFIG_PM unsigned int power_state; /* power state */ @@ -240,6 +241,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, struct snd_card **card_ret);
int snd_card_disconnect(struct snd_card *card); +void snd_card_disconnect_sync(struct snd_card *card); int snd_card_free(struct snd_card *card); int snd_card_free_when_closed(struct snd_card *card); void snd_card_set_id(struct snd_card *card, const char *id); diff --git a/sound/core/init.c b/sound/core/init.c index 32ebe2f6bc59..168ae03d3a1c 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -255,6 +255,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, #ifdef CONFIG_PM init_waitqueue_head(&card->power_sleep); #endif + init_waitqueue_head(&card->remove_sleep);
device_initialize(&card->card_dev); card->card_dev.parent = parent; @@ -452,6 +453,35 @@ int snd_card_disconnect(struct snd_card *card) } EXPORT_SYMBOL(snd_card_disconnect);
+/** + * snd_card_disconnect_sync - disconnect card and wait until files get closed + * @card: card object to disconnect + * + * This calls snd_card_disconnect() for disconnecting all belonging components + * and waits until all pending files get closed. + * It assures that all accesses from user-space finished so that the driver + * can release its resources gracefully. + */ +void snd_card_disconnect_sync(struct snd_card *card) +{ + int err; + + err = snd_card_disconnect(card); + if (err < 0) { + dev_err(card->dev, + "snd_card_disconnect error (%d), skipping sync\n", + err); + return; + } + + spin_lock_irq(&card->files_lock); + wait_event_lock_irq(card->remove_sleep, + list_empty(&card->files_list), + card->files_lock); + spin_unlock_irq(&card->files_lock); +} +EXPORT_SYMBOL_GPL(snd_card_disconnect_sync); + static int snd_card_do_free(struct snd_card *card) { #if IS_ENABLED(CONFIG_SND_MIXER_OSS) @@ -957,6 +987,8 @@ int snd_card_file_remove(struct snd_card *card, struct file *file) break; } } + if (list_empty(&card->files_list)) + wake_up_all(&card->remove_sleep); spin_unlock(&card->files_lock); if (!found) { dev_err(card->dev, "card file remove problem (%p)\n", file);
So far we assumed that each driver implements the hotplug PCM handling properly, e.g. dealing with the pending PCM stream at disconnect callback. But most codes don't care, and it eventually leaves the PCM stream inconsistent state when an abrupt disconnection like sysfs unbind happens.
This patch is simple but a big-hammer solution: invoke snd_pcm_stop() at the common PCM disconnect callback always when the stream is running.
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7eadb7fd8074..054e47ad23ed 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -1152,6 +1152,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) { snd_pcm_stream_lock_irq(substream); if (substream->runtime) { + if (snd_pcm_running(substream)) + snd_pcm_stop(substream, + SNDRV_PCM_STATE_DISCONNECTED); + /* to be sure, set the state unconditionally */ substream->runtime->status->state = SNDRV_PCM_STATE_DISCONNECTED; wake_up(&substream->runtime->sleep); wake_up(&substream->runtime->tsleep);
Hi Takashi-san
here is a refreshed series containing ALSA core changes Morimoto-san posted. Basically the code is same but it's split, and more comments and an error message are added. I tested lightly with the existing hot-pluggable device and no regression was seen.
The commits are found in topic/card-disconnect branch of sound git tree, too. Mark, please pull it to yours when applying the further patches relying on this new helper function.
I tested this patch-set. I noticed that it doesn't work if I used DPCM (Kernel has Oops). I will investigate it next week
Best regards --- Kuninori Morimoto
On Fri, 13 Oct 2017 09:39:00 +0200, Kuninori Morimoto wrote:
Hi Takashi-san
here is a refreshed series containing ALSA core changes Morimoto-san posted. Basically the code is same but it's split, and more comments and an error message are added. I tested lightly with the existing hot-pluggable device and no regression was seen.
The commits are found in topic/card-disconnect branch of sound git tree, too. Mark, please pull it to yours when applying the further patches relying on this new helper function.
I tested this patch-set. I noticed that it doesn't work if I used DPCM (Kernel has Oops). I will investigate it next week
Could you show the Oops message if you have? My concern is whether it happens by stopping at disconnect, or it's just another missing piece.
thanks,
Takashi
Hi Takashi
I tested this patch-set. I noticed that it doesn't work if I used DPCM (Kernel has Oops). I will investigate it next week
Could you show the Oops message if you have? My concern is whether it happens by stopping at disconnect, or it's just another missing piece.
OK, but kernel log doesn't help you. see below
My environment now is I'm using DPCM. FE : rsnd FE : rsnd BE : ak4613
1st issue is that kernel need below patch. I guess BE is using dummy driver, and it doesn't have ops(?). If this is needed, I can post it.
----------------------- diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2fec2fe..972408b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1241,7 +1241,8 @@ static int snd_pcm_do_stop(struct snd_pcm_substream *substream, int state) { if (substream->runtime->trigger_master == substream && snd_pcm_running(substream)) - substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP); + if (substream->ops && substream->ops->trigger) + substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP); return 0; /* unconditonally stop all substreams */ } -----------------------
After this patch, my driver side clock counter mismatch Oops happen. It seems this is because FE side wasn't called snd_pcm_stop(); (= BE side only called snd_pcm_stop()).
I could confirm this by printing who's stop was called by local quick hack.
Maybe timing reason, if kernel has Oops for some reasons, then, both BE/FE snd_pcm_stop() are called. If no Oops, BE snd_pcm_stop() only called.
Best regards --- Kuninori Morimoto
On Fri, 13 Oct 2017 11:11:42 +0200, Kuninori Morimoto wrote:
Hi Takashi
I tested this patch-set. I noticed that it doesn't work if I used DPCM (Kernel has Oops). I will investigate it next week
Could you show the Oops message if you have? My concern is whether it happens by stopping at disconnect, or it's just another missing piece.
OK, but kernel log doesn't help you. see below
My environment now is I'm using DPCM. FE : rsnd FE : rsnd BE : ak4613
1st issue is that kernel need below patch. I guess BE is using dummy driver, and it doesn't have ops(?). If this is needed, I can post it.
No, this can't be right. Every PCM implementation mandates the presence of a trigger callback. It's a must. If a fix needed, it has to be fixed in the driver side.
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2fec2fe..972408b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1241,7 +1241,8 @@ static int snd_pcm_do_stop(struct snd_pcm_substream *substream, int state) { if (substream->runtime->trigger_master == substream && snd_pcm_running(substream))
substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
if (substream->ops && substream->ops->trigger)substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);}
After this patch, my driver side clock counter mismatch Oops happen. It seems this is because FE side wasn't called snd_pcm_stop(); (= BE side only called snd_pcm_stop()).
So the check might work around an Oops, but it's no "fix", per se.
I could confirm this by printing who's stop was called by local quick hack.
Maybe timing reason, if kernel has Oops for some reasons, then, both BE/FE snd_pcm_stop() are called. If no Oops, BE snd_pcm_stop() only called.
Any pending delayed work (like rtd->delayed_work)? This is flushed at soc_cleanup_card_resources(), but it's called at card removal, thus it's too late for the hot-removal of the component.
thanks,
Takashi
On Fri, Oct 13, 2017 at 11:43:59AM +0200, Takashi Iwai wrote:
Kuninori Morimoto wrote:
My environment now is I'm using DPCM. FE : rsnd FE : rsnd BE : ak4613
1st issue is that kernel need below patch. I guess BE is using dummy driver, and it doesn't have ops(?). If this is needed, I can post it.
No, this can't be right. Every PCM implementation mandates the presence of a trigger callback. It's a must. If a fix needed, it has to be fixed in the driver side.
I suspect this is a DPCM framework thing - when the paths are joined up at runtime there's going to be at least one trigger in there but with DPCM it's most likely going to be in the front end, the back end DMA is often going to be transparent to the system and so not have a trigger. However if you just iterate over all the PCMs you'll see the decomposed front and back end links with their dummies in there. Not sure what the fix is here, probably we need to be hiding the back end links more from the ALSA core.
Liam?
Hi Takashi
I could confirm this by printing who's stop was called by local quick hack.
Maybe timing reason, if kernel has Oops for some reasons, then, both BE/FE snd_pcm_stop() are called. If no Oops, BE snd_pcm_stop() only called.
Any pending delayed work (like rtd->delayed_work)? This is flushed at soc_cleanup_card_resources(), but it's called at card removal, thus it's too late for the hot-removal of the component.
Current my dirver used delayed_work, but we can control it by snd_soc_runtime_ignore_pmdown_time(); I think my driver now doesn't use delayed_work.
But there is still issue. I don't know detail, but it seems snd_pcm_dev_disconnect() and snd_pcm_relase() are called it the same time, both are calling snd_pcm_stop().
Then, snd_pcm_relase() side will calls snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.
Mark's suggestion (= hiding BE) can solve this ?
Best regards --- Kuninori Morimoto
On Mon, 16 Oct 2017 04:26:42 +0200, Kuninori Morimoto wrote:
Hi Takashi
I could confirm this by printing who's stop was called by local quick hack.
Maybe timing reason, if kernel has Oops for some reasons, then, both BE/FE snd_pcm_stop() are called. If no Oops, BE snd_pcm_stop() only called.
Any pending delayed work (like rtd->delayed_work)? This is flushed at soc_cleanup_card_resources(), but it's called at card removal, thus it's too late for the hot-removal of the component.
Current my dirver used delayed_work, but we can control it by snd_soc_runtime_ignore_pmdown_time(); I think my driver now doesn't use delayed_work.
But there is still issue. I don't know detail, but it seems snd_pcm_dev_disconnect() and snd_pcm_relase() are called it the same time, both are calling snd_pcm_stop().
Then, snd_pcm_relase() side will calls snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.
This must be also specific to DPCM. Something is really wrong there.
Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race since both are protected via pcm->open_mutex. snd_pcm_stop() calls are protected even more with substream lock.
Mark's suggestion (= hiding BE) can solve this ?
Some of the issues might be addressed, yes, but I'm skeptical whether it covers all. The BE needs proper locking and refcounting that is coupled with the FE, I suppose.
Takashi
Hi Takashi-san, Mark
Thank you for your feedback
Then, snd_pcm_relase() side will calls snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.
This must be also specific to DPCM. Something is really wrong there.
Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race since both are protected via pcm->open_mutex. snd_pcm_stop() calls are protected even more with substream lock.
Mark's suggestion (= hiding BE) can solve this ?
Some of the issues might be addressed, yes, but I'm skeptical whether it covers all. The BE needs proper locking and refcounting that is coupled with the FE, I suppose.
So, this means, your helper patch seems OK, but DPCM side needs more hack.
Mark I'm happy to solve this issue, but I'm not good at DPCM. If you can give me some help/advice, I can debug it.
Best regards --- Kuninori Morimoto
On Tue, 17 Oct 2017 02:59:26 +0200, Kuninori Morimoto wrote:
Hi Takashi-san, Mark
Thank you for your feedback
Then, snd_pcm_relase() side will calls snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.
This must be also specific to DPCM. Something is really wrong there.
Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race since both are protected via pcm->open_mutex. snd_pcm_stop() calls are protected even more with substream lock.
Mark's suggestion (= hiding BE) can solve this ?
Some of the issues might be addressed, yes, but I'm skeptical whether it covers all. The BE needs proper locking and refcounting that is coupled with the FE, I suppose.
So, this means, your helper patch seems OK, but DPCM side needs more hack.
Mark I'm happy to solve this issue, but I'm not good at DPCM. If you can give me some help/advice, I can debug it.
It seems that the whole disconnect call can be dropped for the BE, as it does nothing practically for the register callback, either.
Other than that, managing the object with the ALSA core device list is still worth for tracking the memory release.
Below is the patch to do that. It can be applied on top of the previous two patches (snd_card_disconnect_sync() and PCM-stop-at-disconnect).
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: pcm: Don't call register and disconnect callbacks for internal PCM
The internal PCM (aka DPCM backend PCM) doesn't need any registration procedure, thus currently we bail out immediately at dev_register callback. Similarly, its counterpart, dev_disconnect callback, is superfluous for the internal PCM. For simplifying and avoiding the conflicting disconnect call for internal PCM objects, this patch drops dev_register and dev_disconnect callbacks for the internal ops.
The only uncertain thing by this action is whether skipping the PCM state change to SNDRV_PCM_STATE_DISCONNECT for the internal PCM is mandatory. Looking through the current implementations, this doesn't look so, hence dropping the whole dev_disconnect would make more sense.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7eadb7fd8074..1b073ed0b1f9 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -775,6 +775,9 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, .dev_register = snd_pcm_dev_register, .dev_disconnect = snd_pcm_dev_disconnect, }; + static struct snd_device_ops internal_ops = { + .dev_free = snd_pcm_dev_free, + };
if (snd_BUG_ON(!card)) return -ENXIO; @@ -801,7 +804,8 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device, if (err < 0) goto free_pcm;
- err = snd_device_new(card, SNDRV_DEV_PCM, pcm, &ops); + err = snd_device_new(card, SNDRV_DEV_PCM, pcm, + internal ? &internal_ops : &ops); if (err < 0) goto free_pcm;
@@ -1099,8 +1103,6 @@ static int snd_pcm_dev_register(struct snd_device *device) if (snd_BUG_ON(!device || !device->device_data)) return -ENXIO; pcm = device->device_data; - if (pcm->internal) - return 0;
mutex_lock(®ister_mutex); err = snd_pcm_add(pcm); @@ -1159,12 +1161,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) snd_pcm_stream_unlock_irq(substream); } } - if (!pcm->internal) { - pcm_call_notify(pcm, n_disconnect); - } + + pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { - if (!pcm->internal) - snd_unregister_device(&pcm->streams[cidx].dev); + snd_unregister_device(&pcm->streams[cidx].dev); free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex);
On Tue, Oct 17, 2017 at 12:37:42PM +0200, Takashi Iwai wrote:
Kuninori Morimoto wrote:
I'm happy to solve this issue, but I'm not good at DPCM. If you can give me some help/advice, I can debug it.
Honestly I'm not super good with DPCM either, Liam wrote that code and I've never needed to delve too aggressively into it.
It seems that the whole disconnect call can be dropped for the BE, as it does nothing practically for the register callback, either.
That feels like it's going to blow up on us at some point... it's probably right for most things though.
Hi Takashi-san
It seems that the whole disconnect call can be dropped for the BE, as it does nothing practically for the register callback, either.
That feels like it's going to blow up on us at some point... it's probably right for most things though.
Wow !! Perfect !!
I tested your new "internal PCM" patch, and at least my kernel panic issue on DPCM was solved !
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
# I din't know that "internal" option was for DPCM backend PCM...
Best regards --- Kuninori Morimoto
On Wed, 18 Oct 2017 04:08:44 +0200, Kuninori Morimoto wrote:
Hi Takashi-san
It seems that the whole disconnect call can be dropped for the BE, as it does nothing practically for the register callback, either.
That feels like it's going to blow up on us at some point... it's probably right for most things though.
Wow !! Perfect !!
I tested your new "internal PCM" patch, and at least my kernel panic issue on DPCM was solved !
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
OK, I'll keep the branch not merged to for-next yet for a while in case we find any other breakage. The plan is to merge these changes for 4.15, so this will happen in a few days.
# I din't know that "internal" option was for DPCM backend PCM...
That needs more documentation...
thanks,
Takashi
Hi Takashi
I tested your new "internal PCM" patch, and at least my kernel panic issue on DPCM was solved !
Tested-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
OK, I'll keep the branch not merged to for-next yet for a while in case we find any other breakage. The plan is to merge these changes for 4.15, so this will happen in a few days.
Thanks !!
Best regards --- Kuninori Morimoto
Hi guys,
On Oct 17 2017 09:59, Kuninori Morimoto wrote:>> Some of the issues might be addressed, yes, but I'm skeptical whether
it covers all. The BE needs proper locking and refcounting that is coupled with the FE, I suppose.
So, this means, your helper patch seems OK, but DPCM side needs more hack.
In my opinion, it's better for us to take enough proofs and explanations when changing core functionalities.
Below patch for snd-dummy is to enable unbind operation to platform_device for this module. (but it's too rough to apply mainline.) This works without Iwai-san's two patches.
Let's try: $ modprobe snd-dummy $ aplay -D hw:Dummy /dev/null & $ lsmod | grep dummy $ echo -n snd_dummy.0 > /sys/bus/platform/drivers/snd_dummy/unbind $ lsmod | grep dummy $ cat /proc/asound/cards $ modprobe -r snd-dummy $ cat /proc/asound/cards
Actually, we have no addressed issues in these operations. The issue is only on ALSA SoC part.
But I note that even if unbind works fine to shift state of sound devices into disconnected, poll(2) call to ALSA control character devices does not return (e.g. run 'amixer events'). I don't know exactly the cause yet...
======== 8< --------
From 74bb5c45f0bf36e6487538088c654b88e1efb5b5 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Tue, 17 Oct 2017 17:58:47 +0900 Subject: [PATCH] ALSA: dummy: support unbind operation to shift state of sound devices to disconnected
--- sound/drivers/dummy.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c index c0939a0164a6..dcdefd2931c7 100644 --- a/sound/drivers/dummy.c +++ b/sound/drivers/dummy.c @@ -92,6 +92,7 @@ MODULE_PARM_DESC(hrtimer, "Use hrtimer as the timer source."); #endif
static struct platform_device *devices[SNDRV_CARDS]; +static struct snd_card *cards[SNDRV_CARDS];
#define MIXER_ADDR_MASTER 0 #define MIXER_ADDR_LINE 1 @@ -1134,7 +1135,24 @@ static int snd_dummy_probe(struct platform_device *devptr)
static int snd_dummy_remove(struct platform_device *devptr) { - snd_card_free(platform_get_drvdata(devptr)); + struct snd_card *card = platform_get_drvdata(devptr); + struct snd_device *dev; + int i; + + for (i = 0; i < SNDRV_CARDS; ++i) { + /* Temporary for module removal. */ + if (devices[i] == devptr) + cards[i] = card; + } + + /* + * This shifts state of an instance of sound card into disconnected, but + * don't wait till all of references to instances of sound devices are + * released. + */ + list_for_each_entry_reverse(dev, &card->devices, list) + snd_device_disconnect(card, dev->device_data); + return 0; }
@@ -1178,8 +1196,23 @@ static void snd_dummy_unregister_all(void) { int i;
- for (i = 0; i < ARRAY_SIZE(devices); ++i) - platform_device_unregister(devices[i]); + for (i = 0; i < ARRAY_SIZE(devices); ++i) { + struct platform_device *devptr = devices[i]; + struct snd_card *card = cards[i]; + + if (devptr == NULL) + continue; + if (card) { + /* + * This can wait till the final reference to an + * instance of sound card is released. + */ + snd_card_free(card); + } + + platform_device_unregister(devptr); + } + platform_driver_unregister(&snd_dummy_driver); free_fake_buffer(); }
On Tue, 17 Oct 2017 16:25:10 +0200, Takashi Sakamoto wrote:
Hi guys,
On Oct 17 2017 09:59, Kuninori Morimoto wrote:>> Some of the issues might be addressed, yes, but I'm skeptical whether
it covers all. The BE needs proper locking and refcounting that is coupled with the FE, I suppose.
So, this means, your helper patch seems OK, but DPCM side needs more hack.
In my opinion, it's better for us to take enough proofs and explanations when changing core functionalities.
Below patch for snd-dummy is to enable unbind operation to platform_device for this module. (but it's too rough to apply mainline.) This works without Iwai-san's two patches.
Let's try: $ modprobe snd-dummy $ aplay -D hw:Dummy /dev/null & $ lsmod | grep dummy $ echo -n snd_dummy.0 > /sys/bus/platform/drivers/snd_dummy/unbind $ lsmod | grep dummy $ cat /proc/asound/cards $ modprobe -r snd-dummy $ cat /proc/asound/cards
Actually, we have no addressed issues in these operations. The issue is only on ALSA SoC part.
Well, as I mentioned, a potential breakage would appear in the legacy AC97 codec binding -- if it would do any hot-unplug. But most of legacy drivers work fine without my patches just because it has only top-level unbind.
So, the top-level hot-unplug as you tested with the dummy driver works as is. The problem is only about unbinding the middle-layer component, as mentioned in the patch. It's found mostly in ASoC, but not necessarily tied only with ASoC.
But I note that even if unbind works fine to shift state of sound devices into disconnected, poll(2) call to ALSA control character devices does not return (e.g. run 'amixer events'). I don't know exactly the cause yet...
The disconnection doesn't close the device by itself (we can't), but it replaces with the dummy ops so that it never touches the driver except for closing. That is, the device is in the idle state and just accepts closing.
Takashi
======== 8< --------
From 74bb5c45f0bf36e6487538088c654b88e1efb5b5 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Tue, 17 Oct 2017 17:58:47 +0900 Subject: [PATCH] ALSA: dummy: support unbind operation to shift state of sound devices to disconnected
sound/drivers/dummy.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c index c0939a0164a6..dcdefd2931c7 100644 --- a/sound/drivers/dummy.c +++ b/sound/drivers/dummy.c @@ -92,6 +92,7 @@ MODULE_PARM_DESC(hrtimer, "Use hrtimer as the timer source."); #endif
static struct platform_device *devices[SNDRV_CARDS]; +static struct snd_card *cards[SNDRV_CARDS];
#define MIXER_ADDR_MASTER 0 #define MIXER_ADDR_LINE 1 @@ -1134,7 +1135,24 @@ static int snd_dummy_probe(struct platform_device *devptr)
static int snd_dummy_remove(struct platform_device *devptr) {
- snd_card_free(platform_get_drvdata(devptr));
- struct snd_card *card = platform_get_drvdata(devptr);
- struct snd_device *dev;
- int i;
- for (i = 0; i < SNDRV_CARDS; ++i) {
/* Temporary for module removal. */if (devices[i] == devptr)cards[i] = card;- }
- /*
* This shifts state of an instance of sound card into disconnected, but* don't wait till all of references to instances of sound devices are* released.*/- list_for_each_entry_reverse(dev, &card->devices, list)
snd_device_disconnect(card, dev->device_data);- return 0;
}
@@ -1178,8 +1196,23 @@ static void snd_dummy_unregister_all(void) { int i;
- for (i = 0; i < ARRAY_SIZE(devices); ++i)
platform_device_unregister(devices[i]);
- for (i = 0; i < ARRAY_SIZE(devices); ++i) {
struct platform_device *devptr = devices[i];struct snd_card *card = cards[i];if (devptr == NULL)continue;if (card) {/** This can wait till the final reference to an* instance of sound card is released.*/snd_card_free(card);}platform_device_unregister(devptr);- }
- platform_driver_unregister(&snd_dummy_driver); free_fake_buffer();
}
2.11.0
On Oct 17 2017 23:34, Takashi Iwai wrote:
Well, as I mentioned, a potential breakage would appear in the legacy AC97 codec binding -- if it would do any hot-unplug. But most of legacy drivers work fine without my patches just because it has only top-level unbind.
Yep. Recent drivers get no suffers from the issue because AC'97 is enough ancient, at least for rsnd.
So, the top-level hot-unplug as you tested with the dummy driver works as is. The problem is only about unbinding the middle-layer component, as mentioned in the patch. It's found mostly in ASoC, but not necessarily tied only with ASoC.
Are there any driver outside of ALSA SoC part to use the feature? Actually, not. It's quite natural to assume that the feature is somehow specialized for ALSA SoC part even it it's in ALSA PCM core.
And today I have little time to review the patch. Please check in-reply-to field of my previous message. This is not a reply to yours.
But I note that even if unbind works fine to shift state of sound devices into disconnected, poll(2) call to ALSA control character devices does not return (e.g. run 'amixer events'). I don't know exactly the cause yet...
The disconnection doesn't close the device by itself (we can't), but it replaces with the dummy ops so that it never touches the driver except for closing. That is, the device is in the idle state and just accepts closing.
You did misread. I just said that a call of poll(2) doesn't return.
1599 static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) 1600 { 1601 unsigned int mask; 1602 struct snd_ctl_file *ctl; 1603 1604 ctl = file->private_data; 1605 if (!ctl->subscribed) 1606 return 0; 1607 poll_wait(file, &ctl->change_sleep, wait);
This should be awakened by below lines:
1775 static int snd_ctl_dev_disconnect(struct snd_device *device) 1776 { 1777 struct snd_card *card = device->device_data; 1778 struct snd_ctl_file *ctl; 1779 1780 read_lock(&card->ctl_files_rwlock); 1781 list_for_each_entry(ctl, &card->ctl_files, list) { 1782 wake_up(&ctl->change_sleep);
As long as I observed, it doesn't even if the above line is executed. (how odd...) I suspect to hit some other bugs such as buffer-overrun depending on my environment. But it's out of my current plan and I didn't investigate further. That's all of what I experience.
Thanks
Takashi Sakamoto
On Tue, 17 Oct 2017 17:16:51 +0200, Takashi Sakamoto wrote:
On Oct 17 2017 23:34, Takashi Iwai wrote:
Well, as I mentioned, a potential breakage would appear in the legacy AC97 codec binding -- if it would do any hot-unplug. But most of legacy drivers work fine without my patches just because it has only top-level unbind.
Yep. Recent drivers get no suffers from the issue because AC'97 is enough ancient, at least for rsnd.
Erm, no, what I meant is the legacy AC97 codec driver implementation, i.e. the code in sound/pci/ac97/ac97_codec.c. The driver using this codec implementations are all non-ASoC. But AFAIK most of its usage is always with the hard-dependency, not via bus binding, so most likely we don't see the issue in the legacy drivers, luckily or unluckily.
So, the top-level hot-unplug as you tested with the dummy driver works as is. The problem is only about unbinding the middle-layer component, as mentioned in the patch. It's found mostly in ASoC, but not necessarily tied only with ASoC.
Are there any driver outside of ALSA SoC part to use the feature?
If a middle-layer driver can unbind over bus or class, yes, any driver code may suffer from the very same problem. The soundbus of AOA drivers likely has the same problem, for example, and I can imagine some of media stuff has a similar problem, too.
Actually, not. It's quite natural to assume that the feature is somehow specialized for ALSA SoC part even it it's in ALSA PCM core.
Please stop blaming as if it were only ASoC-specific. It's the nature of Linux driver model. As long as you can unbind some partial component, it has to sync with the rest for a proper removal. The lack of such mechanism in ASoC is partly because ALSA core doesn't provide the capability, and it's the very reason we're discussing here.
And today I have little time to review the patch. Please check in-reply-to field of my previous message. This is not a reply to yours.
But I note that even if unbind works fine to shift state of sound devices into disconnected, poll(2) call to ALSA control character devices does not return (e.g. run 'amixer events'). I don't know exactly the cause yet...
The disconnection doesn't close the device by itself (we can't), but it replaces with the dummy ops so that it never touches the driver except for closing. That is, the device is in the idle state and just accepts closing.
You did misread. I just said that a call of poll(2) doesn't return.
1599 static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) 1600 { 1601 unsigned int mask; 1602 struct snd_ctl_file *ctl; 1603 1604 ctl = file->private_data; 1605 if (!ctl->subscribed) 1606 return 0; 1607 poll_wait(file, &ctl->change_sleep, wait);
This should be awakened by below lines:
1775 static int snd_ctl_dev_disconnect(struct snd_device *device) 1776 { 1777 struct snd_card *card = device->device_data; 1778 struct snd_ctl_file *ctl; 1779 1780 read_lock(&card->ctl_files_rwlock); 1781 list_for_each_entry(ctl, &card->ctl_files, list) { 1782 wake_up(&ctl->change_sleep);
As long as I observed, it doesn't even if the above line is executed. (how odd...)
Hm, then it's worth to check, but it's irrelevant from the issue we're chasing.
I suspect to hit some other bugs such as buffer-overrun depending on my environment.
How can such a thing happen...?
But it's out of my current plan and I didn't investigate further. That's all of what I experience.
OK, thanks for reporting.
Takashi
participants (4)
-
Kuninori Morimoto -
Mark Brown -
Takashi Iwai -
Takashi Sakamoto