Re: [PATCH 2/2] ALSA: emu10k1: track loss of external clock on E-MU cards

[Takashi Iwai <tiwai@suse.de>]

On Mon, 10 Jul 2023 19:34:29 +0200,
Oswald Buddenhagen wrote:
> 
> On Mon, Jul 10, 2023 at 05:05:06PM +0200, Takashi Iwai wrote:
> > On Mon, 10 Jul 2023 08:59:56 +0200,
> > Oswald Buddenhagen wrote:
> >> +	// We consider this a mixer setting, so use the mixer's lock
> >> +	down_write(&emu->card->controls_rwsem);
> > 
> > I really don't want to see the driver touching this lock.  It's
> > basically an internal stuff of ALSA core code.  And, as already
> > discussed, the controls_rwsem wasn't intended as a lock for the mixer
> > content protection originally.  It took over the role partially, but
> > the drivers shouldn't rely on it.
> > 
> i know that this is technically true, but i think that from a
> pragmatic point of view it just makes no sense.
> 
> if we are pedantic about it, we need to revert my 06405d8ee8c
> (emu10k1: remove now superfluous mixer locking) and do the opposite
> change, to add the technically missing locks. that's several tens of
> irq-safe spinlock operations in this driver alone. which are hundreds
> of bytes spent entirely pointlessly, because we _know_ that the
> operation is already locked, because it must be.

Yes, I'm for reintroducing the driver sepcific lock (it can be mutex,
too).

> so i think the most sensible approach is to just make it official,
> which is what my 37bb927d5bb (core: update comment on
> snd_card.controls_rwsem) actually works towards. at least i can't
> think of a reason not to do that, except for some puristic ideals.
> 
> if somebody actually finds a _good_ reason to change this and wants to
> embark on the mammoth task of proving that everything is still safe
> afterwards, they can just do so. adjusting this commit for the new
> reality wouldn't be hard or laborious.
> 
> > > +     snd_emu1010_update_clock(emu);
> > > +     downgrade_write(&emu->card->controls_rwsem);
> > > +     snd_ctl_build_ioff(&id, emu->ctl_clock_source, 0);
> > > +     snd_ctl_notify(emu->card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
> > > +     up_read(&emu->card->controls_rwsem);
> > 
> > Err, too ugly and unnecessary change.  snd_ctl_notify() doesn't take
> > the rwsem, and it can be called at any place without fiddling the
> > rwsem.  It's snd_ctl_notify_one() that needs the downgrade to read
> > (and maybe we should rename the function to be clearer).
> > 
> well, that lock is necessary exactly because we (ab-)use the rwsem as
> the general mixer lock, so we basically need to emulate the ioctl
> entry path, so a concurrent actual put ioctl doesn't blow up in our
> face. i actually agree that it's kinda ugly, but the argument remains
> the same - it just isn't worth doing it differently (we'd have to
> sprinkle around quite some reg_lock operations instead).

Again, snd_ctl_notify() itself doesn't need the rwsem lock at all.
It's snd_ctl_notify_one() that needs a more careful call pattern.

And, that ugly implementation is a thing to be improved in future in
ALSA core side.  If we have this in each driver, it'll be a
nightmare.  So, please don't follow it.


thanks,

Takashi