[PATCH 0/3] ALSA: core: Make some functions return void
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
All callers from other files ignore the return value of this function. And it can only ever return a non-zero value if the parameter card is NULL.
Move the check for card being NULL into snd_card_free_when_closed() to keep the previous behaviour. Note this isn't necessary for snd_card_disconnect_sync() because if card was NULL in there the dereference of card for dev_err() would oops the kernel. Replace this by an oops triggered by the dereference of card for spin_lock_irq().
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- include/sound/core.h | 2 +- sound/core/init.c | 24 ++++++++---------------- 2 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h index 4365c35d038b..9a73c60b6f1e 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -286,7 +286,7 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid, struct module *module, size_t extra_size, struct snd_card **card_ret);
-int snd_card_disconnect(struct snd_card *card); +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); int snd_card_free_when_closed(struct snd_card *card); diff --git a/sound/core/init.c b/sound/core/init.c index 5377f94eb211..a03eddab12fe 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -489,17 +489,17 @@ static const struct file_operations snd_shutdown_f_ops = * Note: The current implementation replaces all active file->f_op with special * dummy file operations (they do nothing except release). */ -int snd_card_disconnect(struct snd_card *card) +void snd_card_disconnect(struct snd_card *card) { struct snd_monitor_file *mfile;
if (!card) - return -EINVAL; + return;
spin_lock(&card->files_lock); if (card->shutdown) { spin_unlock(&card->files_lock); - return 0; + return; } card->shutdown = 1;
@@ -548,7 +548,6 @@ int snd_card_disconnect(struct snd_card *card) wake_up(&card->power_sleep); snd_power_sync_ref(card); #endif - return 0; } EXPORT_SYMBOL(snd_card_disconnect);
@@ -563,15 +562,7 @@ EXPORT_SYMBOL(snd_card_disconnect); */ void snd_card_disconnect_sync(struct snd_card *card) { - int err; - - err = snd_card_disconnect(card); - if (err < 0) { - dev_err(card->dev, - "snd_card_disconnect error (%d), skipping sync\n", - err); - return; - } + snd_card_disconnect(card);
spin_lock_irq(&card->files_lock); wait_event_lock_irq(card->remove_sleep, @@ -619,9 +610,10 @@ static int snd_card_do_free(struct snd_card *card) */ int snd_card_free_when_closed(struct snd_card *card) { - int ret = snd_card_disconnect(card); - if (ret) - return ret; + if (!card) + return -EINVAL; + + snd_card_disconnect(card); put_device(&card->card_dev); return 0; }
All callers from other files ignore the return value of this function. And it can only ever return a non-zero value if the parameter card is NULL.
This cannot happen in snd_card_free() as card was dereferenced just before snd_card_free_when_closed() is called. So the error handling can be dropped there.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de --- include/sound/core.h | 2 +- sound/core/init.c | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h index 9a73c60b6f1e..21884c979c17 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -289,7 +289,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); -int snd_card_free_when_closed(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); int snd_card_register(struct snd_card *card); diff --git a/sound/core/init.c b/sound/core/init.c index a03eddab12fe..6bb3e2b77971 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -608,14 +608,14 @@ static int snd_card_do_free(struct snd_card *card) * * Return: zero if successful, or a negative error code */ -int snd_card_free_when_closed(struct snd_card *card) +void snd_card_free_when_closed(struct snd_card *card) { if (!card) - return -EINVAL; + return;
snd_card_disconnect(card); put_device(&card->card_dev); - return 0; + return; } EXPORT_SYMBOL(snd_card_free_when_closed);
@@ -635,7 +635,6 @@ EXPORT_SYMBOL(snd_card_free_when_closed); int snd_card_free(struct snd_card *card) { DECLARE_COMPLETION_ONSTACK(released); - int ret;
/* The call of snd_card_free() is allowed from various code paths; * a manual call from the driver and the call via devres_free, and @@ -647,9 +646,8 @@ int snd_card_free(struct snd_card *card) return 0;
card->release_completion = &released; - ret = snd_card_free_when_closed(card); - if (ret) - return ret; + snd_card_free_when_closed(card); + /* wait, until all devices are ready for the free operation */ wait_for_completion(&released);
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,
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
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
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/?i...
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=fo...
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
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/?i...
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=fo...
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
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
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
participants (6)
-
Geoff Levand -
Jaroslav Kysela -
Takashi Iwai -
Takashi Sakamoto -
Thierry Reding -
Uwe Kleine-König