[PATCH 0/4 v2] ALSA: dice: improve card registration processing

[PATCH 0/4 v2] ALSA: dice: improve card registration processing

[Takashi Sakamoto]

Hi,

This patchset is to update my previous one:

[alsa-devel] [PATCH 0/4] ALSA: dice: improve card registration processing
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-December/102357.html

The main purpose of this patchset is to postpone card registration after
successive bus resets, to ensure the processing and related transactions.
This change has an advantage for ALSA dice driver to get proper information
about hardware, especially for Dice II based models.

Changes from v1:
 * fix jiffies64 handling bug 
 * rename some members and functions related to card registration.
  * 'struct snd_dice.probed' -> 'registered'
  * do_probe() -> do_registration()

Takashi Sakamoto (4):
  ALSA: dice: split subaddress check from category check
  ALSA: dice: postpone card registration
  ALSA: dice: purge transaction initialization at timeout of Dice
    notification
  ALSA: dice: expand timeout to wait for Dice notification

 sound/firewire/dice/dice-transaction.c | 122 ++++++++++++++--------
 sound/firewire/dice/dice.c             | 183 ++++++++++++++++-----------------
 sound/firewire/dice/dice.h             |   3 +
 3 files changed, 172 insertions(+), 136 deletions(-)

-- 
2.5.0

[PATCH 1/4] ALSA: dice: split subaddress check from category check

[Takashi Sakamoto]

Before allocating an instance of sound card, ALSA dice driver checks
chip_ID_hi in Bus information block of Config ROM, then also checks
subaddresses. The former operation reads cache of Config ROM in Linux
FireWire subsystem, while the latter operation sends read transaction.
The latter can be merged into initialization of transaction system.

This commit splits these two operations to reduce needless transactions
in probe processing.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice-transaction.c | 89 ++++++++++++++++++++++++++--------
 sound/firewire/dice/dice.c             | 72 +++------------------------
 2 files changed, 77 insertions(+), 84 deletions(-)

diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c
index aee7461..75a2125 100644
--- a/sound/firewire/dice/dice-transaction.c
+++ b/sound/firewire/dice/dice-transaction.c
@@ -331,39 +331,59 @@ int snd_dice_transaction_reinit(struct snd_dice *dice)
 	return register_notification_address(dice, false);
 }
 
-int snd_dice_transaction_init(struct snd_dice *dice)
+static int get_subaddrs(struct snd_dice *dice)
 {
-	struct fw_address_handler *handler = &dice->notification_handler;
+	static const int min_values[10] = {
+		10, 0x64 / 4,
+		10, 0x18 / 4,
+		10, 0x18 / 4,
+		0, 0,
+		0, 0,
+	};
 	__be32 *pointers;
+	u32 data;
+	unsigned int i;
 	int err;
 
-	/* Use the same way which dice_interface_check() does. */
-	pointers = kmalloc(sizeof(__be32) * 10, GFP_KERNEL);
+	pointers = kmalloc_array(ARRAY_SIZE(min_values), sizeof(__be32),
+				 GFP_KERNEL);
 	if (pointers == NULL)
 		return -ENOMEM;
 
-	/* Get offsets for sub-addresses */
+	/*
+	 * Check that the sub address spaces exist and are located inside the
+	 * private address space.  The minimum values are chosen so that all
+	 * minimally required registers are included.
+	 */
 	err = snd_fw_transaction(dice->unit, TCODE_READ_BLOCK_REQUEST,
-				 DICE_PRIVATE_SPACE,
-				 pointers, sizeof(__be32) * 10, 0);
+				 DICE_PRIVATE_SPACE, pointers,
+				 sizeof(__be32) * ARRAY_SIZE(min_values), 0);
 	if (err < 0)
 		goto end;
 
