[alsa-devel] [PATCH 2/4] ALSA: dice: postpone card registration

[Takashi Iwai]

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