On Tue, 13 Jun 2023 12:52:43 +0200, Oswald Buddenhagen wrote: > > 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. Hmm I don't get it; if an application just toggles the kctl value between two values in an infinite loop, it'll delete and recreate kctls endlessly as well with your patch, no? > 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. Creating and deleting needs a lot of different works and much heavier tasks. And, above all, many user-space programs will be borked if an element goes away, simply crashing. Some (rather rare) nice ones will still survive, though. I've learned this from the past. > > 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? As a primary reason, I don't want to expose such a stuff. If you need such an unlocked version, you're already doing something very exotic, and in 99% cases, it's something that needs more care. Takashi > 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 >
