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

[Takashi Iwai]

On Thu, 22 Oct 2026 12:56:37 +0200,
Cen Zhang wrote:
> 
> Takashi Iwai <tiwai@xxxxxxx> 于2026年10月22日周四 17:19写道：
> >
> > 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;
> 
> Agree that. But I moved proc rundown outside open_mutex because
> proc_remove() can invoke the
> prealloc release callback, which takes that mutex.

OK, it makes sense.


Takashi