-	/* Allocation callback in address space over host controller */
-	handler->length = 4;
-	handler->address_callback = dice_notification;
-	handler->callback_data = dice;
-	err = fw_core_add_address_handler(handler, &fw_high_memory_region);
-	if (err < 0) {
-		handler->callback_data = NULL;
-		goto end;
+	for (i = 0; i < ARRAY_SIZE(min_values); ++i) {
+		data = be32_to_cpu(pointers[i]);
+		if (data < min_values[i] || data >= 0x40000) {
+			err = -ENODEV;
+			goto end;
+		}
 	}
 
-	/* Register the address space */
-	err = register_notification_address(dice, true);
-	if (err < 0) {
-		fw_core_remove_address_handler(handler);
-		handler->callback_data = NULL;
+	/*
+	 * Check that the implemented DICE driver specification major version
+	 * number matches.
+	 */
+	err = snd_fw_transaction(dice->unit, TCODE_READ_QUADLET_REQUEST,
+				 DICE_PRIVATE_SPACE +
+				 be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
+				 &data, sizeof(data), 0);
+	if (err < 0)
+		goto end;
+
+	if ((data & cpu_to_be32(0xff000000)) != cpu_to_be32(0x01000000)) {
+		dev_err(&dice->unit->device,
+			"unknown DICE version: 0x%08x\n", be32_to_cpu(data));
+		err = -ENODEV;
 		goto end;
 	}
 
@@ -380,3 +400,32 @@ end:
 	kfree(pointers);
 	return err;
 }
+
+int snd_dice_transaction_init(struct snd_dice *dice)
+{
+	struct fw_address_handler *handler = &dice->notification_handler;
+	int err;
+
+	err = get_subaddrs(dice);
+	if (err < 0)
+		return err;
+
+	/* Allocation callback in address space over host controller */
+	handler->length = 4;
+	handler->address_callback = dice_notification;
+	handler->callback_data = dice;
+	err = fw_core_add_address_handler(handler, &fw_high_memory_region);
+	if (err < 0) {
+		handler->callback_data = NULL;
+		return err;
+	}
+
+	/* Register the address space */
+	err = register_notification_address(dice, true);
+	if (err < 0) {
+		fw_core_remove_address_handler(handler);
+		handler->callback_data = NULL;
+	}
+
+	return err;
+}
diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 0cda05c..26271cc 100644
--- a/sound/firewire/dice/dice.c
+++ b/sound/firewire/dice/dice.c
@@ -18,27 +18,12 @@ MODULE_LICENSE("GPL v2");
 #define WEISS_CATEGORY_ID	0x00
 #define LOUD_CATEGORY_ID	0x10
 
-static int dice_interface_check(struct fw_unit *unit)
+static int check_dice_category(struct fw_unit *unit)
 {
-	static const int min_values[10] = {
-		10, 0x64 / 4,
-		10, 0x18 / 4,
-		10, 0x18 / 4,
-		0, 0,
-		0, 0,
-	};
 	struct fw_device *device = fw_parent_device(unit);
 	struct fw_csr_iterator it;
-	int key, val, vendor = -1, model = -1, err;
-	unsigned int category, i;
-	__be32 *pointers;
-	u32 value;
-	__be32 version;
-
-	pointers = kmalloc_array(ARRAY_SIZE(min_values), sizeof(__be32),
-				 GFP_KERNEL);
-	if (pointers == NULL)
-		return -ENOMEM;
+	int key, val, vendor = -1, model = -1;
+	unsigned int category;
 
 	/*
 	 * Check that GUID and unit directory are constructed according to DICE
@@ -64,51 +49,10 @@ static int dice_interface_check(struct fw_unit *unit)
 	else
 		category = DICE_CATEGORY_ID;
 	if (device->config_rom[3] != ((vendor << 8) | category) ||
-	    device->config_rom[4] >> 22 != model) {
-		err = -ENODEV;
-		goto end;
-	}
-
-	/*
-	 * Check that the sub address spaces exist and are located inside the
-	 * private address space.  The minimum values are chosen so that all
-	 * minimally required registers are included.
-	 */
-	err = snd_fw_transaction(unit, TCODE_READ_BLOCK_REQUEST,
-				 DICE_PRIVATE_SPACE, pointers,
-				 sizeof(__be32) * ARRAY_SIZE(min_values), 0);
-	if (err < 0) {
-		err = -ENODEV;
-		goto end;
-	}
-	for (i = 0; i < ARRAY_SIZE(min_values); ++i) {
-		value = be32_to_cpu(pointers[i]);
-		if (value < min_values[i] || value >= 0x40000) {
-			err = -ENODEV;
-			goto end;
-		}
-	}
+	    device->config_rom[4] >> 22 != model)
+		return -ENODEV;
 
-	/*
-	 * Check that the implemented DICE driver specification major version
-	 * number matches.
-	 */
-	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
-				 DICE_PRIVATE_SPACE +
-				 be32_to_cpu(pointers[0]) * 4 + GLOBAL_VERSION,
-				 &version, 4, 0);
-	if (err < 0) {
-		err = -ENODEV;
-		goto end;
-	}
-	if ((version & cpu_to_be32(0xff000000)) != cpu_to_be32(0x01000000)) {
-		dev_err(&unit->device,
-			"unknown DICE version: 0x%08x\n", be32_to_cpu(version));
-		err = -ENODEV;
-		goto end;
-	}
-end:
-	return err;
+	return 0;
 }
 
 static int highest_supported_mode_rate(struct snd_dice *dice,
@@ -254,9 +198,9 @@ static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 	struct snd_dice *dice;
 	int err;
 
-	err = dice_interface_check(unit);
+	err = check_dice_category(unit);
 	if (err < 0)
-		goto end;
+		return -ENODEV;
 
 	err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE,
 			   sizeof(*dice), &card);
-- 
2.5.0

[PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

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

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

Hi,

I had a mistake in this patch...

On Dec 24 2015 07:55, Takashi Sakamoto wrote:
> 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;
> +	}
> +

This 'return' is needless. The v1 patchset includes the same mistake.

