mbox series

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

Message ID 20210215172418.1322825-1-perex@perex.cz (mailing list archive)
Headers show
Series ALSA: control - add generic LED API | expand

Message

Jaroslav Kysela Feb. 15, 2021, 5:24 p.m. UTC
This patchset tries to resolve the diversity in the audio LED
control among the ALSA drivers. A new control layer registration
is introduced which allows to run additional operations on
top of the elementary ALSA sound controls.

A new control access group (three bits in the access flags)
was introduced to carry the LED group information for
the sound controls. The low-level sound drivers can just
mark those controls using this access group. This information
is exported to the user space and eventually the user space
can create sound controls which can belong to a LED group.

The actual state ('route') evaluation is really easy
(the minimal value check for all channels / controls / cards).
If there's more complicated logic for a given hardware,
the card driver may eventually export a new read-only
sound control for the LED group and do the logic itself.

The new LED trigger control code is completely separated
and possibly optional (there's no symbol dependency).
The full code separation allows eventually to move this
LED trigger control to the user space in future.
Actually it replaces the already present functionality
in the kernel space (HDA drivers) and allows a quick adoption
for the recent hardware (SoundWire / ASoC codecs).

# lsmod | grep snd_ctl_led
snd_ctl_led            16384  0

The sound driver implementation is really easy:

1) call snd_ctl_led_request() when control LED layer should be
   automatically activated
   / it calls module_request("snd-ctl-led") on demand /
2) mark all related kcontrols with
	SNDRV_CTL_ELEM_ACCESS_SPK_LED or
	SNDRV_CTL_ELEM_ACCESS_MIC_LED

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)
  - rename state to brightness and show the brightness
    unsigned integer value instead the text on/off string
    (sync with the LED core routines)
  - remove snd_ctl_led_hello() function (CI warning)
  - make snd_ctl_led_get_by_access() function static (CI warning)
  - move snd_ctl_layer_rwsem lock before the registraction
    callback call in snd_ctl_register_layer() - optimization
v1:
  - https://lore.kernel.org/alsa-devel/20210211111400.1131020-1-perex@perex.cz/
Original RFC:
  - https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Perry Yuan <Perry.Yuan@dell.com>

Jaroslav Kysela (5):
  ALSA: control - introduce snd_ctl_notify_one() helper
  ALSA: control - add layer registration routines
  ALSA: control - add generic LED trigger module as the new control
    layer
  ALSA: HDA - remove the custom implementation for the audio LED trigger
  ALSA: control - add sysfs support to the LED trigger module

 include/sound/control.h         |  27 ++-
 include/uapi/sound/asound.h     |   6 +-
 sound/core/Kconfig              |   6 +
 sound/core/Makefile             |   2 +
 sound/core/control.c            | 173 ++++++++++++---
 sound/core/control_led.c        | 378 ++++++++++++++++++++++++++++++++
 sound/pci/hda/Kconfig           |   4 +-
 sound/pci/hda/hda_codec.c       |  69 +-----
 sound/pci/hda/hda_generic.c     | 162 +++-----------
 sound/pci/hda/hda_generic.h     |  15 +-
 sound/pci/hda/hda_local.h       |  16 +-
 sound/pci/hda/patch_ca0132.c    |   4 +-
 sound/pci/hda/patch_realtek.c   |   2 +-
 sound/pci/hda/patch_sigmatel.c  |   6 +-
 sound/pci/hda/thinkpad_helper.c |   2 +-
 15 files changed, 609 insertions(+), 263 deletions(-)
 create mode 100644 sound/core/control_led.c

Comments

Hans de Goede Feb. 21, 2021, 1:14 p.m. UTC | #1
Hi,

