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

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

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/