Re: [PATCH v2 0/5] ALSA: control - add generic LED API

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/21/21 7:01 PM, Jaroslav Kysela wrote:
> Dne 21. 02. 21 v 14:14 Hans de Goede napsal(a):
> 
>>> v2 changes:
>>>   - fix the locking - remove the controls_rwsem read lock
>>>     in the element get (the consistency is already protected
>>>     with the global snd_ctl_led_mutex and possible partial
>>>     value writes are catched with the next value change
>>>     notification callback)
>>
>> I'm afraid that lockdep still is unhappy. With v2 there is a new
>> (different) lockdep warning.
> 
>> If you can send me another fixup-diff then I'll make sure to
>> test this before you do a v3, so that we can be sure that
>> all cases which my setup catches are resolved before sending
>> out v3.
> 
> Thank you for your test. This change (on top of v2) should resolve this
> remaining lockdep:

I can confirm that I'm not seeing any more lockdeps warning after
applying this on top of v2 of the series.

Regards,

Hans



> diff --git a/sound/core/control.c b/sound/core/control.c
> index c9f062fada0a..494f0154e8be 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -2051,7 +2051,9 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops)
>  	for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
>  		card = snd_card_ref(card_number);
>  		if (card) {
> +			down_read(&card->controls_rwsem);
>  			lops->lregister(card);
> +			up_read(&card->controls_rwsem);
>  			snd_card_unref(card);
>  		}
>  	}
> @@ -2113,10 +2115,12 @@ static int snd_ctl_dev_register(struct snd_device *device)
>  				  &snd_ctl_f_ops, card, &card->ctl_dev);
>  	if (err < 0)
>  		return err;
> +	down_read(&card->controls_rwsem);
>  	down_read(&snd_ctl_layer_rwsem);
>  	for (lops = snd_ctl_layer; lops; lops = lops->next)
>  		lops->lregister(card);
>  	up_read(&snd_ctl_layer_rwsem);
> +	up_read(&card->controls_rwsem);
>  	return 0;
>  }
> 
> @@ -2137,10 +2141,12 @@ static int snd_ctl_dev_disconnect(struct snd_device
> *device)
>  	}
>  	read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
> 
> +	down_read(&card->controls_rwsem);
>  	down_read(&snd_ctl_layer_rwsem);
>  	for (lops = snd_ctl_layer; lops; lops = lops->next)
>  		lops->ldisconnect(card);
>  	up_read(&snd_ctl_layer_rwsem);
> +	up_read(&card->controls_rwsem);
> 
>  	return snd_unregister_device(&card->ctl_dev);
>  }
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index cafe4c82ca35..b8bb8fd46686 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -235,11 +235,10 @@ static void snd_ctl_led_register(struct snd_card *card)
>  	mutex_lock(&snd_ctl_led_mutex);
>  	snd_ctl_led_card_valid[card->number] = true;
>  	mutex_unlock(&snd_ctl_led_mutex);
> -	down_read(&card->controls_rwsem);
> +	/* the register callback is already called with held card->controls_rwsem */
>  	list_for_each_entry(kctl, &card->controls, list)
>  		for (ioff = 0; ioff < kctl->count; ioff++)
>  			snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff);
> -	up_read(&card->controls_rwsem);
>  	snd_ctl_led_refresh();
>  }
> 
> 
> 					Jaroslav
>