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

[Takashi Iwai <tiwai@suse.de>]

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
>