[v2,0/2] Add snd_card_disconnect_sync() helper

Commit Message

Takashi Iwai Oct. 17, 2017, 10:37 a.m. UTC

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 <[e-mail]>
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 <[e-mail]>
---
 sound/core/pcm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Mark Brown Oct. 17, 2017, 8:05 p.m. UTC | #1

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.

Kuninori Morimoto Oct. 18, 2017, 2:08 a.m. UTC | #2

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 <[e-mail]>

# I din't know that "internal" option was for DPCM backend PCM...

Best regards
---
Kuninori Morimoto

Takashi Iwai Oct. 18, 2017, 6:10 a.m. UTC | #3

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 <[e-mail]>

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

Kuninori Morimoto Oct. 18, 2017, 7:13 a.m. UTC | #4

Hi Takashi

> > I tested your new "internal PCM" patch, and at least my kernel panic issue
> > on DPCM was solved !
> > 
> > Tested-by: Kuninori Morimoto <[e-mail]>
> 
> 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

Patch

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);