[PATCH 0/2] ALSA: compress: Cleanup and a potential fix
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Takashi
===
Takashi Iwai (2): ALSA: compress: Drop unused functions ALSA: compress: Initialize mutex in snd_compress_new()
include/sound/compress_driver.h | 2 - sound/core/compress_offload.c | 69 +-------------------------------- sound/soc/soc-compress.c | 1 - 3 files changed, 1 insertion(+), 71 deletions(-)
snd_compress_register() and snd_compress_deregister() API functions have been never used by in-tree drivers. Let's clean up the dead code.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/compress_driver.h | 2 - sound/core/compress_offload.c | 68 --------------------------------- 2 files changed, 70 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index 277087f635f3..d91289c6f00e 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -165,8 +165,6 @@ struct snd_compr { };
/* compress device register APIs */ -int snd_compress_register(struct snd_compr *device); -int snd_compress_deregister(struct snd_compr *device); int snd_compress_new(struct snd_card *card, int device, int type, const char *id, struct snd_compr *compr);
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 21ce4c056a92..ed5546ae300a 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -47,8 +47,6 @@ * driver should be able to register multiple nodes */
-static DEFINE_MUTEX(device_mutex); - struct snd_compr_file { unsigned long caps; struct snd_compr_stream stream; @@ -1193,72 +1191,6 @@ int snd_compress_new(struct snd_card *card, int device, } EXPORT_SYMBOL_GPL(snd_compress_new);
-static int snd_compress_add_device(struct snd_compr *device) -{ - int ret; - - if (!device->card) - return -EINVAL; - - /* register the card */ - ret = snd_card_register(device->card); - if (ret) - goto out; - return 0; - -out: - pr_err("failed with %d\n", ret); - return ret; - -} - -static int snd_compress_remove_device(struct snd_compr *device) -{ - return snd_card_free(device->card); -} - -/** - * snd_compress_register - register compressed device - * - * @device: compressed device to register - */ -int snd_compress_register(struct snd_compr *device) -{ - int retval; - - if (device->name == NULL || device->ops == NULL) - return -EINVAL; - - pr_debug("Registering compressed device %s\n", device->name); - if (snd_BUG_ON(!device->ops->open)) - return -EINVAL; - if (snd_BUG_ON(!device->ops->free)) - return -EINVAL; - if (snd_BUG_ON(!device->ops->set_params)) - return -EINVAL; - if (snd_BUG_ON(!device->ops->trigger)) - return -EINVAL; - - mutex_init(&device->lock); - - /* register a compressed card */ - mutex_lock(&device_mutex); - retval = snd_compress_add_device(device); - mutex_unlock(&device_mutex); - return retval; -} -EXPORT_SYMBOL_GPL(snd_compress_register); - -int snd_compress_deregister(struct snd_compr *device) -{ - pr_debug("Removing compressed device %s\n", device->name); - mutex_lock(&device_mutex); - snd_compress_remove_device(device); - mutex_unlock(&device_mutex); - return 0; -} -EXPORT_SYMBOL_GPL(snd_compress_deregister); - MODULE_DESCRIPTION("ALSA Compressed offload framework"); MODULE_AUTHOR("Vinod Koul vinod.koul@linux.intel.com"); MODULE_LICENSE("GPL v2");
Currently the snd_compr.lock mutex isn't initialized in the API functions although the lock is used many places in other code in compress offload API. It's because the object was expected to be initialized via snd_compress_register(), but this was never used by ASoC, which is the only user. Instead, ASoC initializes the mutex by itself, and this is error-prone.
This patch moves the mutex initialization into the more appropriate place, snd_compress_new(), for avoiding the missing init.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/compress_offload.c | 1 + sound/soc/soc-compress.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index ed5546ae300a..de514ec8c83d 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -1177,6 +1177,7 @@ int snd_compress_new(struct snd_card *card, int device, compr->card = card; compr->device = device; compr->direction = dirn; + mutex_init(&compr->lock);
snd_compress_set_id(compr, id);
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index b4f59350a5a8..36060800e9bd 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -604,7 +604,6 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) break; }
- mutex_init(&compr->lock); ret = snd_compress_new(rtd->card->snd_card, num, direction, new_name, compr); if (ret < 0) {
On Wed, Jul 14, 2021 at 06:24:24PM +0200, Takashi Iwai wrote:
Currently the snd_compr.lock mutex isn't initialized in the API functions although the lock is used many places in other code in compress offload API. It's because the object was expected to be initialized via snd_compress_register(), but this was never used by ASoC, which is the only user. Instead, ASoC initializes the mutex by itself, and this is error-prone.
Reviewed-by: Mark Brown broonie@kernel.org
On 14-07-21, 18:24, Takashi Iwai wrote:
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Thanks for the cleanup. We never have a compress only device and it is a mix of PCM and compress FEs mostly :)
So:
Acked-By: Vinod Koul vkoul@kernel.org
On 15/07/2021 06:39, Vinod Koul wrote:
On 14-07-21, 18:24, Takashi Iwai wrote:
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Thanks for the cleanup. We never have a compress only device and it is a mix of PCM and compress FEs mostly :)
Actually I'm working on a series where a card would only contain nothing else but a compr, no PCMs...
So:
Acked-By: Vinod Koul vkoul@kernel.org
On 7/15/2021 10:51, Péter Ujfalusi wrote:
On 15/07/2021 06:39, Vinod Koul wrote:
On 14-07-21, 18:24, Takashi Iwai wrote:
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Thanks for the cleanup. We never have a compress only device and it is a mix of PCM and compress FEs mostly :)
Actually I'm working on a series where a card would only contain nothing else but a compr, no PCMs...
and I should have replied from here ;)
I'm preparing to split up the SOF into IPC clients [1] and the probes support (which is using compress API) is going to be moved as a separate client, creating it's own sound card with the single comprCxD0 (and the controlCx).
Is this cleanup going to prevent this?
[1] https://github.com/thesofproject/linux/pull/3007
On Thu, 15 Jul 2021 09:56:59 +0200, Ujfalusi, Peter wrote:
On 7/15/2021 10:51, Péter Ujfalusi wrote:
On 15/07/2021 06:39, Vinod Koul wrote:
On 14-07-21, 18:24, Takashi Iwai wrote:
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Thanks for the cleanup. We never have a compress only device and it is a mix of PCM and compress FEs mostly :)
Actually I'm working on a series where a card would only contain nothing else but a compr, no PCMs...
and I should have replied from here ;)
I'm preparing to split up the SOF into IPC clients [1] and the probes support (which is using compress API) is going to be moved as a separate client, creating it's own sound card with the single comprCxD0 (and the controlCx).
Is this cleanup going to prevent this?
Unless you use snd_compress_register() function, nothing can break. And if you were to use the function together with ASoC, it must be very wrong :)
Takashi
On 7/15/2021 11:03, Takashi Iwai wrote:
On Thu, 15 Jul 2021 09:56:59 +0200, Ujfalusi, Peter wrote:
On 7/15/2021 10:51, Péter Ujfalusi wrote:
On 15/07/2021 06:39, Vinod Koul wrote:
On 14-07-21, 18:24, Takashi Iwai wrote:
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Thanks for the cleanup. We never have a compress only device and it is a mix of PCM and compress FEs mostly :)
Actually I'm working on a series where a card would only contain nothing else but a compr, no PCMs...
and I should have replied from here ;)
I'm preparing to split up the SOF into IPC clients [1] and the probes support (which is using compress API) is going to be moved as a separate client, creating it's own sound card with the single comprCxD0 (and the controlCx).
Is this cleanup going to prevent this?
Unless you use snd_compress_register() function, nothing can break. And if you were to use the function together with ASoC, it must be very wrong :)
Yes, I just checked the patch ;)
and since I just did that: Reviewed-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com
Thank you, Péter
On Wed, 14 Jul 2021 18:24:22 +0200, Takashi Iwai wrote:
Hi,
this series contains a couple of patches for compress-offload API, one for a cleanup of unused API functions and another for the proper mutex initialization. I stumbled on the second while doing the first.
Takashi
===
Takashi Iwai (2): ALSA: compress: Drop unused functions ALSA: compress: Initialize mutex in snd_compress_new()
Applied now to for-next branch.
Takashi
participants (6)
-
Mark Brown -
Péter Ujfalusi -
Péter Ujfalusi -
Takashi Iwai -
Ujfalusi, Peter
-
Vinod Koul