On Mon, 10 Jul 2023 08:59:56 +0200,
Oswald Buddenhagen wrote:
>
> +static void emu1010_clock_work(struct work_struct *work)
> +{
> + struct snd_emu10k1 *emu;
> + struct snd_ctl_elem_id id;
> +
> + emu = container_of(work, struct snd_emu10k1,
> + emu1010.clock_work);
> + if (emu->card->shutdown)
> + return;
> +#ifdef CONFIG_PM_SLEEP
> + if (emu->suspend)
> + return;
> +#endif
> +
> + // 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.
> + 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).
thanks,
Takashi
