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