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

[Cen Zhang]

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.

Best regards,
Cen Zhang