[PATCH RFC 0/3] Embed card device into struct snd_card

[PATCH RFC 0/3] Embed card device into struct snd_card

[Takashi Iwai]

The third batch is a more intensive change; now struct device for
card-object is embedded in snd_card struct.  This makes the presence
of the device object presistent, which can be later referred by
other devices at the object creation time.

Also, along with this change, the refcounting of the card object
was changed just to use the standard device refcount.  This needs
to move the destructor into the device release callback.

Onre more batch will follow.

Takashi

[PATCH 1/3] ALSA: Embed card device into struct snd_card

[Takashi Iwai]

As prepared in the previous patch, we are ready to create a device
struct for the card object in snd_card_create() now.  This patch
changes the scheme from the old style to:

- embed a device struct for the card object into snd_card struct,
- initialize the card device in snd_card_create() (but not register),
- registration is done in snd_card_register() via device_add()

The actual card device is stored in card->card_dev.  The card->dev
pointer is kept unchanged and pointing to the parent device as before
for compatibility reason.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h | 10 ++++++----
 sound/core/init.c    | 54 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index fb05573215d3..f072e596f21a 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -22,6 +22,7 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/sched.h>		/* wake_up() */
 #include <linux/mutex.h>		/* struct mutex */
 #include <linux/rwsem.h>		/* struct rw_semaphore */
@@ -41,8 +42,6 @@
 /* forward declarations */
 struct pci_dev;
 struct module;
-struct device;
-struct device_attribute;
 
 /* device allocation stuff */
 
@@ -131,7 +130,8 @@ struct snd_card {
 	wait_queue_head_t shutdown_sleep;
 	atomic_t refcount;		/* refcount for disconnection */
 	struct device *dev;		/* device assigned to this card */
-	struct device *card_dev;	/* cardX object for sysfs */
+	struct device card_dev;		/* cardX object for sysfs */
+	bool registered;		/* card_dev is registered? */
 
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
@@ -145,6 +145,8 @@ struct snd_card {
 #endif
 };
 
+#define dev_to_snd_card(p)	container_of(p, struct snd_card, card_dev)
+
 #ifdef CONFIG_PM
 static inline void snd_power_lock(struct snd_card *card)
 {
@@ -193,7 +195,7 @@ struct snd_minor {
 /* return a device pointer linked to each sound device as a parent */
 static inline struct device *snd_card_get_device_link(struct snd_card *card)
 {
-	return card ? card->card_dev : NULL;
+	return card ? &card->card_dev : NULL;
 }
 
 /* sound.c */
diff --git a/sound/core/init.c b/sound/core/init.c
index 0115034914c9..2c0301811828 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -156,6 +156,13 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int),
 	return mask; /* unchanged */
 }
 
+static int snd_card_do_free(struct snd_card *card);
+
+static void release_card_device(struct device *dev)
+{
+	snd_card_do_free(dev_to_snd_card(dev));
+}
+
 /**
  *  snd_card_new - create and initialize a soundcard structure
  *  @parent: the parent device object
@@ -189,6 +196,8 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	card = kzalloc(sizeof(*card) + extra_size, GFP_KERNEL);
 	if (!card)
 		return -ENOMEM;
+	if (extra_size > 0)
+		card->private_data = (char *)card + sizeof(struct snd_card);
 	if (xid)
 		strlcpy(card->id, xid, sizeof(card->id));
 	err = 0;
@@ -208,7 +217,8 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 		mutex_unlock(&snd_card_mutex);
 		snd_printk(KERN_ERR "cannot find the slot for index %d (range 0-%i), error: %d\n",
 			 idx, snd_ecards_limit - 1, err);
-		goto __error;
+		kfree(card);
+		return err;
 	}
 	set_bit(idx, snd_cards_lock);		/* lock it */
 	if (idx >= snd_ecards_limit)
@@ -230,6 +240,15 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	mutex_init(&card->power_lock);
 	init_waitqueue_head(&card->power_sleep);
 #endif
+
+	device_initialize(&card->card_dev);
+	card->card_dev.parent = parent;
+	card->card_dev.class = sound_class;
+	card->card_dev.release = release_card_device;
+	err = kobject_set_name(&card->card_dev.kobj, "card%d", idx);
+	if (err < 0)
+		goto __error;
+
 	/* the control interface cannot be accessed from the user space until */
 	/* snd_cards_bitmask and snd_cards are set with snd_card_register */
 	err = snd_ctl_create(card);
@@ -242,15 +261,13 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 		snd_printk(KERN_ERR "unable to create card info\n");
 		goto __error_ctl;
 	}