>   	/* 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;

I'll post new patchset later. Sorry to reviewers...


Regards

Takashi Sakamoto

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Stefan Richter]

On Dec 24 Takashi Sakamoto wrote:
[...]
> 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.
[...]
> --- 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);

According to the kerneldoc comment at cancel_work_sync, this should
actually be cancel_delayed_work_sync(&dice->dwork);.

> +
>  	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);

So the worker needs to obtain &dice->mutex.  But...

[...]
> @@ -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);
>  }

...if I read snd_card_free_when_closed() and its surrounding code
correctly, then dice_card_free() is called from within this, which can
cause a deadlock.  Right?

If so, then it seems to me that cancel_delayed_work_sync(&dice->dwork)
should be moved out of dice_card_free() and be put at the beginning of
dice_remove().
-- 
Stefan Richter
-=====-===== ==-- ==---
http://arcgraph.de/sr/

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Stefan Richter]

On Dec 24 Stefan Richter wrote:
> On Dec 24 Takashi Sakamoto wrote:
> [...]
> > 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.  
> [...]
> > --- 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);  
> 
> According to the kerneldoc comment at cancel_work_sync, this should
> actually be cancel_delayed_work_sync(&dice->dwork);.
> 
> > +
> >  	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);
> >  }

Another comment to dice_card_free() which is aside from this patch:

You are calling mutex_destroy(&dice->mutex) here.  But if I am right in my
understanding that dice_card_free() is called from dice_remove(), then
this is wrong because dice_card_free() still holding the mutex.

I am not sure what the proper fix is.  Perhaps just never perform
mutex_destroy().

> > +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);  
> 
> So the worker needs to obtain &dice->mutex.  But...
> 
> [...]
> > @@ -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);
> >  }  
> 
> ...if I read snd_card_free_when_closed() and its surrounding code
> correctly, then dice_card_free() is called from within this, which can
> cause a deadlock.  Right?
> 
> If so, then it seems to me that cancel_delayed_work_sync(&dice->dwork)
> should be moved out of dice_card_free() and be put at the beginning of
> dice_remove().

-- 
Stefan Richter
-=====-===== ==-- ==---
http://arcgraph.de/sr/

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

On Dec 25 2015 04:10, Stefan Richter wrote:
> On Dec 24 Stefan Richter wrote:
>> On Dec 24 Takashi Sakamoto wrote:
>> [...]
>>> 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.  
>> [...]
>>> --- 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);  
>>
>> According to the kerneldoc comment at cancel_work_sync, this should
>> actually be cancel_delayed_work_sync(&dice->dwork);.
>>
>>> +
>>>  	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);
>>>  }
> 
> Another comment to dice_card_free() which is aside from this patch:
> 
> You are calling mutex_destroy(&dice->mutex) here.  But if I am right in my
> understanding that dice_card_free() is called from dice_remove(), then
> this is wrong because dice_card_free() still holding the mutex.
> 
> I am not sure what the proper fix is.  Perhaps just never perform
> mutex_destroy().

Indeed.

I decide to move the cancel_work_sync() to .remove callback.
(actually, it should be cancel_delayed_work_sync().) After .remove
callback, .update callback is not called anymore therefore no works are
schedulled for the registration.

Then, the mutex_lock()/mutex_unlock() is not need for this purpose. When
sound card is released, the work is confirmed to finished and
unschedulled anymore because .remove is called before.

>>> +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);  
>>
>> So the worker needs to obtain &dice->mutex.  But...
>>
>> [...]
>>> @@ -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);
>>>  }  
>>
>> ...if I read snd_card_free_when_closed() and its surrounding code
>> correctly, then dice_card_free() is called from within this, which can
>> cause a deadlock.  Right?
>>
>> If so, then it seems to me that cancel_delayed_work_sync(&dice->dwork)
>> should be moved out of dice_card_free() and be put at the beginning of
>> dice_remove().


Thanks

Takashi Sakamoto

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Stefan Richter]

On Dec 24 Takashi Sakamoto wrote:
> --- a/sound/firewire/dice/dice.c
> +++ b/sound/firewire/dice/dice.c
[...]
>  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);
>  

In previous versions of the patch, you used
	schedule_delayed_work(&dice->dwork, delay);
in dice_probe() and
	mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
in dice_bus_reset().

Now you are using schedule_delayed_work() in both.  This means that
dice_bus_reset() is now unable to further defer the work.  Is this
intentional?

By the way, in drivers/firewire/core-cdev.c, I used a somewhat different
workqueue scheduling scheme.  Problem:
  - The first 1000 ms after a bus reset are to be used for re-allocations
    of previously allocated isochronous resources.  Attempts for new iso
    resource allocations shall be deferred until after 1000 ms after the
    latest bus reset.
My solution:
  - The work which is to perform allocations/ reallocations/ deallocations
    is scheduled immediately.  But the worker function (iso_resource_work)
    reschedules itself if it notices that (1.) its job is to allocate and
    (2.) the last bus reset was less than 1 s ago.

I used the same principle in drivers/firewire/core-card.c.  Problem:
  - If system software wants to reset the bus, it shall wait at least
    2000 ms until after the last bus reset.  The reason is to allow for
    various bus reset handling protocols to be performed first (e.g.
    isochronous resource allocations, re-logins and the likes).
My solution:
  - br_work() reschedules itself if it detects that the last bus reset was
    less than 2 s ago.

Admittedly there is a small remaining window after the worker looked at
card->reset_jiffies and before it performs its real work, and another bus
rest could happen in this window.  I suppose if I wanted to close even
this window, I would have to couple card->reset_jiffies with a
bus generation counter and then make the remaining work depended on this
bus generation.

Back to your patch:  I am not sure how strictly you want to guarantee the
delay between last reset and do_registration()'s execution.  Maybe it
would be beneficial to put the check for card->reset_jiffies and self-
rescheduling into do_registration(), similar to the two examples from
firewire-core, or maybe that's not really necessary for your purposes.
-- 
Stefan Richter
-=====-===== ==-- ==---
http://arcgraph.de/sr/

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Stefan Richter]

On Dec 24 Stefan Richter wrote:
> I am not sure how strictly you want to guarantee the
> delay between last reset and do_registration()'s execution.  Maybe it
> would be beneficial to put the check for card->reset_jiffies and self-
> rescheduling into do_registration(), similar to the two examples from
> firewire-core, or maybe that's not really necessary for your purposes.

Or as I wrote in the other thread:
Maybe you can live with less precision and simply use a fixed relative
delay, disregarding card->reset_jiffies altogether.
-- 
Stefan Richter
-=====-===== ==-- ==---
http://arcgraph.de/sr/

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

On De 25 2015 05:51, Stefan Richter wrote:
> On Dec 24 Takashi Sakamoto wrote:
>> --- a/sound/firewire/dice/dice.c
>> +++ b/sound/firewire/dice/dice.c
> [...]
>>  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);
>>  
> 
> In previous versions of the patch, you used
> 	schedule_delayed_work(&dice->dwork, delay);
> in dice_probe() and
> 	mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
> in dice_bus_reset().
> 
> Now you are using schedule_delayed_work() in both.  This means that
> dice_bus_reset() is now unable to further defer the work.  Is this
> intentional?

No intention, just my mistake. Originally, I had an intention to use
'mod_delayed_work()' here.
https://www.kernel.org/doc/htmldocs/device-drivers/API-mod-delayed-work.html

> By the way, in drivers/firewire/core-cdev.c, I used a somewhat different
> workqueue scheduling scheme.  Problem:
>   - The first 1000 ms after a bus reset are to be used for re-allocations
>     of previously allocated isochronous resources.  Attempts for new iso
>     resource allocations shall be deferred until after 1000 ms after the
>     latest bus reset.
> My solution:
>   - The work which is to perform allocations/ reallocations/ deallocations
>     is scheduled immediately.  But the worker function (iso_resource_work)
>     reschedules itself if it notices that (1.) its job is to allocate and
>     (2.) the last bus reset was less than 1 s ago.
> 
> I used the same principle in drivers/firewire/core-card.c.  Problem:
>   - If system software wants to reset the bus, it shall wait at least
>     2000 ms until after the last bus reset.  The reason is to allow for
>     various bus reset handling protocols to be performed first (e.g.
>     isochronous resource allocations, re-logins and the likes).
> My solution:
>   - br_work() reschedules itself if it detects that the last bus reset was
>     less than 2 s ago.
> 
> Admittedly there is a small remaining window after the worker looked at
> card->reset_jiffies and before it performs its real work, and another bus
> rest could happen in this window.  I suppose if I wanted to close even
> this window, I would have to couple card->reset_jiffies with a
> bus generation counter and then make the remaining work depended on this
> bus generation.
> 
> Back to your patch:  I am not sure how strictly you want to guarantee the
> delay between last reset and do_registration()'s execution.  Maybe it
> would be beneficial to put the check for card->reset_jiffies and self-
> rescheduling into do_registration(), similar to the two examples from
> firewire-core, or maybe that's not really necessary for your purposes.

On Dec 25 2015 06:04, Stefan Richter wrote:
> Or as I wrote in the other thread:
> Maybe you can live with less precision and simply use a fixed relative
> delay, disregarding card->reset_jiffies altogether.

For the case that users execute insmod/rmmod by their own. In this case,
no need to be delay for the registration work.



Thanks

Takashi Sakamoto

[PATCH 3/4] ALSA: dice: purge transaction initialization at timeout of Dice notification

[Takashi Sakamoto]

In previous commit, card registration is processed under situation
with few bus reset. There's no need to add a workaround of transaction
re-initialization at timeout.

This commit purges the re-initialization.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice-transaction.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c
index 75a2125..d5f7de7 100644
--- a/sound/firewire/dice/dice-transaction.c
+++ b/sound/firewire/dice/dice-transaction.c
@@ -65,16 +65,15 @@ static unsigned int get_clock_info(struct snd_dice *dice, __be32 *info)
 static int set_clock_info(struct snd_dice *dice,
 			  unsigned int rate, unsigned int source)
 {
-	unsigned int retries = 3;
 	unsigned int i;
 	__be32 info;
 	u32 mask;
 	u32 clock;
 	int err;
-retry:
+
 	err = get_clock_info(dice, &info);
 	if (err < 0)
-		goto end;
+		return err;
 
 	clock = be32_to_cpu(info);
 	if (source != UINT_MAX) {
@@ -87,10 +86,8 @@ retry:
 			if (snd_dice_rates[i] == rate)
 				break;
 		}
-		if (i == ARRAY_SIZE(snd_dice_rates)) {
-			err = -EINVAL;
-			goto end;
-		}
+		if (i == ARRAY_SIZE(snd_dice_rates))
+			return -EINVAL;
 
 		mask = CLOCK_RATE_MASK;
 		clock &= ~mask;
@@ -104,25 +101,13 @@ retry:
 	err = snd_dice_transaction_write_global(dice, GLOBAL_CLOCK_SELECT,
 						&info, 4);
 	if (err < 0)
-		goto end;
+		return err;
 
-	/* Timeout means it's invalid request, probably bus reset occurred. */
 	if (wait_for_completion_timeout(&dice->clock_accepted,
-			msecs_to_jiffies(NOTIFICATION_TIMEOUT_MS)) == 0) {
-		if (retries-- == 0) {
-			err = -ETIMEDOUT;
-			goto end;
-		}
-
-		err = snd_dice_transaction_reinit(dice);
-		if (err < 0)
-			goto end;
+			msecs_to_jiffies(NOTIFICATION_TIMEOUT_MS)) == 0)
+		return -ETIMEDOUT;
 