On 2/15/21 6:24 PM, Jaroslav Kysela wrote:
> This patchset tries to resolve the diversity in the audio LED
> control among the ALSA drivers. A new control layer registration
> is introduced which allows to run additional operations on
> top of the elementary ALSA sound controls.
> 
> A new control access group (three bits in the access flags)
> was introduced to carry the LED group information for
> the sound controls. The low-level sound drivers can just
> mark those controls using this access group. This information
> is exported to the user space and eventually the user space
> can create sound controls which can belong to a LED group.
> 
> The actual state ('route') evaluation is really easy
> (the minimal value check for all channels / controls / cards).
> If there's more complicated logic for a given hardware,
> the card driver may eventually export a new read-only
> sound control for the LED group and do the logic itself.
> 
> The new LED trigger control code is completely separated
> and possibly optional (there's no symbol dependency).
> The full code separation allows eventually to move this
> LED trigger control to the user space in future.
> Actually it replaces the already present functionality
> in the kernel space (HDA drivers) and allows a quick adoption
> for the recent hardware (SoundWire / ASoC codecs).
> 
> # lsmod | grep snd_ctl_led
> snd_ctl_led            16384  0
> 
> The sound driver implementation is really easy:
> 
> 1) call snd_ctl_led_request() when control LED layer should be
>    automatically activated
>    / it calls module_request("snd-ctl-led") on demand /
> 2) mark all related kcontrols with
> 	SNDRV_CTL_ELEM_ACCESS_SPK_LED or
> 	SNDRV_CTL_ELEM_ACCESS_MIC_LED
> 
> 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.

In case you don't know, lockdep disables itself after catching
the first problem, to avoid spamming the logs too much.
So the previous issue was masking this one (or it could be new):

[   22.213898] ======================================================
[   22.213903] WARNING: possible circular locking dependency detected
[   22.213909] 5.11.0+ #255 Tainted: G            E
[   22.213914] ------------------------------------------------------
[   22.213918] systemd-udevd/390 is trying to acquire lock:
[   22.213923] ffff8a7f03d5a720 (&card->controls_rwsem){++++}-{3:3}, at: 0xffffffffc0861896
[   22.213948] 
               but task is already holding lock:
[   22.213952] ffffffffc07772b0 (snd_ctl_layer_rwsem){++++}-{3:3}, at: snd_ctl_register_layer+0x16b/0x360 [snd]
[   22.213977] 
               which lock already depends on the new lock.

[   22.213981] 
               the existing dependency chain (in reverse order) is:
[   22.213985] 
               -> #1 (snd_ctl_layer_rwsem){++++}-{3:3}:
[   22.213998]        down_read+0x40/0x50
[   22.214009]        snd_ctl_notify_one+0x8d/0x150 [snd]
[   22.214022]        snd_ctl_find_id+0x24e/0x350 [snd]
[   22.214034]        snd_ctl_find_id+0x2f3/0x350 [snd]
[   22.214047]        init_module+0x5bbab/0x5da91 [snd_hdmi_lpe_audio]
[   22.214057]        platform_probe+0x3f/0x90
[   22.214065]        really_probe+0xf2/0x440
[   22.214073]        driver_probe_device+0xe1/0x150
[   22.214080]        device_driver_attach+0xa8/0xb0
[   22.214087]        __driver_attach+0x8c/0x150
[   22.214095]        bus_for_each_dev+0x78/0xa0
[   22.214102]        bus_add_driver+0x12e/0x1f0
[   22.214109]        driver_register+0x8f/0xe0
[   22.214116]        do_one_initcall+0x6e/0x2e0
[   22.214125]        do_init_module+0x5c/0x260
[   22.214135]        load_module+0x2570/0x27e0
[   22.214143]        __do_sys_init_module+0x130/0x190
[   22.214151]        do_syscall_64+0x33/0x40
[   22.214158]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   22.214166]
               -> #0 (&card->controls_rwsem){++++}-{3:3}:
[   22.214179]        __lock_acquire+0x113d/0x1e10
[   22.214187]        lock_acquire+0xe4/0x390
[   22.214194]        down_read+0x40/0x50
[   22.214200]        0xffffffffc0861896
[   22.214207]        snd_ctl_register_layer+0x183/0x360 [snd]
[   22.214220]        snd_device_register_all+0x4c/0x60 [snd]
[   22.214232]        snd_card_register+0x74/0x1d0 [snd]
[   22.214244]        init_module+0x5bccb/0x5da91 [snd_hdmi_lpe_audio]
[   22.214253]        platform_probe+0x3f/0x90
[   22.214259]        really_probe+0xf2/0x440
[   22.214266]        driver_probe_device+0xe1/0x150
[   22.214273]        device_driver_attach+0xa8/0xb0
[   22.214281]        __driver_attach+0x8c/0x150
[   22.214288]        bus_for_each_dev+0x78/0xa0
[   22.214295]        bus_add_driver+0x12e/0x1f0
[   22.214302]        driver_register+0x8f/0xe0
[   22.214309]        do_one_initcall+0x6e/0x2e0
[   22.214316]        do_init_module+0x5c/0x260
[   22.214324]        load_module+0x2570/0x27e0
[   22.214332]        __do_sys_init_module+0x130/0x190
[   22.214339]        do_syscall_64+0x33/0x40
[   22.214346]        entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   22.214354] 
               other info that might help us debug this:

