| Message ID | 20230207191907.467756-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: core: Make some functions return void | expand |
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
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>
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
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