-		msleep(500);	/* arbitrary */
-		goto retry;
-	}
-end:
-	return err;
+	return 0;
 }
 
 int snd_dice_transaction_get_clock_source(struct snd_dice *dice,
-- 
2.5.0

[PATCH 4/4] ALSA: dice: expand timeout to wait for Dice notification

[Takashi Sakamoto]

Some users have reported that their Dice based models generate ETIMEDOUT
when starting PCM playback. It means that current timeout (=100msec) is
not enough for their models to transfer notifications.

This commit expands the timeout up to 2 sec. As a result, in a worst case,
any operations to start AMDTP streams takes 2 sec or more. Then, in
userspace, snd_pcm_hw_params(), snd_pcm_prepare(), snd_pcm_recover(),
snd_rawmidi_open(), snd_seq_connect_from() and snd_seq_connect_to() may
take the time.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice-transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/dice/dice-transaction.c b/sound/firewire/dice/dice-transaction.c
index d5f7de7..de5cd6c 100644
--- a/sound/firewire/dice/dice-transaction.c
+++ b/sound/firewire/dice/dice-transaction.c
@@ -9,7 +9,7 @@
 
 #include "dice.h"
 
-#define NOTIFICATION_TIMEOUT_MS	100
+#define NOTIFICATION_TIMEOUT_MS	(2 * MSEC_PER_SEC)
 
 static u64 get_subaddr(struct snd_dice *dice, enum snd_dice_addr_type type,
 		       u64 offset)
-- 
2.5.0

[PATCH 0/4 v5] ALSA: dice: improve card registration processing

[Takashi Sakamoto]

Hi,

This patchset updates my previous one:
[alsa-devel] [PATCH 0/5 v4] ALSA: dice: improve card registration processing
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-December/102504.html

Changes:
 * Merge patch 02 and 03 in v4.
 * Allocate sound card instance in sound card registration work, independently
   of dice instance, allocated in .probe callback and set as driver_data of
   device structure.
 * Use __be32 type for __be32 data.

In this version, sound card instance is not allocated in .probe callback. In
the callback, dice instance is allocated independently of the card instance.
Thus, in .probe callback, this driver reserve no slot of sound card. The
allocation of sound card instance is performed in a scheduled work. When
processing in the work fails, the work can be re-scheduled again according to
bus reset.

Takashi Sakamoto (4):
  ALSA: dice: split subaddress check from category check
  ALSA: dice: postpone card registration
  ALSA: dice: purge transaction initialization at timeout of Dice
    notification
  ALSA: dice: expand timeout to wait for Dice notification

 sound/firewire/dice/dice-transaction.c | 123 +++++++++++-------
 sound/firewire/dice/dice.c             | 227 +++++++++++++++++----------------
 sound/firewire/dice/dice.h             |   3 +
 3 files changed, 202 insertions(+), 151 deletions(-)

-- 
2.5.0

[PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

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.

I use a loose strategy to manage a race condition between the work 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 work because the work includes to wait for the transaction. This
commit uses no lock primitives for the race condition. Instead, checking
'registered' member of 'struct snd_dice' avoid executing the work again.
If sound card is not registered, the work can be scheduled again by bus
reset handler.

When .remove callback is executed, the sound card is going to be released.
The work should not be pending or executed in the releasing. This commit
uses cancel_delayed_work_sync() in .remove callback and wait till the
pending work finished. After .remove callback, .update callback is not
executed, therefore no works are scheduled again.

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

diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 26271cc..f5e15c4 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);
@@ -175,6 +177,16 @@ static void dice_card_strings(struct snd_dice *dice)
 	strcpy(card->mixername, "DICE");
 }
 
