[PATCH 0/3] ALSA: core: Make some functions return void

[PATCH 0/3] ALSA: core: Make some functions return void

[Uwe Kleine-König]

Hello,

while checking in which cases hda_tegra_remove() can return a non-zero value, I
found that actually cannot happen. This series makes the involved functions
return void to make this obvious.

This is a preparation for making platform_driver::remove return void, too.

Best regards
Uwe

Uwe Kleine-König (3):
  ALSA: core: Make snd_card_disconnect() return void
  ALSA: core: Make snd_card_free_when_closed() return void
  ALSA: core: Make snd_card_free() return void

 include/sound/core.h      |  6 +++---
 sound/core/init.c         | 40 ++++++++++++++-------------------------
 sound/pci/hda/hda_tegra.c |  6 ++----
 sound/ppc/snd_ps3.c       |  4 +---
 4 files changed, 20 insertions(+), 36 deletions(-)


base-commit: 05ecb680708a1dbe6554d6fc17e5d9a8a7cb5e6a
-- 
2.39.0

[PATCH 3/3] ALSA: core: Make snd_card_free() return void

[Uwe Kleine-König]

The function returns 0 unconditionally. Make it return void instead and
simplify all callers accordingly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 include/sound/core.h      | 2 +-
 sound/core/init.c         | 6 ++----
 sound/pci/hda/hda_tegra.c | 6 ++----
 sound/ppc/snd_ps3.c       | 4 +---
 4 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 21884c979c17..3edc4ab08774 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -288,7 +288,7 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid,
 
 void snd_card_disconnect(struct snd_card *card);
 void snd_card_disconnect_sync(struct snd_card *card);
-int snd_card_free(struct snd_card *card);
+void snd_card_free(struct snd_card *card);
 void snd_card_free_when_closed(struct snd_card *card);
 int snd_card_free_on_error(struct device *dev, int ret);
 void snd_card_set_id(struct snd_card *card, const char *id);
diff --git a/sound/core/init.c b/sound/core/init.c
index 6bb3e2b77971..df0c22480375 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -632,7 +632,7 @@ EXPORT_SYMBOL(snd_card_free_when_closed);
  * Return: Zero. Frees all associated devices and frees the control
  * interface associated to given soundcard.
  */
-int snd_card_free(struct snd_card *card)
+void snd_card_free(struct snd_card *card)
 {
 	DECLARE_COMPLETION_ONSTACK(released);
 
@@ -643,15 +643,13 @@ int snd_card_free(struct snd_card *card)
 	 * the check here at the beginning.
 	 */
 	if (card->releasing)
-		return 0;
+		return;
 
 	card->release_completion = &released;
 	snd_card_free_when_closed(card);
 
 	/* wait, until all devices are ready for the free operation */
 	wait_for_completion(&released);
-
-	return 0;
 }
 EXPORT_SYMBOL(snd_card_free);
 
diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 976a112c7d00..c2bf86781894 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -582,12 +582,10 @@ static void hda_tegra_probe_work(struct work_struct *work)
 
 static int hda_tegra_remove(struct platform_device *pdev)
 {
-	int ret;
-
-	ret = snd_card_free(dev_get_drvdata(&pdev->dev));
+	snd_card_free(dev_get_drvdata(&pdev->dev));
 	pm_runtime_disable(&pdev->dev);
 
-	return ret;
+	return 0;
 }
 
 static void hda_tegra_shutdown(struct platform_device *pdev)
diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c
index 631a61ce52f4..8d349231205e 100644
--- a/sound/ppc/snd_ps3.c
+++ b/sound/ppc/snd_ps3.c
@@ -1053,9 +1053,7 @@ static void snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
 	 * ctl and preallocate buffer will be freed in
 	 * snd_card_free
 	 */
