All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: clemens@ladisch.de, tiwai@suse.de
Cc: alsa-devel@alsa-project.org, ffado-devel@lists.sf.net
Subject: [PATCH 2/4] ALSA: dice: postpone card registration
Date: Thu, 24 Dec 2015 07:55:08 +0900	[thread overview]
Message-ID: <1450911310-15507-3-git-send-email-o-takashi@sakamocchi.jp> (raw)
In-Reply-To: <1450911310-15507-1-git-send-email-o-takashi@sakamocchi.jp>

Some models based on ASIC for Dice II series (STD, CP) change their
hardware configurations after appearing on IEEE 1394 bus. This is due to
interactions of boot loader (RedBoot), firmwares (eCos) and vendor's
configurations. This causes current ALSA dice driver to get wrong
information about the hardware's capability because its probe function
runs just after detecting unit of the model.

As long as I investigated, it takes a bit time (less than 1 second) to load
the firmware after bootstrap. Just after loaded, the driver can get
information about the unit. Then the hardware is initialized according to
vendor's configurations. After, the got information becomes wrong.
Between bootstrap, firmware loading and post configuration, some bus resets
are observed.

This commit offloads most processing of probe function into workqueue and
schedules the workqueue after successive bus resets. This has an effect to
get correct hardware information and avoid involvement to bus reset storm.

For code simplicity, this change effects all of Dice-based models, i.e.
Dice II, Dice Jr., Dice Mini and Dice III. Due to the same reason, sound
card instance is kept till card free function even if some errors are
detected in the workqueue.

I use a loose strategy to manage a race condition between the workqueue and
the bus reset. This is due to a specification of dice transaction. When bus
reset occurs, registered address for the transaction is cleared. Drivers
must re-register their own address again. While, this operation is
required for the workqueue because the workqueue includes to wait for the
transaction. This commit uses no lock primitives for the race condition in
bus reset handler. Instead, checking 'registered' member of
'struct snd_dice' avoid executing the workqueue several times.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice.c | 111 +++++++++++++++++++++++++++++++++------------
 sound/firewire/dice/dice.h |   3 ++
 2 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 26271cc..adb8c87 100644
--- a/sound/firewire/dice/dice.c
+++ b/sound/firewire/dice/dice.c
@@ -18,6 +18,8 @@ MODULE_LICENSE("GPL v2");
 #define WEISS_CATEGORY_ID	0x00
 #define LOUD_CATEGORY_ID	0x10
 