+static void dice_free(struct snd_dice *dice)
+{
+	snd_dice_stream_destroy_duplex(dice);
+	snd_dice_transaction_destroy(dice);
+	fw_unit_put(dice->unit);
+
+	mutex_destroy(&dice->mutex);
+	kfree(dice);
+}
+
 /*
  * This module releases the FireWire unit data after all ALSA character devices
  * are released by applications. This is for releasing stream data or finishing
@@ -183,39 +195,21 @@ static void dice_card_strings(struct snd_dice *dice)
  */
 static void dice_card_free(struct snd_card *card)
 {
-	struct snd_dice *dice = card->private_data;
-
-	snd_dice_stream_destroy_duplex(dice);
-	snd_dice_transaction_destroy(dice);
-	fw_unit_put(dice->unit);
-
-	mutex_destroy(&dice->mutex);
+	dice_free(card->private_data);
 }
 
-static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
+static void do_registration(struct work_struct *work)
 {
-	struct snd_card *card;
-	struct snd_dice *dice;
+	struct snd_dice *dice = container_of(work, struct snd_dice, dwork.work);
 	int err;
 
-	err = check_dice_category(unit);
-	if (err < 0)
-		return -ENODEV;
+	if (dice->registered)
+		return;
 
-	err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE,
-			   sizeof(*dice), &card);
+	err = snd_card_new(&dice->unit->device, -1, NULL, THIS_MODULE, 0,
+			   &dice->card);
 	if (err < 0)
-		goto end;
-
-	dice = card->private_data;
-	dice->card = card;
-	dice->unit = fw_unit_get(unit);
-	card->private_free = dice_card_free;
-
-	spin_lock_init(&dice->lock);
-	mutex_init(&dice->mutex);
-	init_completion(&dice->clock_accepted);
-	init_waitqueue_head(&dice->hwdep_wait);
+		return;
 
 	err = snd_dice_transaction_init(dice);
 	if (err < 0)
@@ -227,56 +221,131 @@ static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 
 	dice_card_strings(dice);
 
+	snd_dice_create_proc(dice);
+
 	err = snd_dice_create_pcm(dice);
 	if (err < 0)
 		goto error;
 
-	err = snd_dice_create_hwdep(dice);
+	err = snd_dice_create_midi(dice);
 	if (err < 0)
 		goto error;
 
-	snd_dice_create_proc(dice);
-
-	err = snd_dice_create_midi(dice);
+	err = snd_dice_create_hwdep(dice);
 	if (err < 0)
 		goto error;
 
-	err = snd_dice_stream_init_duplex(dice);
+	err = snd_card_register(dice->card);
 	if (err < 0)
 		goto error;
 
-	err = snd_card_register(card);
+	/*
+	 * After registered, dice instance can be released corresponding to
+	 * releasing the sound card instance.
+	 */
+	dice->card->private_free = dice_card_free;
+	dice->card->private_data = dice;
+	dice->registered = true;
+
+	return;
+error:
+	snd_dice_transaction_destroy(dice);
+	snd_card_free(dice->card);
+	dev_info(&dice->unit->device,
+		 "Sound card registration failed: %d\n", err);
+}
+
+static 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;
+
+	mod_delayed_work(system_wq, &dice->dwork, delay);
+}
+
+static int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
+{
+	struct snd_dice *dice;
+	int err;
+
+	err = check_dice_category(unit);
+	if (err < 0)
+		return -ENODEV;
+
+	/* Allocate this independent of sound card instance. */
+	dice = kzalloc(sizeof(struct snd_dice), GFP_KERNEL);
+	if (dice == NULL)
+		return -ENOMEM;
+
+	dice->unit = fw_unit_get(unit);
+	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_stream_init_duplex(dice);
 	if (err < 0) {
-		snd_dice_stream_destroy_duplex(dice);
-		goto error;
+		dice_free(dice);
+		return err;
 	}
 
-	dev_set_drvdata(&unit->device, dice);
-end:
-	return err;
-error:
-	snd_card_free(card);
-	return err;
+	/* Allocate and register this sound card later. */
+	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
+	schedule_registration(dice);
+
+	return 0;
 }
 
 static void dice_remove(struct fw_unit *unit)
 {
 	struct snd_dice *dice = dev_get_drvdata(&unit->device);
 
-	/* No need to wait for releasing card object in this context. */
-	snd_card_free_when_closed(dice->card);
+	/*
+	 * Confirm to stop the work for registration before the sound card is
+	 * going to be released. The work is not scheduled again because bus
+	 * reset handler is not called anymore.
+	 */
+	cancel_delayed_work_sync(&dice->dwork);
+
+	if (dice->registered) {
+		/* No need to wait for releasing card object in this context. */
+		snd_card_free_when_closed(dice->card);
+	} else {
+		/* Don't forget this case. */
+		dice_free(dice);
+	}
 }
 
 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);
+
 	/* The handler address register becomes initialized. */
 	snd_dice_transaction_reinit(dice);
 
-	mutex_lock(&dice->mutex);
-	snd_dice_stream_update_duplex(dice);
-	mutex_unlock(&dice->mutex);
+	/*
+	 * After registration, userspace can start packet streaming, then this
+	 * code block works fine.
+	 */
+	if (dice->registered) {
+		mutex_lock(&dice->mutex);
+		snd_dice_stream_update_duplex(dice);
+		mutex_unlock(&dice->mutex);
+	}
 }
 
 #define DICE_INTERFACE	0x000001
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

