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