Re: [PATCH] ALSA: control_led: Fix deadlock between sysfs writes and card disconnect

[Takashi Iwai <tiwai@suse.de>]

On Sun, 11 Oct 2026 20:06:17 +0200,
Ali Ahmet Memiş wrote:
> 
> snd_ctl_led_disconnect() removes the ctl-led sysfs files while
> holding card->controls_rwsem.

Hm, it's not clear to me how this happens: I see no controls_rwsem
down around snd_ctl_led_disconnect() call.  Could you elaborate about
it a bit more?


thanks,

Takashi

> device_unregister() waits for running
> sysfs callbacks which can deadlock when a concurrent write causes
> the attach or detach callback to wait for controls_rwsem.
> 
> Break kernfs active protection in the attach, detach and list
> callbacks and hold a module reference instead.
> 
> Fixes: a135dfb5de15 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Ali Ahmet Memiş <aliamemis@disroot.org>
> ---
>  sound/core/control_led.c | 62 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index ec0e6c7ad..501387287 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -563,12 +563,43 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
>  	return count;
>  }
>  
> +/*
> + * snd_ctl_led_disconnect() removes these files with card->controls_rwsem
> + * held. so don't let the removal wait for a callback that takes it.
> + */
> +static struct kernfs_node *snd_ctl_led_sysfs_enter(struct device *dev,
> +						   struct device_attribute *attr)
> +{
> +	struct kernfs_node *kn;
> +
> +	if (!try_module_get(THIS_MODULE))
> +		return NULL;
> +	kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
> +	if (!kn)
> +		module_put(THIS_MODULE);
> +	return kn;
> +}
> +
> +static void snd_ctl_led_sysfs_exit(struct kernfs_node *kn)
> +{
> +	sysfs_unbreak_active_protection(kn);
> +	module_put(THIS_MODULE);
> +}
> +
>  static ssize_t attach_store(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf, size_t count)
>  {
>  	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> -	return set_led_id(led_card, buf, count, true);
> +	struct kernfs_node *kn;
> +	ssize_t ret;
> +
> +	kn = snd_ctl_led_sysfs_enter(dev, attr);
> +	if (!kn)
> +		return -ENODEV;
> +	ret = set_led_id(led_card, buf, count, true);
> +	snd_ctl_led_sysfs_exit(kn);
> +	return ret;
>  }
>  
>  static ssize_t detach_store(struct device *dev,
> @@ -576,7 +607,15 @@ static ssize_t detach_store(struct device *dev,
>  			    const char *buf, size_t count)
>  {
>  	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> -	return set_led_id(led_card, buf, count, false);
> +	struct kernfs_node *kn;
> +	ssize_t ret;
> +
> +	kn = snd_ctl_led_sysfs_enter(dev, attr);
> +	if (!kn)
> +		return -ENODEV;
> +	ret = set_led_id(led_card, buf, count, false);
> +	snd_ctl_led_sysfs_exit(kn);
> +	return ret;
>  }
>  
>  static ssize_t reset_store(struct device *dev,
> @@ -594,10 +633,8 @@ static ssize_t reset_store(struct device *dev,
>  	return count;
>  }
>  
> -static ssize_t list_show(struct device *dev,
> -			 struct device_attribute *attr, char *buf)
> +static ssize_t snd_ctl_led_list(struct snd_ctl_led_card *led_card, char *buf)
>  {
> -	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
>  	struct snd_ctl_led_ctl *lctl;
>  	size_t l = 0;
>  	struct snd_card *card __free(snd_card_unref) =
> @@ -620,6 +657,21 @@ static ssize_t list_show(struct device *dev,
>  	return l;
>  }
>  
> +static ssize_t list_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> +	struct kernfs_node *kn;
> +	ssize_t ret;
> +
> +	kn = snd_ctl_led_sysfs_enter(dev, attr);
> +	if (!kn)
> +		return -ENODEV;
> +	ret = snd_ctl_led_list(led_card, buf);
> +	snd_ctl_led_sysfs_exit(kn);
> +	return ret;
> +}
> +
>  static DEVICE_ATTR_WO(attach);
>  static DEVICE_ATTR_WO(detach);
>  static DEVICE_ATTR_WO(reset);
> -- 
> 2.55.0
>