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