[PATCH 0/4] ALSA: dice: improve card registration processing

[Takashi Sakamoto]

Hi,

This patchset is to update a part of my previous RFCv2 (patch 01, 03 and 04. 02 is dropped according to Clemens' comment).

[alsa-devel] [RFC][PATCH 00/10 v2] ALSA: dice: stabiliza packet streaming
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-December/101897.html

The main purpose of this patchset is to postpone card registration after
successive bus resets, to ensure probe processing and related transactions.
This change has effects for ALSA dice driver to get proper information about
hardware, especially for Dice II based models.

Changes from RFC v2:
 * Patch 03 is newly added to drop a workaround to ensure transactions under
   a situation of probe processing with successive bus reset.
 * Comment updates.

Takashi Sakamoto (4):
  ALSA: dice: split subaddress check from category check
  ALSA: dice: postpone card registration
  ALSA: dice: purge transaction initialization at timeout of Dice
    notification
  ALSA: dice: expand timeout to wait for Dice notification

 sound/firewire/dice/dice-transaction.c | 122 +++++++++++++++--------
 sound/firewire/dice/dice.c             | 177 ++++++++++++++++-----------------
 sound/firewire/dice/dice.h             |   3 +
 3 files changed, 166 insertions(+), 136 deletions(-)

-- 
2.5.0

[PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

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.

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

diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index 26271cc..afec02f 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,17 +187,68 @@ 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);
 
 	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->probed)
+		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->probed = 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 int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 {
+	struct fw_card *fw_card = fw_parent_device(unit)->card;
 	struct snd_card *card;
 	struct snd_dice *dice;
+	unsigned long delay;
 	int err;
 
 	err = check_dice_category(unit);
@@ -205,29 +258,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 +281,13 @@ 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;
-
-	err = snd_card_register(card);
-	if (err < 0) {
-		snd_dice_stream_destroy_duplex(dice);
-		goto error;
-	}
+	/* Register this sound card later. */
+	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
+	delay = msecs_to_jiffies(PROBE_DELAY_MS) +
+				fw_card->reset_jiffies - get_jiffies_64();
+	schedule_delayed_work(&dice->dwork, delay);
 
-	dev_set_drvdata(&unit->device, dice);
-end:
-	return err;
+	return 0;
 error:
 	snd_card_free(card);
 	return err;
@@ -263,13 +297,28 @@ 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);
+	struct fw_card *fw_card = fw_parent_device(unit)->card;
+	unsigned long delay;
+
+	/* Postpone a workqueue for deferred registration. */
+	if (!dice->probed) {
+		delay = msecs_to_jiffies(PROBE_DELAY_MS) -
+				(get_jiffies_64() - fw_card->reset_jiffies);
+		mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
+		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..4741113 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 probed;
+	struct delayed_work dwork;
+
 	/* Offsets for sub-addresses */
 	unsigned int global_offset;
 	unsigned int rx_offset;
-- 
2.5.0

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Iwai]

On Tue, 22 Dec 2015 14:45:41 +0100,
Takashi Sakamoto wrote:
> 
> 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.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/firewire/dice/dice.c | 105 +++++++++++++++++++++++++++++++++------------
>  sound/firewire/dice/dice.h |   3 ++
>  2 files changed, 80 insertions(+), 28 deletions(-)
> 
> diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
> index 26271cc..afec02f 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,17 +187,68 @@ 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);
>  
>  	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->probed)
> +		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->probed = 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 int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
>  {
> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
>  	struct snd_card *card;
>  	struct snd_dice *dice;
> +	unsigned long delay;
>  	int err;
>  
>  	err = check_dice_category(unit);
> @@ -205,29 +258,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 +281,13 @@ 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;
> -
> -	err = snd_card_register(card);
> -	if (err < 0) {
> -		snd_dice_stream_destroy_duplex(dice);
> -		goto error;
> -	}
> +	/* Register this sound card later. */
> +	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
> +	delay = msecs_to_jiffies(PROBE_DELAY_MS) +
> +				fw_card->reset_jiffies - get_jiffies_64();
> +	schedule_delayed_work(&dice->dwork, delay);

Missing negative jiffies check?


>  
> -	dev_set_drvdata(&unit->device, dice);
> -end:
> -	return err;
> +	return 0;
>  error:
>  	snd_card_free(card);
>  	return err;
> @@ -263,13 +297,28 @@ 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);
> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
> +	unsigned long delay;
> +
> +	/* Postpone a workqueue for deferred registration. */
> +	if (!dice->probed) {
> +		delay = msecs_to_jiffies(PROBE_DELAY_MS) -
> +				(get_jiffies_64() - fw_card->reset_jiffies);
> +		mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
> +		return;

No protection for race here?


Takashi

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

On Dec 22 2015 23:03, Takashi Iwai wrote:
> On Tue, 22 Dec 2015 14:45:41 +0100,
> Takashi Sakamoto wrote:
>>
>> 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.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/firewire/dice/dice.c | 105 +++++++++++++++++++++++++++++++++------------
>>  sound/firewire/dice/dice.h |   3 ++
>>  2 files changed, 80 insertions(+), 28 deletions(-)
>>
>> diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
>> index 26271cc..afec02f 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,17 +187,68 @@ 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);
>>  
>>  	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->probed)
>> +		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->probed = 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 int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
>>  {
>> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
>>  	struct snd_card *card;
>>  	struct snd_dice *dice;
>> +	unsigned long delay;
>>  	int err;
>>  
>>  	err = check_dice_category(unit);
>> @@ -205,29 +258,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 +281,13 @@ 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;
>> -
>> -	err = snd_card_register(card);
>> -	if (err < 0) {
>> -		snd_dice_stream_destroy_duplex(dice);
>> -		goto error;
>> -	}
>> +	/* Register this sound card later. */
>> +	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
>> +	delay = msecs_to_jiffies(PROBE_DELAY_MS) +
>> +				fw_card->reset_jiffies - get_jiffies_64();
>> +	schedule_delayed_work(&dice->dwork, delay);
> 
> Missing negative jiffies check?

Indeed. I forgot a case that users execute rmmod/insmod by theirselves
long after bus reset. In this case, the delay is assigned to minus
value, while it's unsigned type so it can have large number.

I'd like to add this function as a solution for this issue.

static inline void schedule_registration(struct snd_dice *dice)
{
  struct fw_card *fw_card = fw_parent_device(dice->unit)->card;
  u64 delay;

  delay = fw_card->reset_jiffies + msecs_to_jiffies(PROBE_DELAY_MS);
  if (time_after64(delay, get_jiffies_64()))
      delay -= get_jiffies_64();
  else
    delay = 0;

  schedule_delayed_work(&dice->dwork, delay);
}

>>  
>> -	dev_set_drvdata(&unit->device, dice);
>> -end:
>> -	return err;
>> +	return 0;
>>  error:
>>  	snd_card_free(card);
>>  	return err;
>> @@ -263,13 +297,28 @@ 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);
>> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
>> +	unsigned long delay;
>> +
>> +	/* Postpone a workqueue for deferred registration. */
>> +	if (!dice->probed) {
>> +		delay = msecs_to_jiffies(PROBE_DELAY_MS) -
>> +				(get_jiffies_64() - fw_card->reset_jiffies);
>> +		mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
>> +		return;
> 
> No protection for race here?

Both of state transition of workqueue and the flag of 'dice->probed'
work fine to manage the race condition.

This workqueue is kept each instance of IEEE 1394 unit driver, thus the
same work can not run concurrently for the same unit. When already
scheduled, the work is expectedly postponed by mod_delayed_work(). When
the work is running, mod_delayed_work() schedules next work, but the
flag of 'dice->probed' has an effect to return immediately in the next
work. When workqueue has already finished, the flag of 'dice->probed'
avoid rescheduling of the work.


Thank you

Takashi Sakamoto

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Iwai]

