Re: [PATCH] ALSA: jack: Access input_dev under mutex

[Takashi Iwai <tiwai@suse.de>]

On Fri, 01 Apr 2022 14:29:31 +0200,
Amadeusz Sławiński wrote:
> 
> @@ -517,11 +525,15 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>  		return -ENOMEM;
>  	}
>  
> -	/* don't creat input device for phantom jack */
> -	if (!phantom_jack) {
>  #ifdef CONFIG_SND_JACK_INPUT_DEV
> +	mutex_init(&jack->input_dev_lock);
> +
> +	/* don't create input device for phantom jack */
> +	if (!phantom_jack) {
>  		int i;
>  
> +		mutex_lock(&jack->input_dev_lock);
> +

This lock is superfluous.  The object is being created here and isn't
registered anywhere, so no one else can use it yet.
And moreover ....

>  		jack->input_dev = input_allocate_device();
>  		if (jack->input_dev == NULL) {
>  			err = -ENOMEM;

... you forgot unlock here, and this error path will lead to a
deadlock.


> @@ -537,8 +549,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>  				input_set_capability(jack->input_dev, EV_SW,
>  						     jack_switch_types[i]);
>  
> -#endif /* CONFIG_SND_JACK_INPUT_DEV */
> +		mutex_unlock(&jack->input_dev_lock);
>  	}
> +#endif /* CONFIG_SND_JACK_INPUT_DEV */
>  
>  	err = snd_device_new(card, SNDRV_DEV_JACK, jack, &ops);
>  	if (err < 0)
> @@ -556,7 +569,9 @@ int snd_jack_new(struct snd_card *card, const char *id, int type,
>  
>  fail_input:
>  #ifdef CONFIG_SND_JACK_INPUT_DEV
> +	mutex_lock(&jack->input_dev_lock);
>  	input_free_device(jack->input_dev);
> +	mutex_unlock(&jack->input_dev_lock);

Ditto, no need for mutex lock yet.

One thing I'm not sure is whether it's good to use mutex.
Basically snd_jack_report() is considered to be callable from
non-atomic context, too, so far, and we don't need to prohibit it.
OTOH, all current calls are in the sleepable context, so it's likely
no big problem.  But if we use mutex, it should be documented in
snd_jack_report() function, too.


thanks,

Takashi