A few ALSA control API helpers like snd_ctl_rename(), snd_ctl_remove() and snd_ctl_find_*() suppose the callers taking card->controls_rwsem. But it's error-prone and fragile. This patch set tries to change those API functions to take the card->controls>rwsem internally by themselves, so that the drivers don't need to take care of lockings.
After applying this patch set, only a couple of places still touch card->controls_rwsem (which are OK-ish as they need for traversing the control linked list).
Takashi
===
Takashi Iwai (11): ALSA: control: Take card->controls_rwsem in snd_ctl_rename() staging: greybus: audio_helper: Use snd_ctl_remove_id() ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id() ALSA: control: Take controls_rwsem lock in snd_ctl_remove() ALSA: control: Add lockdep warning to internal functions ASoC: sigmadsp: Simplify with snd_ctl_activate_id() staging: greybus: Avoid abusing controls_rwsem ALSA: control: Make snd_ctl_find_id() argument const ALSA: control: Introduce unlocked version for snd_ctl_find_*() helpers ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid() ALSA: emu10k1: Go back and simplify with snd_ctl_find_id()
drivers/staging/greybus/audio_codec.c | 18 ++-- drivers/staging/greybus/audio_codec.h | 1 + drivers/staging/greybus/audio_helper.c | 20 +--- include/sound/control.h | 6 +- sound/core/control.c | 126 ++++++++++++++++++++----- sound/core/control_compat.c | 2 +- sound/core/control_led.c | 2 +- sound/core/jack.c | 2 - sound/core/oss/mixer_oss.c | 10 +- sound/core/pcm.c | 2 - sound/isa/sb/emu8000.c | 2 - sound/isa/sb/sb16_csp.c | 2 - sound/pci/emu10k1/emufx.c | 5 - sound/pci/hda/hda_codec.c | 2 - sound/soc/atmel/mchp-pdmc.c | 12 +-- sound/soc/codecs/sigmadsp.c | 25 +---- sound/soc/soc-topology.c | 3 - 17 files changed, 129 insertions(+), 111 deletions(-)
Use the standard snd_ctl_remove_id() helper function instead of open code. This allows us to reduce the manual card->rwsem lock in the caller side.
Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: greybus-dev@lists.linaro.org Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/staging/greybus/audio_helper.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c index 223987616e07..97ce5b9ad7fd 100644 --- a/drivers/staging/greybus/audio_helper.c +++ b/drivers/staging/greybus/audio_helper.c @@ -149,7 +149,6 @@ static int gbaudio_remove_controls(struct snd_card *card, struct device *dev, for (i = 0; i < num_controls; i++) { const struct snd_kcontrol_new *control = &controls[i]; struct snd_ctl_elem_id id; - struct snd_kcontrol *kctl;
if (prefix) snprintf(id.name, sizeof(id.name), "%s %s", prefix, @@ -161,17 +160,10 @@ static int gbaudio_remove_controls(struct snd_card *card, struct device *dev, id.device = control->device; id.subdevice = control->subdevice; id.index = control->index; - kctl = snd_ctl_find_id(card, &id); - if (!kctl) { - dev_err(dev, "Failed to find %s\n", control->name); - continue; - } - err = snd_ctl_remove(card, kctl); - if (err < 0) { + err = snd_ctl_remove_id(card, &id); + if (err < 0) dev_err(dev, "%d: Failed to remove %s\n", err, control->name); - continue; - } } return 0; } @@ -181,11 +173,7 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component, unsigned int num_controls) { struct snd_card *card = component->card->snd_card; - int err;
- down_write(&card->controls_rwsem); - err = gbaudio_remove_controls(card, component->dev, controls, - num_controls, component->name_prefix); - up_write(&card->controls_rwsem); - return err; + return gbaudio_remove_controls(card, component->dev, controls, + num_controls, component->name_prefix); }
The controls_rwsem of snd_card object is rather an internal lock, and not really meant to be used by others for its data protection.
This patch addresses it by replacing the controls_rwsem usages with the own (new) mutex.
Note that the up_write() and down_write() calls around gbaudio_remove_component_controls() are simply dropped without replacement. These temporary up/down were a workaround since gbaudio_remove_component_controls() itself took the rwsem. Now it was also gone, we can clean up the workaround, too.
Cc: Vaibhav Agarwal vaibhav.sr@gmail.com Cc: Mark Greer mgreer@animalcreek.com Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: greybus-dev@lists.linaro.org Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/staging/greybus/audio_codec.c | 18 +++++++----------- drivers/staging/greybus/audio_codec.h | 1 + 2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c index 72ace74ea605..2f05e761fb9a 100644 --- a/drivers/staging/greybus/audio_codec.c +++ b/drivers/staging/greybus/audio_codec.c @@ -807,7 +807,6 @@ int gbaudio_register_module(struct gbaudio_module_info *module) { int ret; struct snd_soc_component *comp; - struct snd_card *card; struct gbaudio_jack *jack = NULL;
if (!gbcodec) { @@ -816,21 +815,20 @@ int gbaudio_register_module(struct gbaudio_module_info *module) }
comp = gbcodec->component; - card = comp->card->snd_card;
- down_write(&card->controls_rwsem); + mutex_lock(&gbcodec->register_mutex);
if (module->num_dais) { dev_err(gbcodec->dev, "%d:DAIs not supported via gbcodec driver\n", module->num_dais); - up_write(&card->controls_rwsem); + mutex_unlock(&gbcodec->register_mutex); return -EINVAL; }
ret = gbaudio_init_jack(module, comp->card); if (ret) { - up_write(&card->controls_rwsem); + mutex_unlock(&gbcodec->register_mutex); return ret; }
@@ -867,7 +865,7 @@ int gbaudio_register_module(struct gbaudio_module_info *module) ret = snd_soc_dapm_new_widgets(comp->card); dev_dbg(comp->dev, "Registered %s module\n", module->name);
- up_write(&card->controls_rwsem); + mutex_unlock(&gbcodec->register_mutex); return ret; } EXPORT_SYMBOL(gbaudio_register_module); @@ -935,13 +933,12 @@ static void gbaudio_codec_cleanup(struct gbaudio_module_info *module) void gbaudio_unregister_module(struct gbaudio_module_info *module) { struct snd_soc_component *comp = gbcodec->component; - struct snd_card *card = comp->card->snd_card; struct gbaudio_jack *jack, *n; int mask;
dev_dbg(comp->dev, "Unregister %s module\n", module->name);
- down_write(&card->controls_rwsem); + mutex_lock(&gbcodec->register_mutex); mutex_lock(&gbcodec->lock); gbaudio_codec_cleanup(module); list_del(&module->list); @@ -978,10 +975,8 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module) dev_dbg(comp->dev, "Removing %d controls\n", module->num_controls); /* release control semaphore */ - up_write(&card->controls_rwsem); gbaudio_remove_component_controls(comp, module->controls, module->num_controls); - down_write(&card->controls_rwsem); } if (module->dapm_widgets) { dev_dbg(comp->dev, "Removing %d widgets\n", @@ -992,7 +987,7 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module)
dev_dbg(comp->dev, "Unregistered %s module\n", module->name);
- up_write(&card->controls_rwsem); + mutex_unlock(&gbcodec->register_mutex); } EXPORT_SYMBOL(gbaudio_unregister_module);
@@ -1012,6 +1007,7 @@ static int gbcodec_probe(struct snd_soc_component *comp) info->dev = comp->dev; INIT_LIST_HEAD(&info->module_list); mutex_init(&info->lock); + mutex_init(&info->register_mutex); INIT_LIST_HEAD(&info->dai_list);
/* init dai_list used to maintain runtime stream info */ diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index ce15e800e607..f3f7a7ec6be4 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -71,6 +71,7 @@ struct gbaudio_codec_info { /* to maintain runtime stream params for each DAI */ struct list_head dai_list; struct mutex lock; + struct mutex register_mutex; };
struct gbaudio_widget {
On Tue, Jul 18, 2023 at 04:12:55PM +0200, Takashi Iwai wrote:
Use the standard snd_ctl_remove_id() helper function instead of open code. This allows us to reduce the manual card->rwsem lock in the caller side.
Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: greybus-dev@lists.linaro.org Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/audio_helper.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
On Tue, Jul 18, 2023 at 04:13:00PM +0200, Takashi Iwai wrote:
The controls_rwsem of snd_card object is rather an internal lock, and not really meant to be used by others for its data protection.
This patch addresses it by replacing the controls_rwsem usages with the own (new) mutex.
Note that the up_write() and down_write() calls around gbaudio_remove_component_controls() are simply dropped without replacement. These temporary up/down were a workaround since gbaudio_remove_component_controls() itself took the rwsem. Now it was also gone, we can clean up the workaround, too.
Cc: Vaibhav Agarwal vaibhav.sr@gmail.com Cc: Mark Greer mgreer@animalcreek.com Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: greybus-dev@lists.linaro.org Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/audio_codec.c | 18 +++++++----------- drivers/staging/greybus/audio_codec.h | 1 + 2 files changed, 8 insertions(+), 11 deletions(-)
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
On Tue, 18 Jul 2023 16:12:53 +0200, Takashi Iwai wrote:
A few ALSA control API helpers like snd_ctl_rename(), snd_ctl_remove() and snd_ctl_find_*() suppose the callers taking card->controls_rwsem. But it's error-prone and fragile. This patch set tries to change those API functions to take the card->controls>rwsem internally by themselves, so that the drivers don't need to take care of lockings.
After applying this patch set, only a couple of places still touch card->controls_rwsem (which are OK-ish as they need for traversing the control linked list).
Takashi
===
Takashi Iwai (11): ALSA: control: Take card->controls_rwsem in snd_ctl_rename() staging: greybus: audio_helper: Use snd_ctl_remove_id() ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id() ALSA: control: Take controls_rwsem lock in snd_ctl_remove() ALSA: control: Add lockdep warning to internal functions ASoC: sigmadsp: Simplify with snd_ctl_activate_id() staging: greybus: Avoid abusing controls_rwsem ALSA: control: Make snd_ctl_find_id() argument const ALSA: control: Introduce unlocked version for snd_ctl_find_*() helpers ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid() ALSA: emu10k1: Go back and simplify with snd_ctl_find_id()
Now all merged. The branch is based on 6.5-rc2, and tagged as ctl-lock-fixes-6.6 so that other branch can merge this if needed.
Takashi
-
Greg Kroah-Hartman -
Takashi Iwai