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