+#define PROBE_DELAY_MS		(2 * MSEC_PER_SEC)
+
 static int check_dice_category(struct fw_unit *unit)
 {
 	struct fw_device *device = fw_parent_device(unit);
@@ -185,6 +187,9 @@ static void dice_card_free(struct snd_card *card)
 {
 	struct snd_dice *dice = card->private_data;
 
+	/* The workqueue for registration uses the memory block. */
+	cancel_work_sync(&dice->dwork.work);
+
 	snd_dice_stream_destroy_duplex(dice);
 	snd_dice_transaction_destroy(dice);
 	fw_unit_put(dice->unit);
@@ -192,6 +197,66 @@ static void dice_card_free(struct snd_card *card)
 	mutex_destroy(&dice->mutex);
 }
 
+static void do_registration(struct work_struct *work)
+{
+	struct snd_dice *dice = container_of(work, struct snd_dice, dwork.work);
+	int err;
+
+	mutex_lock(&dice->mutex);
+
+	if (dice->card->shutdown || dice->registered)
+		goto end;
+
+	err = snd_dice_transaction_init(dice);
+	if (err < 0)
+		goto end;
+
+	err = dice_read_params(dice);
+	if (err < 0)
+		goto end;
+
+	dice_card_strings(dice);
+
+	err = snd_dice_create_pcm(dice);
+	if (err < 0)
+		goto end;
+
+	err = snd_dice_create_midi(dice);
+	if (err < 0)
+		goto end;
+
+	err = snd_card_register(dice->card);
+	if (err < 0)
+		goto end;
+
+	dice->registered = true;
+end:
+	mutex_unlock(&dice->mutex);
+
+	/*
+	 * It's a difficult work to manage a race condition between workqueue,
+	 * unit event handlers and processes. The memory block for this card
+	 * is released as the same way that usual sound cards are going to be
+	 * released.
+	 */
+}
+
+static inline void schedule_registration(struct snd_dice *dice)
+{
+	struct fw_card *fw_card = fw_parent_device(dice->unit)->card;
+	u64 now, delay;
+
+	now = get_jiffies_64();
+	delay = fw_card->reset_jiffies + msecs_to_jiffies(PROBE_DELAY_MS);
+
+	if (time_after64(delay, now))
+		delay -= now;
+	else
+		delay = 0;
+
+	schedule_delayed_work(&dice->dwork, delay);
+}
+
 static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 {
 	struct snd_card *card;
@@ -205,29 +270,20 @@ static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 	err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE,
 			   sizeof(*dice), &card);
 	if (err < 0)
-		goto end;
+		return err;
 
 	dice = card->private_data;
 	dice->card = card;
 	dice->unit = fw_unit_get(unit);
 	card->private_free = dice_card_free;
+	dev_set_drvdata(&unit->device, dice);
 
 	spin_lock_init(&dice->lock);
 	mutex_init(&dice->mutex);
 	init_completion(&dice->clock_accepted);
 	init_waitqueue_head(&dice->hwdep_wait);
 
-	err = snd_dice_transaction_init(dice);
-	if (err < 0)
-		goto error;
-
-	err = dice_read_params(dice);
-	if (err < 0)
-		goto error;
-
-	dice_card_strings(dice);
-
-	err = snd_dice_create_pcm(dice);
+	err = snd_dice_stream_init_duplex(dice);
 	if (err < 0)
 		goto error;
 
@@ -237,23 +293,11 @@ static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 
 	snd_dice_create_proc(dice);
 
-	err = snd_dice_create_midi(dice);
-	if (err < 0)
-		goto error;
-
-	err = snd_dice_stream_init_duplex(dice);
-	if (err < 0)
-		goto error;
+	/* Register this sound card later. */
+	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
+	schedule_registration(dice);
 
-	err = snd_card_register(card);
-	if (err < 0) {
-		snd_dice_stream_destroy_duplex(dice);
-		goto error;
-	}
-
-	dev_set_drvdata(&unit->device, dice);
-end:
-	return err;
+	return 0;
 error:
 	snd_card_free(card);
 	return err;
@@ -263,14 +307,25 @@ static void dice_remove(struct fw_unit *unit)
 {
 	struct snd_dice *dice = dev_get_drvdata(&unit->device);
 
+	/* For a race condition of struct snd_card.shutdown. */
+	mutex_lock(&dice->mutex);
+
 	/* No need to wait for releasing card object in this context. */
 	snd_card_free_when_closed(dice->card);
+
+	mutex_unlock(&dice->mutex);
 }
 
 static void dice_bus_reset(struct fw_unit *unit)
 {
 	struct snd_dice *dice = dev_get_drvdata(&unit->device);
 
+	/* Postpone a workqueue for deferred registration. */
+	if (!dice->registered) {
+		schedule_registration(dice);
+		return;
+	}
+
 	/* The handler address register becomes initialized. */
 	snd_dice_transaction_reinit(dice);
 
diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h
index 101550ac..3d5ebeb 100644
--- a/sound/firewire/dice/dice.h
+++ b/sound/firewire/dice/dice.h
@@ -45,6 +45,9 @@ struct snd_dice {
 	spinlock_t lock;
 	struct mutex mutex;
 
+	bool registered;
+	struct delayed_work dwork;
+
 	/* Offsets for sub-addresses */
 	unsigned int global_offset;
 	unsigned int rx_offset;
-- 
2.5.0

  parent reply	other threads:[~2015-12-23 22:55 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-23 22:55 [PATCH 0/4 v2] ALSA: dice: improve card registration processing Takashi Sakamoto
2015-12-23 22:55 ` [PATCH 1/4] ALSA: dice: split subaddress check from category check Takashi Sakamoto
2015-12-23 22:55 ` Takashi Sakamoto [this message]
2015-12-24  5:04   ` [PATCH 2/4] ALSA: dice: postpone card registration Takashi Sakamoto
2015-12-24 15:12   ` Stefan Richter
2015-12-24 19:10     ` Stefan Richter
2015-12-25  0:04       ` Takashi Sakamoto
2015-12-24 20:51   ` Stefan Richter
2015-12-24 21:04     ` Stefan Richter
2015-12-25  0:21     ` Takashi Sakamoto
2015-12-23 22:55 ` [PATCH 3/4] ALSA: dice: purge transaction initialization at timeout of Dice notification Takashi Sakamoto
2015-12-23 22:55 ` [PATCH 4/4] ALSA: dice: expand timeout to wait for " Takashi Sakamoto
  -- strict thread matches above, loose matches on Subject: below --
2015-12-31  4:58 [PATCH 0/4 v5] ALSA: dice: improve card registration processing Takashi Sakamoto
2015-12-31  4:58 ` [PATCH 2/4] ALSA: dice: postpone card registration Takashi Sakamoto
2015-12-22 13:45 [PATCH 0/4] ALSA: dice: improve card registration processing Takashi Sakamoto
2015-12-22 13:45 ` [PATCH 2/4] ALSA: dice: postpone card registration Takashi Sakamoto
2015-12-22 14:03   ` Takashi Iwai
2015-12-23  4:21     ` Takashi Sakamoto
2015-12-23  7:29       ` Takashi Iwai
2015-12-23  9:33         ` Takashi Sakamoto
2015-12-23 14:06           ` Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1450911310-15507-3-git-send-email-o-takashi@sakamocchi.jp \
    --to=o-takashi@sakamocchi.jp \
    --cc=alsa-devel@alsa-project.org \
    --cc=clemens@ladisch.de \
    --cc=ffado-devel@lists.sf.net \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.