-	ret = snd_card_free(the_card.card);
-	if (ret)
-		pr_info("%s: ctl freecard=%d\n", __func__, ret);
+	snd_card_free(the_card.card);
 
 	dma_free_coherent(&dev->core,
 			  PAGE_SIZE,
-- 
2.39.0

Re: [PATCH 3/3] ALSA: core: Make snd_card_free() return void

[Geoff Levand]

Hi Uwe,

On 2/7/23 11:19, Uwe Kleine-König wrote:
> The function returns 0 unconditionally. Make it return void instead and
> simplify all callers accordingly.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/sound/core.h      | 2 +-
>  sound/core/init.c         | 6 ++----
>  sound/pci/hda/hda_tegra.c | 6 ++----
>  sound/ppc/snd_ps3.c       | 4 +---
>  4 files changed, 6 insertions(+), 12 deletions(-)

> --- a/sound/ppc/snd_ps3.c
> +++ b/sound/ppc/snd_ps3.c
> @@ -1053,9 +1053,7 @@ static void snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
>  	 * ctl and preallocate buffer will be freed in
>  	 * snd_card_free
>  	 */
> -	ret = snd_card_free(the_card.card);
> -	if (ret)
> -		pr_info("%s: ctl freecard=%d\n", __func__, ret);
> +	snd_card_free(the_card.card);
>  
>  	dma_free_coherent(&dev->core,
>  			  PAGE_SIZE,

Looks OK for PS3.

Acked-by: Geoff Levand <geoff@infradead.org>

Re: [PATCH 3/3] ALSA: core: Make snd_card_free() return void

[Thierry Reding]

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

On Tue, Feb 07, 2023 at 08:19:07PM +0100, Uwe Kleine-König wrote:
> The function returns 0 unconditionally. Make it return void instead and
> simplify all callers accordingly.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  include/sound/core.h      | 2 +-
>  sound/core/init.c         | 6 ++----
>  sound/pci/hda/hda_tegra.c | 6 ++----
>  sound/ppc/snd_ps3.c       | 4 +---
>  4 files changed, 6 insertions(+), 12 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

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

Re: [PATCH 0/3] ALSA: core: Make some functions return void

[Takashi Sakamoto]

Hi,

On Tue, Feb 07, 2023 at 08:19:04PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> while checking in which cases hda_tegra_remove() can return a non-zero value, I
> found that actually cannot happen. This series makes the involved functions
> return void to make this obvious.
> 
> This is a preparation for making platform_driver::remove return void, too.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (3):
>   ALSA: core: Make snd_card_disconnect() return void
>   ALSA: core: Make snd_card_free_when_closed() return void
>   ALSA: core: Make snd_card_free() return void
> 
>  include/sound/core.h      |  6 +++---
>  sound/core/init.c         | 40 ++++++++++++++-------------------------
>  sound/pci/hda/hda_tegra.c |  6 ++----
>  sound/ppc/snd_ps3.c       |  4 +---
>  4 files changed, 20 insertions(+), 36 deletions(-)

All of the changes in the patches look good to me, as the return value
seems not to be so effective for a long time (15 years or more) and
expected so for future.
 
Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

One of my concern is that we are mostly in the last week for v6.3
development. When taking influence of the changes into account, it
would be possible to postpone to v6.4 development. But it's up to the
maintainer.


> base-commit: 05ecb680708a1dbe6554d6fc17e5d9a8a7cb5e6a

A small nitpicking. I think you use Linus' tree or so:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=05ecb680708a

Because the hash is not reachable from the branch for next merge window on
the tree of sound subsystem upstream:
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=for-next&id=05ecb680708a

I guess it is safer to use his tree as your work-base, even
if the three patches can be applied to it as is (fortunately).


Regards

Takashi Sakamoto

Re: [PATCH 0/3] ALSA: core: Make some functions return void

[Uwe Kleine-König]

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

Hello,

On Wed, Feb 08, 2023 at 05:33:48PM +0900, Takashi Sakamoto wrote:
> On Tue, Feb 07, 2023 at 08:19:04PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > while checking in which cases hda_tegra_remove() can return a non-zero value, I
> > found that actually cannot happen. This series makes the involved functions
> > return void to make this obvious.
> > 
> > This is a preparation for making platform_driver::remove return void, too.
> > 
> > Best regards
> > Uwe
> > 
> > Uwe Kleine-König (3):
> >   ALSA: core: Make snd_card_disconnect() return void
> >   ALSA: core: Make snd_card_free_when_closed() return void
> >   ALSA: core: Make snd_card_free() return void
> > 
> >  include/sound/core.h      |  6 +++---
> >  sound/core/init.c         | 40 ++++++++++++++-------------------------
> >  sound/pci/hda/hda_tegra.c |  6 ++----
> >  sound/ppc/snd_ps3.c       |  4 +---
> >  4 files changed, 20 insertions(+), 36 deletions(-)
> 
> All of the changes in the patches look good to me, as the return value
> seems not to be so effective for a long time (15 years or more) and
> expected so for future.

To give a more complete picture: Returning an error code in a cleanup
function does active harm. It makes driver authors expect that there
must be error handing and that they can/should return that error from
the remove function. This is wrong however and likely yields resource
leaks. See bbc126ae381cf0a27822c1f822d0aeed74cc40d9 for an example.

> Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> One of my concern is that we are mostly in the last week for v6.3
> development. When taking influence of the changes into account, it
> would be possible to postpone to v6.4 development. But it's up to the
> maintainer.

There is no rush from my side. Eventually I want to modify
platform_driver::remove which depends on this patch series, but the 6.4
merge window is fine for me, as this effort will likely take some more
time.

> > base-commit: 05ecb680708a1dbe6554d6fc17e5d9a8a7cb5e6a
> 
> A small nitpicking. I think you use Linus' tree or so:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=05ecb680708a
> 
> Because the hash is not reachable from the branch for next merge window on
> the tree of sound subsystem upstream:
> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=for-next&id=05ecb680708a
> 
> I guess it is safer to use his tree as your work-base, even
> if the three patches can be applied to it as is (fortunately).

I see your point. My reason not to do that is that finding out the right
tree (and branch) to base a patch on is not always trivial. As someone
who sends patches touching the whole tree this is a considerable
overhead and most of the time it's not worth the effort in my
experience. Even if I had based my patch on tiwai's tree, there might be
a patch still in flux that is picked up first and conflicts with my
change. Most of the time the patch still applies and stating the base is
just for the benefit of the auto builders (which might have problems
finding the base commit if it's not in next) and if it doesn't apply the
person picking up the patch probably knows about Linus' tree and so git
is helpful resolving the resulting conflict.

It's still more complicated for patches that might or might not be
considered a fix. Then it's up to the maintainer if they apply it to
their fixes or next branch. (And that situation is just about to happen
as I found a problem in a driver I touched here. So stay tuned :-)

So I gave up thinking too much about the right base and take the
opportunistic route of just using Linus' tree (or the last -rc1) and if
there really a merge conflict pops up, support the maintainer only then.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 0/3] ALSA: core: Make some functions return void

[Jaroslav Kysela]

On 07. 02. 23 20:19, Uwe Kleine-König wrote:
> Hello,
> 
> while checking in which cases hda_tegra_remove() can return a non-zero value, I
> found that actually cannot happen. This series makes the involved functions
> return void to make this obvious.
> 
> This is a preparation for making platform_driver::remove return void, too.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (3):
>    ALSA: core: Make snd_card_disconnect() return void
>    ALSA: core: Make snd_card_free_when_closed() return void
>    ALSA: core: Make snd_card_free() return void
> 
>   include/sound/core.h      |  6 +++---
>   sound/core/init.c         | 40 ++++++++++++++-------------------------
>   sound/pci/hda/hda_tegra.c |  6 ++----
>   sound/ppc/snd_ps3.c       |  4 +---
>   4 files changed, 20 insertions(+), 36 deletions(-)

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 0/3] ALSA: core: Make some functions return void

[Takashi Iwai]

On Tue, 07 Feb 2023 20:19:04 +0100,
Uwe Kleine-König wrote:
> 
> Hello,
> 
> while checking in which cases hda_tegra_remove() can return a non-zero value, I
> found that actually cannot happen. This series makes the involved functions
> return void to make this obvious.
> 
> This is a preparation for making platform_driver::remove return void, too.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (3):
>   ALSA: core: Make snd_card_disconnect() return void
>   ALSA: core: Make snd_card_free_when_closed() return void
>   ALSA: core: Make snd_card_free() return void

Applied all three patches now.  Thanks.


Takashi