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

[Uwe Kleine-König]

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 at 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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20230208/8b7e99e7/attachment.sig>