[alsa-devel] [PATCH] ALSA: hda: Simplify snd_hdac_refresh_widgets()

[Takashi Iwai]

On Mon, 08 Jul 2019 18:49:01 +0200,
Evan Green wrote:
> 
> On Fri, Jul 5, 2019 at 3:06 AM Takashi Iwai <tiwai at suse.de> wrote:
> >
> > Along with the recent fix for the races of snd_hdac_refresh_widgets()
> > it turned out that the instantiation of widgets sysfs at
> > snd_hdac_sysfs_reinit() could cause a race.  The race itself was
> > already covered later by extending the mutex protection range, the
> > commit 98482377dc72 ("ALSA: hda: Fix widget_mutex incomplete
> > protection"), but this also indicated that the call of *_reinit() is
> > basically superfluous, as the widgets shall be created sooner or later
> > from snd_hdac_device_register().
> >
> > This patch removes the redundant call of snd_hdac_sysfs_reinit() at
> > first.  By this removal, the sysfs argument itself in
> > snd_hdac_refresh_widgets() becomes superfluous, too, because the only
> > case sysfs=false is always with codec->widgets=NULL.  So, we drop this
> > redundant argument as well.
> >
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >  include/sound/hdaudio.h      |  2 +-
> >  sound/hda/hdac_device.c      | 13 +++++--------
> >  sound/hda/hdac_sysfs.c       |  2 +-
> >  sound/pci/hda/hda_codec.c    |  2 +-
> >  sound/soc/codecs/hdac_hdmi.c |  2 +-
> >  5 files changed, 9 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > index e8346784cf3f..f475293d0668 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -120,7 +120,7 @@ void snd_hdac_device_unregister(struct hdac_device *codec);
> >  int snd_hdac_device_set_chip_name(struct hdac_device *codec, const char *name);
> >  int snd_hdac_codec_modalias(struct hdac_device *hdac, char *buf, size_t size);
> >
> > -int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs);
> > +int snd_hdac_refresh_widgets(struct hdac_device *codec);
> >
> >  unsigned int snd_hdac_make_cmd(struct hdac_device *codec, hda_nid_t nid,
> >                                unsigned int verb, unsigned int parm);
> > diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> > index 11050bfd8068..a265c1d68876 100644
> > --- a/sound/hda/hdac_device.c
> > +++ b/sound/hda/hdac_device.c
> > @@ -89,7 +89,7 @@ int snd_hdac_device_init(struct hdac_device *codec, struct hdac_bus *bus,
> >
> >         fg = codec->afg ? codec->afg : codec->mfg;
> >
> > -       err = snd_hdac_refresh_widgets(codec, false);
> > +       err = snd_hdac_refresh_widgets(codec);
> >         if (err < 0)
> >                 goto error;
> >
> > @@ -394,9 +394,8 @@ static void setup_fg_nodes(struct hdac_device *codec)
> >  /**
> >   * snd_hdac_refresh_widgets - Reset the widget start/end nodes
> >   * @codec: the codec object
> > - * @sysfs: re-initialize sysfs tree, too
> >   */
> > -int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs)
> > +int snd_hdac_refresh_widgets(struct hdac_device *codec)
> >  {
> >         hda_nid_t start_nid;
> >         int nums, err = 0;
> > @@ -414,11 +413,9 @@ int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs)
> >                 goto unlock;
> >         }
> >
> > -       if (sysfs) {
> > -               err = hda_widget_sysfs_reinit(codec, start_nid, nums);
> > -               if (err < 0)
> > -                       goto unlock;
> > -       }
> > +       err = hda_widget_sysfs_reinit(codec, start_nid, nums);
> > +       if (err < 0)
> > +               goto unlock;
> >
> >         codec->num_nodes = nums;
> >         codec->start_nid = start_nid;
> > diff --git a/sound/hda/hdac_sysfs.c b/sound/hda/hdac_sysfs.c
> > index 909d5ef1179c..e56e83325903 100644
> > --- a/sound/hda/hdac_sysfs.c
> > +++ b/sound/hda/hdac_sysfs.c
> > @@ -428,7 +428,7 @@ int hda_widget_sysfs_reinit(struct hdac_device *codec,
> >         int i;
> >
> >         if (!codec->widgets)
> > -               return hda_widget_sysfs_init(codec);
> > +               return 0;
> 
> Hi Takashi,
> So this no-ops out reinit() if there are no widgets. Are there any
> cases in which hda_widget_sysfs_exit() could be called, followed by
> hda_widget_sysfs_reinit()? That would be problematic, as exit would
> wipe out the widgets, but now reinit would never restore them.

Such a pattern must be a bug.

> Are there any plausible cases where a caller might call
> refresh_widgets() and then attempt to utilize or otherwise depend on
> the widgets being present? The semantics of this function seem odd to
> me now, as it will now return having updated num_nodes, start_nid, and
> end_nid, but without having actually built the widgets array.
> 
> I'm probably biased, but I still think it makes more sense to let
> sysfs_reinit actually reinitialize the widgets, but just pass down
> start_nid and count so that the tree is built correctly.

The semantic is that refresh is a refresh -- not creating a stuff.

Actually this helper function is exposed solely for the case where the
widget tree is updated at the codec driver probe time (typically Intel
HDMI codec).  If it's used for anything else, especially implicitly
relying on its creation behavior, it's a bug to be fixed.


thanks,

Takashi