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@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