[   22.214357]  Possible unsafe locking scenario:

[   22.214361]        CPU0                    CPU1
[   22.214365]        ----                    ----
[   22.214368]   lock(snd_ctl_layer_rwsem);
[   22.214376]                                lock(&card->controls_rwsem);
[   22.214383]                                lock(snd_ctl_layer_rwsem);
[   22.214390]   lock(&card->controls_rwsem);
[   22.214397] 
                *** DEADLOCK ***
[   22.214401] 2 locks held by systemd-udevd/390:
[   22.214406]  #0: ffff8a7f0768f188 (&dev->mutex){....}-{3:3}, at: device_driver_attach+0x3b/0xb0
[   22.214426]  #1: ffffffffc07772b0 (snd_ctl_layer_rwsem){++++}-{3:3}, at: snd_ctl_register_layer+0x16b/0x360 [snd]
[   22.214450] 
               stack backtrace:
[   22.214455] CPU: 0 PID: 390 Comm: systemd-udevd Tainted: G            E     5.11.0+ #255
[   22.214463] Hardware name: HP HP x2 Detachable 10-p0XX/827C, BIOS F.11 09/30/2016
[   22.214468] Call Trace:
[   22.214477]  dump_stack+0x8b/0xb0
[   22.214491]  check_noncircular+0xfb/0x110
[   22.214505]  __lock_acquire+0x113d/0x1e10
[   22.214518]  lock_acquire+0xe4/0x390
[   22.214527]  ? 0xffffffffc0861896
[   22.214539]  down_read+0x40/0x50
[   22.214547]  ? 0xffffffffc0861896
[   22.214554]  0xffffffffc0861896
[   22.214562]  snd_ctl_register_layer+0x183/0x360 [snd]
[   22.214578]  snd_device_register_all+0x4c/0x60 [snd]
[   22.214593]  snd_card_register+0x74/0x1d0 [snd]
[   22.214608]  init_module+0x5bccb/0x5da91 [snd_hdmi_lpe_audio]
[   22.214624]  platform_probe+0x3f/0x90
[   22.214633]  really_probe+0xf2/0x440
[   22.214643]  driver_probe_device+0xe1/0x150
[   22.214652]  device_driver_attach+0xa8/0xb0
[   22.214662]  __driver_attach+0x8c/0x150
[   22.214670]  ? device_driver_attach+0xb0/0xb0
[   22.214678]  ? device_driver_attach+0xb0/0xb0
[   22.214687]  bus_for_each_dev+0x78/0xa0
[   22.214696]  bus_add_driver+0x12e/0x1f0
[   22.214706]  driver_register+0x8f/0xe0
[   22.214715]  ? 0xffffffffc06f5000
[   22.214721]  do_one_initcall+0x6e/0x2e0
[   22.214735]  do_init_module+0x5c/0x260
[   22.214745]  load_module+0x2570/0x27e0
[   22.214768]  ? __do_sys_init_module+0x130/0x190
[   22.214776]  __do_sys_init_module+0x130/0x190
[   22.214791]  do_syscall_64+0x33/0x40
[   22.214799]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   22.214809] RIP: 0033:0x7fbdcf3cc6be
[   22.214817] Code: 48 8b 0d bd 27 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8a 27
[   22.214825] RSP: 002b:00007ffea888abe8 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
[   22.214835] RAX: ffffffffffffffda RBX: 000055bdd044d9f0 RCX: 00007fbdcf3cc6be
[   22.214841] RDX: 00007fbdcf50235a RSI: 000000000000b1f8 RDI: 000055bdd0c549a0
[   22.214846] RBP: 000055bdd0c549a0 R08: 000055bdd0c549a0 R09: 00007ffea8887c88
[   22.214852] R10: 000055bdd0143010 R11: 0000000000000246 R12: 00007fbdcf50235a
[   22.214857] R13: 000055bdd042af40 R14: 0000000000000007 R15: 000055bdd0430bf0

I did not get around to testing your fixup-diff before you send
out v2 last time.

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.

Regards,

Hans
Jaroslav Kysela Feb. 21, 2021, 6:01 p.m. UTC | #2
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:

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
Hans de Goede Feb. 21, 2021, 7:32 p.m. UTC | #3
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
>