ALSA: hda: Avoid racy recreation of widget kobjects

The refresh of HD-audio widget sysfs kobjects via
snd_hdac_refresh_widget_sysfs() is slightly racy.
The driver recreates the whole tree from scratch after deleting the
whole.  When CONFIG_DEBUG_KOBJECT_RELEASE option is used, kobject
release doesn't happen immediately but delayed, while the re-creation
of the same named kobject happens soon after invoking kobject_put().
This may end up with the conflicts of duplicated kobjects, as found in
the bug report below.

In this patch, we take another approach to refresh the tree: instead
of recreating the whole tree, just add the new nodes and delete the
non-existing nodes.  Since the refresh happens only once at
initialization, no longer race would happen.

Along with the code change, merge snd_hdac_refresh_widget_sysfs() with
the existing snd_hdac_refresh_widgets() with an additional bool flag
for simplifying the code.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=197307
Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 96546b30e90075f394ed5e8be608ddeb1bec519c..6f1118545bb8600eb88821a7d27c17d67f64084e 100644 (file)
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -111,8 +111,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);
-int snd_hdac_refresh_widget_sysfs(struct hdac_device *codec);
+int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs);
 
 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 19deb306facb7c771f6bd3a2cf0ea3f1f7c57131..06f845e293cb2dfcdaba0208aea03c69d3ad0ddc 100644 (file)
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -87,7 +87,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);
+       err = snd_hdac_refresh_widgets(codec, false);
        if (err < 0)
                goto error;
 
@@ -388,11 +388,12 @@ 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)
+int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs)
 {
        hda_nid_t start_nid;
-       int nums;
+       int nums, err;
 
        nums = snd_hdac_get_sub_nodes(codec, codec->afg, &start_nid);
        if (!start_nid || nums <= 0 || nums >= 0xff) {
@@ -401,6 +402,12 @@ int snd_hdac_refresh_widgets(struct hdac_device *codec)
                return -EINVAL;
        }
 
+       if (sysfs) {
+               err = hda_widget_sysfs_reinit(codec, start_nid, nums);
+               if (err < 0)
+                       return err;
+       }
+
        codec->num_nodes = nums;
        codec->start_nid = start_nid;
        codec->end_nid = start_nid + nums;
@@ -408,36 +415,6 @@ int snd_hdac_refresh_widgets(struct hdac_device *codec)
 }
 EXPORT_SYMBOL_GPL(snd_hdac_refresh_widgets);
 
-/**
- * snd_hdac_refresh_widget_sysfs - Reset the codec widgets and reinit the
- * codec sysfs
- * @codec: the codec object
- *
- * first we need to remove sysfs, then refresh widgets and lastly
- * recreate it
- */
-int snd_hdac_refresh_widget_sysfs(struct hdac_device *codec)
-{
-       int ret;
-
-       if (device_is_registered(&codec->dev))
-               hda_widget_sysfs_exit(codec);
-       ret = snd_hdac_refresh_widgets(codec);
-       if (ret) {
-               dev_err(&codec->dev, "failed to refresh widget: %d\n", ret);
-               return ret;
-       }
-       if (device_is_registered(&codec->dev)) {
-               ret = hda_widget_sysfs_init(codec);
-               if (ret) {
-                       dev_err(&codec->dev, "failed to init sysfs: %d\n", ret);
-                       return ret;
-               }
-       }
-       return ret;
-}
-EXPORT_SYMBOL_GPL(snd_hdac_refresh_widget_sysfs);
-
 /* return CONNLIST_LEN parameter of the given widget */
 static unsigned int get_num_conns(struct hdac_device *codec, hda_nid_t nid)
 {

diff --git a/sound/hda/hdac_sysfs.c b/sound/hda/hdac_sysfs.c
index 42d61bf41969c2176f1e10842e0c8e651d202a23..e123ba6b2e81cbc5cf4b0c0bb252084501064f1b 100644 (file)
--- a/sound/hda/hdac_sysfs.c
+++ b/sound/hda/hdac_sysfs.c
@@ -414,3 +414,50 @@ void hda_widget_sysfs_exit(struct hdac_device *codec)
 {
        widget_tree_free(codec);
 }
+
+int hda_widget_sysfs_reinit(struct hdac_device *codec,
+                           hda_nid_t start_nid, int num_nodes)
+{
+       struct hdac_widget_tree *tree;
+       hda_nid_t end_nid = start_nid + num_nodes;
+       hda_nid_t nid;
+       int i;
+
+       if (!codec->widgets)
+               return hda_widget_sysfs_init(codec);
+
+       tree = kmemdup(codec->widgets, sizeof(*tree), GFP_KERNEL);
+       if (!tree)
+               return -ENOMEM;
+
+       tree->nodes = kcalloc(num_nodes + 1, sizeof(*tree->nodes), GFP_KERNEL);
+       if (!tree->nodes) {
+               kfree(tree);
+               return -ENOMEM;
+       }
+
+       /* prune non-existing nodes */
+       for (i = 0, nid = codec->start_nid; i < codec->num_nodes; i++, nid++) {
+               if (nid < start_nid || nid >= end_nid)
+                       free_widget_node(codec->widgets->nodes[i],
+                                        &widget_node_group);
+       }
+
+       /* add new nodes */
+       for (i = 0, nid = start_nid; i < num_nodes; i++, nid++) {
+               if (nid < codec->start_nid || nid >= codec->end_nid)
+                       add_widget_node(tree->root, nid, &widget_node_group,
+                                       &tree->nodes[i]);
+               else
+                       tree->nodes[i] =
+                               codec->widgets->nodes[nid - codec->start_nid];
+       }
+
+       /* replace with the new tree */
+       kfree(codec->widgets->nodes);
+       kfree(codec->widgets);
+       codec->widgets = tree;
+
+       kobject_uevent(tree->root, KOBJ_CHANGE);
+       return 0;
+}

diff --git a/sound/hda/local.h b/sound/hda/local.h
index 0d5bb159d538ef53dcddb1914a3b4068c90f4795..c586ea62d49ec5b8ea4d652c1d2f846a8ce705ef 100644 (file)
--- a/sound/hda/local.h
+++ b/sound/hda/local.h
@@ -28,6 +28,8 @@ static inline unsigned int get_wcaps_channels(u32 wcaps)
 
 extern const struct attribute_group *hdac_dev_attr_groups[];
 int hda_widget_sysfs_init(struct hdac_device *codec);
+int hda_widget_sysfs_reinit(struct hdac_device *codec, hda_nid_t start_nid,
+                           int num_nodes);
 void hda_widget_sysfs_exit(struct hdac_device *codec);
 
 #endif /* __HDAC_LOCAL_H */

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 3db26c451837cf0e2893d1071c1a699465d89e1b..bb086665c3e02d127383c9b8148d3f37d32e0d4b 100644 (file)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -977,7 +977,7 @@ int snd_hda_codec_update_widgets(struct hda_codec *codec)
        hda_nid_t fg;
        int err;
 
-       err = snd_hdac_refresh_widget_sysfs(&codec->core);
+       err = snd_hdac_refresh_widgets(&codec->core, true);
        if (err < 0)
                return err;