Re: [PATCH] ALSA: pcm: Drain proc writers before freeing preallocated buffers

[Takashi Iwai]

On Thu, 22 Oct 2026 10:58:02 +0200,
Cen Zhang wrote:
> 
> Hi Takashi,
> 
> Takashi Iwai <tiwai@xxxxxxx> 于2026年10月20日周二 16:40写道：
> >
> > I believe a better fix is to handle the disconnection of procfs more
> > properly.  The hda codec driver calls snd_device_disconnect() for each
> > pcm, but in the PCM side, the procfs removal relies on the card level
> > disconnection (snd_info_card_disconnect()).
> >
> > That is, something like below works instead?
> >
> >
> > thanks,
> >
> > Takashi
> >
> > --- a/sound/core/pcm.c
> > +++ b/sound/core/pcm.c
> > @@ -1119,6 +1119,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
> >         for (cidx = 0; cidx < 2; cidx++) {
> >                 if (pcm->streams[cidx].dev)
> >                         snd_unregister_device(pcm->streams[cidx].dev);
> > +               snd_pcm_stream_proc_done(&pcm->streams[cidx]);
> >                 free_chmap(&pcm->streams[cidx]);
> >         }
> >         return 0;
> >
> 
> Sorry for the delayed reply ― something came up to me over the past
> few days.
> 
> Thanks for the review. Handling the proc entries at PCM disconnect
> looks like a better approach.But I noticed two possible issues with
> adding the call here: proc removal can invoke a prealloc release
> callback that takes open_mutex, which is already held; freeing the
> subtree also leaves pstr->oss.proc_entry pointing to freed storage
> until final unregister.

Hm, basically we don't have to release the resources but just
disconnect procfs (that guarantees that the pending callbacks are
finished).  So how about something like below instead?

> I'll follow your approach, but perform proc rundown outside open_mutex
> and retain the entry objects until final cleanup. I'll also prevent subsequent
> card registration from republishing the disconnected entries.

But those are assumptions: the proc rundown must be after all pending
stuff is gone and it shouldn't be called until the next registration.
And, running re-registering before the complete release is already
fundamentally broken, and that should be addressed instead.


thanks,

Takashi

-- 8< --
diff --git a/include/sound/info.h b/include/sound/info.h
index adbc506860d6..4b08a32ff4fe 100644
--- a/include/sound/info.h
+++ b/include/sound/info.h
@@ -117,6 +117,7 @@ struct snd_info_entry *snd_info_create_module_entry(struct module *module,
 struct snd_info_entry *snd_info_create_card_entry(struct snd_card *card,
 					     const char *name,
 					     struct snd_info_entry *parent);
+void snd_info_disconnect_entry(struct snd_info_entry *entry);
 void snd_info_free_entry(struct snd_info_entry *entry);
 
 int snd_info_card_create(struct snd_card *card);
diff --git a/sound/core/info.c b/sound/core/info.c
index c4d57d0809ad..bb9f564001ea 100644
--- a/sound/core/info.c
+++ b/sound/core/info.c
@@ -737,6 +737,15 @@ static void snd_info_clear_entries(struct snd_info_entry *entry)
 	entry->p = NULL;
 }
 
+void snd_info_disconnect_entry(struct snd_info_entry *entry)
+{
+	if (entry && entry->p) {
+		proc_remove(entry->p);
+		guard(mutex)(&info_mutex);
+		snd_info_clear_entries(entry);
+	}
+}
+
 /**
  * snd_info_free_entry - release the info entry
  * @entry: the info entry
@@ -749,11 +758,7 @@ void snd_info_free_entry(struct snd_info_entry * entry)
 
 	if (!entry)
 		return;
-	if (entry->p) {
-		proc_remove(entry->p);
-		guard(mutex)(&info_mutex);
-		snd_info_clear_entries(entry);
-	}
+	snd_info_disconnect_entry(entry);
 
 	/* free all children at first */
 	list_for_each_entry_safe(p, n, &entry->children, list)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 1e4e0ecb3dbd..58098ee894a1 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -520,6 +520,11 @@ static int snd_pcm_stream_proc_init(struct snd_pcm_str *pstr)
 	return 0;
 }
 
+static void snd_pcm_stream_proc_disconnect(struct snd_pcm_str *pstr)
+{
+	snd_info_disconnect_entry(pstr->proc_root);
+}
+
 static int snd_pcm_stream_proc_done(struct snd_pcm_str *pstr)
 {
 	snd_info_free_entry(pstr->proc_root);
@@ -580,6 +585,7 @@ static int snd_pcm_substream_proc_init(struct snd_pcm_substream *substream)
 
 #else /* !CONFIG_SND_VERBOSE_PROCFS */
 static inline int snd_pcm_stream_proc_init(struct snd_pcm_str *pstr) { return 0; }
+static inline void snd_pcm_stream_proc_disconnect(struct snd_pcm_str *pstr) { return 0; }
 static inline int snd_pcm_stream_proc_done(struct snd_pcm_str *pstr) { return 0; }
 static inline int snd_pcm_substream_proc_init(struct snd_pcm_substream *substream) { return 0; }
 #endif /* CONFIG_SND_VERBOSE_PROCFS */
@@ -1119,6 +1125,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
 	for (cidx = 0; cidx < 2; cidx++) {
 		if (pcm->streams[cidx].dev)
 			snd_unregister_device(pcm->streams[cidx].dev);
+		snd_pcm_stream_proc_disconnect(&pcm->streams[cidx]);
 		free_chmap(&pcm->streams[cidx]);
 	}
 	return 0;