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

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

On De 25 2015 05:51, Stefan Richter wrote:
> On Dec 24 Takashi Sakamoto wrote:
>> --- a/sound/firewire/dice/dice.c
>> +++ b/sound/firewire/dice/dice.c
> [...]
>>  static void dice_bus_reset(struct fw_unit *unit)
>>  {
>>  	struct snd_dice *dice = dev_get_drvdata(&unit->device);
>>  
>> +	/* Postpone a workqueue for deferred registration. */
>> +	if (!dice->registered) {
>> +		schedule_registration(dice);
>> +		return;
>> +	}
>> +
>>  	/* The handler address register becomes initialized. */
>>  	snd_dice_transaction_reinit(dice);
>>  
> 
> In previous versions of the patch, you used
> 	schedule_delayed_work(&dice->dwork, delay);
> in dice_probe() and
> 	mod_delayed_work(dice->dwork.wq, &dice->dwork, delay);
> in dice_bus_reset().
> 
> Now you are using schedule_delayed_work() in both.  This means that
> dice_bus_reset() is now unable to further defer the work.  Is this
> intentional?

No intention, just my mistake. Originally, I had an intention to use
'mod_delayed_work()' here.
https://www.kernel.org/doc/htmldocs/device-drivers/API-mod-delayed-work.html

> By the way, in drivers/firewire/core-cdev.c, I used a somewhat different
> workqueue scheduling scheme.  Problem:
>   - The first 1000 ms after a bus reset are to be used for re-allocations
>     of previously allocated isochronous resources.  Attempts for new iso
>     resource allocations shall be deferred until after 1000 ms after the
>     latest bus reset.
> My solution:
>   - The work which is to perform allocations/ reallocations/ deallocations
>     is scheduled immediately.  But the worker function (iso_resource_work)
>     reschedules itself if it notices that (1.) its job is to allocate and
>     (2.) the last bus reset was less than 1 s ago.
> 
> I used the same principle in drivers/firewire/core-card.c.  Problem:
>   - If system software wants to reset the bus, it shall wait at least
>     2000 ms until after the last bus reset.  The reason is to allow for
>     various bus reset handling protocols to be performed first (e.g.
>     isochronous resource allocations, re-logins and the likes).
> My solution:
>   - br_work() reschedules itself if it detects that the last bus reset was
>     less than 2 s ago.
> 
> Admittedly there is a small remaining window after the worker looked at
> card->reset_jiffies and before it performs its real work, and another bus
> rest could happen in this window.  I suppose if I wanted to close even
> this window, I would have to couple card->reset_jiffies with a
> bus generation counter and then make the remaining work depended on this
> bus generation.
> 
> Back to your patch:  I am not sure how strictly you want to guarantee the
> delay between last reset and do_registration()'s execution.  Maybe it
> would be beneficial to put the check for card->reset_jiffies and self-
> rescheduling into do_registration(), similar to the two examples from
> firewire-core, or maybe that's not really necessary for your purposes.

On Dec 25 2015 06:04, Stefan Richter wrote:
> Or as I wrote in the other thread:
> Maybe you can live with less precision and simply use a fixed relative
> delay, disregarding card->reset_jiffies altogether.

For the case that users execute insmod/rmmod by their own. In this case,
no need to be delay for the registration work.



Thanks

Takashi Sakamoto