kern/58163: dead branches in dtv_demux_close

[campbell+netbsd%mumble.net@localhost]

>Number:         58163
>Category:       kern
>Synopsis:       dead branches in dtv_demux_close
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Apr 17 17:05:00 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The NetDTV Foundation
>Environment:
>Description:
dtv_demux_close, a struct fileops::fo_close function, has a branch for the event of dtv_demux_stop failure:

    341 	/* If the demux is still running, stop it */
    342 	if (demux->dd_running) {
    343 		error = dtv_demux_stop(demux);
    344 		if (error)
    345 			return error;
    346 	}

This early return skips over releasing resources:

    353 	mutex_destroy(&demux->dd_lock);
    354 	cv_destroy(&demux->dd_section_cv);
    355 	kmem_free(demux, sizeof(*demux));
    356 
    357 	/* Update the global device open count */
    358 	dtv_common_close(sc);

https://nxr.netbsd.org/xref/src/sys/dev/dtv/dtv_demux.c?r=1.11#341

This would be a bug if dtv_demux_stop could fail, because .fo_close is final; there will never be a second call to .fo_close, so any failure here is necessarily a memory leak.

But dtv_demux_stop only fails if dtv_device_stop_transfer fails, and every struct dtv_hw_if::stop_transfer function unconditionally returns zero:

https://nxr.netbsd.org/xref/src/sys/dev/pci/coram.c?r=1.20#716
https://nxr.netbsd.org/xref/src/sys/dev/pci/cxdtv.c?r=1.22#637
https://nxr.netbsd.org/xref/src/sys/dev/usb/auvitek_dtv.c?r=1.10#262
https://nxr.netbsd.org/xref/src/sys/dev/usb/emdtv_dtv.c?r=1.17#343

Rather than maintain these buggy dead branches, .stop_transfer should just be made to return void and all this logic be made unconditional.
>How-To-Repeat:
code inspection
>Fix:
1. make struct dtv_hw_if::stop_transfer return void
2. prune dead branches
3. review this code for MP-safety while here, access to dd_running looks dodgy -- maybe dtv_demux_close should just call dtv_demux_stop unconditionally