Re: [PATCH 6/8] ALSA: emu10k1: add support for 2x/4x word clocks in E-MU D.A.S. mode

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

On Tue, Jun 13, 2023 at 11:20:23AM +0200, Takashi Iwai wrote:
>On Tue, 13 Jun 2023 09:38:20 +0200,
>Oswald Buddenhagen wrote:
>> 
>> Notably, add_ctls() now uses snd_ctl_add_locked(), so it doesn't
>> deadlock when called from snd_emu1010_clock_shift_put(). This also
>> affects the initial creation of the controls, which is OK, as that is
>> done before the card is registered, so no concurrent access can occur.
>
>Creating and removing the controls from kctl put callback is no good
>idea.  In general, dynamic control creation/deletion already confuses
>user-space.
>
i kind of expected that, but what i've tried so far worked remarkably 
well (ok, it was mostly alsamixer).

> On top of that, if it's done by a control element, it can
>be even triggered endlessly by user.
>
it shouldn't, because there is no circularity between the controls. 
even if the app sets all controls as a response to new ones appearing, 
the second round will be a no-op for the multiplier control, and 
therefore causes no new creattion/deletion notifications, and thus 
terminates the recursion.

but suppose a sufficiently broken application exists. then causing it to 
fail still seems quite acceptable: this is effectively new hardware (the 
new mode needs to be enabled manually), so it would be simply 
unsupported by the application until that gets fixed.

>A safer approach would be to create controls statically, and set
>active flag dynamically, I suppose.
>
i wanted to do that, but the problem is that not only the number of 
controls changes, but also the number of enum values in each control, as 
there is no way to make particular enum values inactive.
and i didn't want to keep three whole sets of controls around at all 
times, as that seems a bit wasteful.

also, i don't think that disabling would be fundamentally different from 
deleting: the particular code paths taken are somewhat different, but 
the high-level view is essentially the same. so we can't really make 
predictions which one would work better.

>And, if we really have to create / delete a kctl element from some
>kctl action, don't do it in the callback but process in another work.
>
would that really improve anything? for the notification to be received 
before the ioctl returns, it would have to be watched by a different 
thread. but if the app thought that there is a race, it would have to 
take the lock before issuing the ioctl anyway. so i think for user space 
it doesn't matter when exactly the notifications are emitted.

otoh, making the mixer reorganization async would introduce rather 
significant complexity to the driver due to having to deal with ioctls 
that come in while the inconsistent state persists (which seems likely 
during a state restoration).

so i would _really_ prefer to keep things as they are, and think about 
changing them only once we have hard evidence that the approach is too 
problematic.

regards,
ossi