[PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Takashi Iwai]

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(-)

-- 
2.26.2

[PATCH 1/2] ALSA: compress: Drop unused functions

[Takashi Iwai]

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");
-- 
2.26.2

[PATCH 2/2] ALSA: compress: Initialize mutex in snd_compress_new()

[Takashi Iwai]

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) {
-- 
2.26.2

Re: [PATCH 2/2] ALSA: compress: Initialize mutex in snd_compress_new()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Vinod Koul]

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>

-- 
~Vinod

Re: [PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Péter Ujfalusi]

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>
> 

-- 
Péter

Re: [PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Ujfalusi, Peter]

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

-- 
Péter

Re: [PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Takashi Iwai]

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

Re: [PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Péter Ujfalusi]

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

Re: [PATCH 0/2] ALSA: compress: Cleanup and a potential fix

[Takashi Iwai]

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