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
