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

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

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