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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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