-	if (extra_size > 0)
-		card->private_data = (char *)card + sizeof(struct snd_card);
 	*card_ret = card;
 	return 0;
 
       __error_ctl:
 	snd_device_free_all(card);
       __error:
-	kfree(card);
+	put_device(&card->card_dev);
   	return err;
 }
 EXPORT_SYMBOL(snd_card_new);
@@ -407,9 +424,9 @@ int snd_card_disconnect(struct snd_card *card)
 		snd_printk(KERN_ERR "not all devices for card %i can be disconnected\n", card->number);
 
 	snd_info_card_disconnect(card);
-	if (card->card_dev) {
-		device_unregister(card->card_dev);
-		card->card_dev = NULL;
+	if (card->registered) {
+		device_del(&card->card_dev);
+		card->registered = false;
 	}
 #ifdef CONFIG_PM
 	wake_up(&card->power_sleep);
@@ -463,7 +480,7 @@ void snd_card_unref(struct snd_card *card)
 	if (atomic_dec_and_test(&card->refcount)) {
 		wake_up(&card->shutdown_sleep);
 		if (card->free_on_last_close)
-			snd_card_do_free(card);
+			put_device(&card->card_dev);
 	}
 }
 EXPORT_SYMBOL(snd_card_unref);
@@ -481,7 +498,7 @@ int snd_card_free_when_closed(struct snd_card *card)
 
 	card->free_on_last_close = 1;
 	if (atomic_dec_and_test(&card->refcount))
-		snd_card_do_free(card);
+		put_device(&card->card_dev);
 	return 0;
 }
 
@@ -495,7 +512,7 @@ int snd_card_free(struct snd_card *card)
 
 	/* wait, until all devices are ready for the free operation */
 	wait_event(card->shutdown_sleep, !atomic_read(&card->refcount));
-	snd_card_do_free(card);
+	put_device(&card->card_dev);
 	return 0;
 }
 
@@ -686,12 +703,11 @@ int snd_card_register(struct snd_card *card)
 	if (snd_BUG_ON(!card))
 		return -EINVAL;
 
-	if (!card->card_dev) {
-		card->card_dev = device_create(sound_class, card->dev,
-					       MKDEV(0, 0), card,
-					       "card%i", card->number);
-		if (IS_ERR(card->card_dev))
-			card->card_dev = NULL;
+	if (!card->registered) {
+		err = device_add(&card->card_dev);
+		if (err < 0)
+			return err;
+		card->registered = true;
 	}
 
 	if ((err = snd_device_register_all(card)) < 0)
@@ -721,11 +737,11 @@ int snd_card_register(struct snd_card *card)
 	if (snd_mixer_oss_notify_callback)
 		snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_REGISTER);
 #endif
-	if (card->card_dev) {
-		err = device_create_file(card->card_dev, &card_id_attrs);
+	if (card->registered) {
+		err = device_create_file(&card->card_dev, &card_id_attrs);
 		if (err < 0)
 			return err;
-		err = device_create_file(card->card_dev, &card_number_attrs);
+		err = device_create_file(&card->card_dev, &card_number_attrs);
 		if (err < 0)
 			return err;
 	}
-- 
1.8.5.2

[PATCH 2/3] ALSA: Use static groups for id and number card sysfs attr files

[Takashi Iwai]

... instead of calling device_create_file() manually.
No functional change.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/init.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/sound/core/init.c b/sound/core/init.c
index 2c0301811828..46a9e7cb1c04 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -157,6 +157,7 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int),
 }
 
 static int snd_card_do_free(struct snd_card *card);
