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 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