On Wed, 23 Dec 2015 05:21:34 +0100,
Takashi Sakamoto wrote:
> 
> On Dec 22 2015 23:03, Takashi Iwai wrote:
> > On Tue, 22 Dec 2015 14:45:41 +0100,
> > Takashi Sakamoto wrote:
> >>
> >> 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.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >> ---
> >>  sound/firewire/dice/dice.c | 105 +++++++++++++++++++++++++++++++++------------
> >>  sound/firewire/dice/dice.h |   3 ++
> >>  2 files changed, 80 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
> >> index 26271cc..afec02f 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,17 +187,68 @@ 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);
> >>  
> >>  	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->probed)
> >> +		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->probed = 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 int dice_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
> >>  {
> >> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
> >>  	struct snd_card *card;
> >>  	struct snd_dice *dice;
> >> +	unsigned long delay;
> >>  	int err;
> >>  
> >>  	err = check_dice_category(unit);
> >> @@ -205,29 +258,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 +281,13 @@ 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;
> >> -
> >> -	err = snd_card_register(card);
> >> -	if (err < 0) {
> >> -		snd_dice_stream_destroy_duplex(dice);
> >> -		goto error;
> >> -	}
> >> +	/* Register this sound card later. */
> >> +	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
> >> +	delay = msecs_to_jiffies(PROBE_DELAY_MS) +
> >> +				fw_card->reset_jiffies - get_jiffies_64();
> >> +	schedule_delayed_work(&dice->dwork, delay);
> > 
> > Missing negative jiffies check?
> 
> Indeed. I forgot a case that users execute rmmod/insmod by theirselves
> long after bus reset. In this case, the delay is assigned to minus
> value, while it's unsigned type so it can have large number.
> 
> I'd like to add this function as a solution for this issue.
> 
> static inline void schedule_registration(struct snd_dice *dice)
> {
>   struct fw_card *fw_card = fw_parent_device(dice->unit)->card;
>   u64 delay;
> 
>   delay = fw_card->reset_jiffies + msecs_to_jiffies(PROBE_DELAY_MS);
>   if (time_after64(delay, get_jiffies_64()))
>       delay -= get_jiffies_64();
>   else
>     delay = 0;

This is no good.  You shouldn't refer jiffies twice.  It may be
updated meanwhile.


>   schedule_delayed_work(&dice->dwork, delay);
> }
> 
> >>  
> >> -	dev_set_drvdata(&unit->device, dice);
> >> -end:
> >> -	return err;
> >> +	return 0;
> >>  error:
> >>  	snd_card_free(card);
> >>  	return err;
> >> @@ -263,13 +297,28 @@ 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);
> >> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
> >> +	unsigned long delay;
> >> +
> >> +	/* Postpone a workqueue for deferred registration. */
> >> +	if (!dice->probed) {
> >> +		delay = msecs_to_jiffies(PROBE_DELAY_MS) -
> >> +				(get_jiffies_64() - fw_card->reset_jiffies);
> >> +		mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
> >> +		return;
> > 
> > No protection for race here?
> 
> Both of state transition of workqueue and the flag of 'dice->probed'
> work fine to manage the race condition.
>
> This workqueue is kept each instance of IEEE 1394 unit driver, thus the
> same work can not run concurrently for the same unit.

But the race is against another (your own) work.  Without the
protection, you have no guarantee of the consistency between
dice->probed evaluation and the next mod_delayed_work() execution.


Takashi

> When already
> scheduled, the work is expectedly postponed by mod_delayed_work(). When
> the work is running, mod_delayed_work() schedules next work, but the
> flag of 'dice->probed' has an effect to return immediately in the next
> work. When workqueue has already finished, the flag of 'dice->probed'
> avoid rescheduling of the work.
> 
> 
> Thank you
> 
> Takashi Sakamoto
> 

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Sakamoto]