+static const struct attribute_group *card_dev_attr_groups[];
 
 static void release_card_device(struct device *dev)
 {
@@ -245,6 +246,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	card->card_dev.parent = parent;
 	card->card_dev.class = sound_class;
 	card->card_dev.release = release_card_device;
+	card->card_dev.groups = card_dev_attr_groups;
 	err = kobject_set_name(&card->card_dev.kobj, "card%d", idx);
 	if (err < 0)
 		goto __error;
@@ -671,8 +673,7 @@ card_id_store_attr(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static struct device_attribute card_id_attrs =
-	__ATTR(id, S_IRUGO | S_IWUSR, card_id_show_attr, card_id_store_attr);
+static DEVICE_ATTR(id, S_IRUGO | S_IWUSR, card_id_show_attr, card_id_store_attr);
 
 static ssize_t
 card_number_show_attr(struct device *dev,
@@ -682,8 +683,22 @@ card_number_show_attr(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%i\n", card ? card->number : -1);
 }
 
-static struct device_attribute card_number_attrs =
-	__ATTR(number, S_IRUGO, card_number_show_attr, NULL);
+static DEVICE_ATTR(number, S_IRUGO, card_number_show_attr, NULL);
+
+static struct attribute *card_dev_attrs[] = {
+	&dev_attr_id.attr,
+	&dev_attr_number.attr,
+	NULL
+};
+
+static struct attribute_group card_dev_attr_group = {
+	.attrs	= card_dev_attrs,
+};
+
+static const struct attribute_group *card_dev_attr_groups[] = {
+	&card_dev_attr_group,
+	NULL
+};
 
 /**
  *  snd_card_register - register the soundcard
@@ -737,15 +752,6 @@ int snd_card_register(struct snd_card *card)
 	if (snd_mixer_oss_notify_callback)
 		snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_REGISTER);
 #endif
-	if (card->registered) {
-		err = device_create_file(&card->card_dev, &card_id_attrs);
-		if (err < 0)
-			return err;
-		err = device_create_file(&card->card_dev, &card_number_attrs);
-		if (err < 0)
-			return err;
-	}
-
 	return 0;
 }
 
-- 
1.8.5.2

[PATCH 3/3] ALSA: Use standard device refcount for card accounting

[Takashi Iwai]

Drop the own refcount but use the standard device refcounting via
get_device() and put_device().  Introduce a new completion to snd_card
instead of the wait queue for syncing the last release, which is used
in snd_card_free().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h   |  7 +++----
 sound/core/init.c      | 53 ++++++++++++++------------------------------------
 sound/core/sound.c     |  2 +-
 sound/core/sound_oss.c |  2 +-
 4 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index f072e596f21a..f410a49862fd 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -42,6 +42,7 @@
 /* forward declarations */
 struct pci_dev;
 struct module;
+struct completion;
 
 /* device allocation stuff */
 
@@ -126,9 +127,7 @@ struct snd_card {
 								state */
 	spinlock_t files_lock;		/* lock the files for this card */
 	int shutdown;			/* this card is going down */
-	int free_on_last_close;		/* free in context of file_release */
-	wait_queue_head_t shutdown_sleep;
-	atomic_t refcount;		/* refcount for disconnection */
+	struct completion *release_completion;
 	struct device *dev;		/* device assigned to this card */
 	struct device card_dev;		/* cardX object for sysfs */
 	bool registered;		/* card_dev is registered? */
@@ -302,7 +301,7 @@ int snd_card_info_done(void);
 int snd_component_add(struct snd_card *card, const char *component);
 int snd_card_file_add(struct snd_card *card, struct file *file);
 int snd_card_file_remove(struct snd_card *card, struct file *file);
-void snd_card_unref(struct snd_card *card);
+#define snd_card_unref(card)	put_device(&(card)->card_dev)
 
 #define snd_card_set_dev(card, devptr) ((card)->dev = (devptr))
 
diff --git a/sound/core/init.c b/sound/core/init.c
index 46a9e7cb1c04..3fdb4e2ce5ea 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -28,6 +28,7 @@
 #include <linux/time.h>
 #include <linux/ctype.h>
 #include <linux/pm.h>
+#include <linux/completion.h>
 
 #include <sound/core.h>
 #include <sound/control.h>
@@ -235,8 +236,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	INIT_LIST_HEAD(&card->ctl_files);
 	spin_lock_init(&card->files_lock);
 	INIT_LIST_HEAD(&card->files_list);
-	init_waitqueue_head(&card->shutdown_sleep);
-	atomic_set(&card->refcount, 0);
 #ifdef CONFIG_PM
 	mutex_init(&card->power_lock);
 	init_waitqueue_head(&card->power_sleep);
@@ -466,58 +465,36 @@ static int snd_card_do_free(struct snd_card *card)
 		snd_printk(KERN_WARNING "unable to free card info\n");
 		/* Not fatal error */
 	}
+	if (card->release_completion)
+		complete(card->release_completion);
 	kfree(card);
 	return 0;
 }
 
-/**
- * snd_card_unref - release the reference counter
- * @card: the card instance
- *
- * Decrements the reference counter.  When it reaches to zero, wake up
- * the sleeper and call the destructor if needed.
- */
-void snd_card_unref(struct snd_card *card)
-{
-	if (atomic_dec_and_test(&card->refcount)) {
-		wake_up(&card->shutdown_sleep);
-		if (card->free_on_last_close)
-			put_device(&card->card_dev);
-	}
-}
-EXPORT_SYMBOL(snd_card_unref);
-
 int snd_card_free_when_closed(struct snd_card *card)
 {
-	int ret;
-
-	atomic_inc(&card->refcount);
-	ret = snd_card_disconnect(card);
-	if (ret) {
-		atomic_dec(&card->refcount);
+	int ret = snd_card_disconnect(card);
+	if (ret)
 		return ret;
-	}
-
-	card->free_on_last_close = 1;
-	if (atomic_dec_and_test(&card->refcount))
-		put_device(&card->card_dev);
+	put_device(&card->card_dev);
 	return 0;
 }
-
 EXPORT_SYMBOL(snd_card_free_when_closed);
 
 int snd_card_free(struct snd_card *card)
 {
-	int ret = snd_card_disconnect(card);
+	struct completion released;
+	int ret;
+
+	init_completion(&released);
+	card->release_completion = &released;
+	ret = snd_card_free_when_closed(card);
 	if (ret)
 		return ret;
-
 	/* wait, until all devices are ready for the free operation */
-	wait_event(card->shutdown_sleep, !atomic_read(&card->refcount));
-	put_device(&card->card_dev);
+	wait_for_completion(&released);
 	return 0;
 }
-
 EXPORT_SYMBOL(snd_card_free);
 
 /* retrieve the last word of shortname or longname */
@@ -924,7 +901,7 @@ int snd_card_file_add(struct snd_card *card, struct file *file)
 		return -ENODEV;
 	}
 	list_add(&mfile->list, &card->files_list);
-	atomic_inc(&card->refcount);
+	get_device(&card->card_dev);
 	spin_unlock(&card->files_lock);
 	return 0;
 }
