Re: [PATCH 3/3] ALSA: emu10k1: (re-)add mixer locking

[Takashi Iwai <tiwai@suse.de>]

On Thu, 13 Jul 2023 12:01:43 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Jul 13, 2023 at 11:21:38AM +0200, Takashi Iwai wrote:
> > On Thu, 13 Jul 2023 11:07:12 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> On Thu, Jul 13, 2023 at 10:33:26AM +0200, Takashi Iwai wrote:
> >> > instead of relying on a (hackish) big iron lock that wasn't
> >> > considered to be used originally at all.
> >> > i think you're focusing on the wrong thing here.
> >> the fact that the lock was originally meant to do something else is
> >> meaningless. you could just as well create a dedicated lock
> >> specifically for that task - the important thing is that the core
> >> would provide a guarantee to the drivers that mixer callbacks are
> >> locked, just like it does for some pcm callbacks unless the driver
> >> opts out. given that mixer operations are rare in the big picture,
> >> fine-grained locking in the drivers is unnecessary (except where not
> >> mixer-only data is accessed).  given the amount of code this saves,
> >> this seems like a rather worthwhile trade-off with the formal
> >> cleanness of drivers having self-contained locking.
> > 
> > My whole point is that no driver should touch card->controls_rwsem
> > from outside (unless the driver needs to traverse the card's linked
> > list by some special reasons).
> > 
> nothing in what i wrote even suggests that it _should_. how a driver
> would explicitly interact with _a_ mixer callback lock is entirely
> open so far.
> 
> > Unlike PCM, the control get/put has never been considered to be fully
> > protected,
> > 
> the whole argument is that it _should_.
> 
> > and it was always driver's responsibility.
> > 
> clearly a responsibility that has been widely shirked, even before it
> was actually safe to do so. the pragmatic thing to do would be
> accepting this reality and ensuring locking by the core, in whichever
> way.

Well, I took your patch 3 just because you wanted to have a protection
of your data from both get/put callback and from another code path in
another patch.  It was an (ab)use of controls->rwsem that couldn't be
accepted, so the patch 3 was taken as an alternative.

If this isn't the scenario, let me know: I'd rather drop the patch
again, as it's superfluous.

Again, my point is that you shouldn't use controls_rwsem for the
driver's data protection purpose.

There's many rooms for improvements in ALSA core, and things may
change.  So, if the driver needs to protect its own data from both
mixer code path and from another, use the own lock, instead of
touching controls_rwsem (which is basically an internal stuff for
ALSA control core).


Takashi