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().