@@ -967,7 +944,7 @@ int snd_card_file_remove(struct snd_card *card, struct file *file)
 		return -ENOENT;
 	}
 	kfree(found);
-	snd_card_unref(card);
+	put_device(&card->card_dev);
 	return 0;
 }
 
diff --git a/sound/core/sound.c b/sound/core/sound.c
index 437c25ea6403..4aaa53161644 100644
--- a/sound/core/sound.c
+++ b/sound/core/sound.c
@@ -118,7 +118,7 @@ void *snd_lookup_minor_data(unsigned int minor, int type)
 	if (mreg && mreg->type == type) {
 		private_data = mreg->private_data;
 		if (private_data && mreg->card_ptr)
-			atomic_inc(&mreg->card_ptr->refcount);
+			get_device(&mreg->card_ptr->card_dev);
 	} else
 		private_data = NULL;
 	mutex_unlock(&sound_mutex);
diff --git a/sound/core/sound_oss.c b/sound/core/sound_oss.c
index b19184d45f19..573a65eb2b79 100644
--- a/sound/core/sound_oss.c
+++ b/sound/core/sound_oss.c
@@ -55,7 +55,7 @@ void *snd_lookup_oss_minor_data(unsigned int minor, int type)
 	if (mreg && mreg->type == type) {
 		private_data = mreg->private_data;
 		if (private_data && mreg->card_ptr)
-			atomic_inc(&mreg->card_ptr->refcount);
+			get_device(&mreg->card_ptr->card_dev);
 	} else
 		private_data = NULL;
 	mutex_unlock(&sound_oss_mutex);
-- 
1.8.5.2

Re: [PATCH RFC 0/3] Embed card device into struct snd_card

[Takashi Iwai]

At Wed, 12 Feb 2014 12:20:10 +0100,
Takashi Iwai wrote:
> 
> The third batch is a more intensive change; now struct device for
> card-object is embedded in snd_card struct.  This makes the presence
> of the device object presistent, which can be later referred by
> other devices at the object creation time.
> 
> Also, along with this change, the refcounting of the card object
> was changed just to use the standard device refcount.  This needs
> to move the destructor into the device release callback.

This patch series have been merged to for-next branch now, too.


Takashi