Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai <tiwai@suse.de>]

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(&register_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);
-- 
2.14.2