On 2015年12月23日 16:29, Takashi Iwai wrote:
>>>> +	/* Register this sound card later. */
>>>> +	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
>>>> +	delay = msecs_to_jiffies(PROBE_DELAY_MS) +
>>>> +				fw_card->reset_jiffies - get_jiffies_64();
>>>> +	schedule_delayed_work(&dice->dwork, delay);
>>>
>>> Missing negative jiffies check?
>>
>> Indeed. I forgot a case that users execute rmmod/insmod by theirselves
>> long after bus reset. In this case, the delay is assigned to minus
>> value, while it's unsigned type so it can have large number.
>>
>> I'd like to add this function as a solution for this issue.
>>
>> static inline void schedule_registration(struct snd_dice *dice)
>> {
>>   struct fw_card *fw_card = fw_parent_device(dice->unit)->card;
>>   u64 delay;
>>
>>   delay = fw_card->reset_jiffies + msecs_to_jiffies(PROBE_DELAY_MS);
>>   if (time_after64(delay, get_jiffies_64()))
>>       delay -= get_jiffies_64();
>>   else
>>     delay = 0;
> 
> This is no good.  You shouldn't refer jiffies twice.  It may be
> updated meanwhile.

Hm... There's no helper functions for atomic operation of jiffies64, so
no way except for using stack. How is this?

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);
}

>>   schedule_delayed_work(&dice->dwork, delay);
>> }
>>
>>>>  
>>>> -	dev_set_drvdata(&unit->device, dice);
>>>> -end:
>>>> -	return err;
>>>> +	return 0;
>>>>  error:
>>>>  	snd_card_free(card);
>>>>  	return err;
>>>> @@ -263,13 +297,28 @@ 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);
>>>> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
>>>> +	unsigned long delay;
>>>> +
>>>> +	/* Postpone a workqueue for deferred registration. */
>>>> +	if (!dice->probed) {
>>>> +		delay = msecs_to_jiffies(PROBE_DELAY_MS) -
>>>> +				(get_jiffies_64() - fw_card->reset_jiffies);
>>>> +		mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
>>>> +		return;
>>>
>>> No protection for race here?
>>
>> Both of state transition of workqueue and the flag of 'dice->probed'
>> work fine to manage the race condition.
>>
>> This workqueue is kept each instance of IEEE 1394 unit driver, thus the
>> same work can not run concurrently for the same unit.
> 
> But the race is against another (your own) work.  Without the
> protection, you have no guarantee of the consistency between
> dice->probed evaluation and the next mod_delayed_work() execution.

Yes. But in this case, below lines return from the work.

+static void do_registration(struct work_struct *work)
+{
+ ...
+	if (dice->card->shutdown || dice->probed)
+		goto end;

I use quite loose strategy to the race condition between unit's update
handler and the work, because of transaction re-initialization. The
transaction system should be initialized in advance of the work. When
some lock primitives are used, processing of the update handler is
delayed. It's a bit inconvinient to the work.


Thanks

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Iwai]

On Wed, 23 Dec 2015 10:33:15 +0100,
Takashi Sakamoto wrote:
> 
> On 2015年12月23日 16:29, Takashi Iwai wrote:
> >>>> +	/* Register this sound card later. */
> >>>> +	INIT_DEFERRABLE_WORK(&dice->dwork, do_registration);
> >>>> +	delay = msecs_to_jiffies(PROBE_DELAY_MS) +
> >>>> +				fw_card->reset_jiffies - get_jiffies_64();
> >>>> +	schedule_delayed_work(&dice->dwork, delay);
> >>>
> >>> Missing negative jiffies check?
> >>
> >> Indeed. I forgot a case that users execute rmmod/insmod by theirselves
> >> long after bus reset. In this case, the delay is assigned to minus
> >> value, while it's unsigned type so it can have large number.
> >>
> >> I'd like to add this function as a solution for this issue.
> >>
> >> static inline void schedule_registration(struct snd_dice *dice)
> >> {
> >>   struct fw_card *fw_card = fw_parent_device(dice->unit)->card;
> >>   u64 delay;
> >>
> >>   delay = fw_card->reset_jiffies + msecs_to_jiffies(PROBE_DELAY_MS);
> >>   if (time_after64(delay, get_jiffies_64()))
> >>       delay -= get_jiffies_64();
> >>   else
> >>     delay = 0;
> > 
> > This is no good.  You shouldn't refer jiffies twice.  It may be
> > updated meanwhile.
> 
> Hm... There's no helper functions for atomic operation of jiffies64, so
> no way except for using stack. How is this?
> 
> 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);
> }

Yes, it looks better.


> >>   schedule_delayed_work(&dice->dwork, delay);
> >> }
> >>
> >>>>  
> >>>> -	dev_set_drvdata(&unit->device, dice);
> >>>> -end:
> >>>> -	return err;
> >>>> +	return 0;
> >>>>  error:
> >>>>  	snd_card_free(card);
> >>>>  	return err;
> >>>> @@ -263,13 +297,28 @@ 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);
> >>>> +	struct fw_card *fw_card = fw_parent_device(unit)->card;
> >>>> +	unsigned long delay;
> >>>> +
> >>>> +	/* Postpone a workqueue for deferred registration. */
> >>>> +	if (!dice->probed) {
> >>>> +		delay = msecs_to_jiffies(PROBE_DELAY_MS) -
> >>>> +				(get_jiffies_64() - fw_card->reset_jiffies);
> >>>> +		mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
> >>>> +		return;
> >>>
> >>> No protection for race here?
> >>
> >> Both of state transition of workqueue and the flag of 'dice->probed'
> >> work fine to manage the race condition.
> >>
> >> This workqueue is kept each instance of IEEE 1394 unit driver, thus the
> >> same work can not run concurrently for the same unit.
> > 
> > But the race is against another (your own) work.  Without the
> > protection, you have no guarantee of the consistency between
> > dice->probed evaluation and the next mod_delayed_work() execution.
> 
> Yes. But in this case, below lines return from the work.
> 
> +static void do_registration(struct work_struct *work)
> +{
> + ...
> +	if (dice->card->shutdown || dice->probed)
> +		goto end;
> 
> I use quite loose strategy to the race condition between unit's update
> handler and the work, because of transaction re-initialization. The
> transaction system should be initialized in advance of the work. When
> some lock primitives are used, processing of the update handler is
> delayed. It's a bit inconvinient to the work.

Fair enough, but then please describe it in the comment.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel