Differential D50393
snd_hda: Fix a sporadic panic during kldunload Authored by tijl on May 17 2025, 12:33 PM. Tags None Referenced Files
Subscribers
Details The interrupt handler releases the device lock in hdaa_stream_intr to In the module Makefile sort hdac.c after hdacc.c so hdac_detach is PR: 286385
Diff Detail
Event TimelineComment Actions I didn't know about the Makefile thing. Does the sorting really affect the order in which the detach routines will be called? Comment Actions Yes, when unloading a kernel module the drivers defined with DRIVER_MODULE appear to be deleted in reverse order. Comment Actions Order, though, is really undefined... Drivers have to be prepared for any order, so this patch is, at best, a bandaid that respect. The draining before the detach is good, however. Comment Actions I haven't tested this for a few days, but I remember that even without the Makefile change, things worked as expected. Is this not the case? Comment Actions There are 2 different panics:
The first is fixed without the Makefile change. It happens when an interrupt occurs after the child hdacc device has been deleted by the bus_generic_detach call in hdac_detach. The second happens when an interrupt occurs earlier, during hdaa_pcm_detach. The detach thread holds the channel lock and destroys it in chn_kill while the interrupt thread tries to acquire that lock in chn_intr. I can't reproduce this panic but afaict the Makefile change is needed so hdac_detach is called before hdaa_pcm_detach so the draining happens first. Comment Actions For me the panics are fixed with this patch, but I'll wait for @imp and the others reviewers with regards to the Makefile change. Comment Actions The makefile change is dubious at best. It's a poor substitute for using DRIVER_MODULE_ORDERED which will be deterministic and not depend on details of how the static linker (ld.bfd or ld.lld) work. The other change also seems a bit dubious. Usually you want to detach child devices first, and child device routines should fully clean themselves up from the parent. This sounds like a bug in the parent driver. When the child driver is detached and tears down its interrupt handler, the parent needs to safely handle that case of an interrupt occurring without an attached child. You can get into this today if you just use devctl to detach the child device and leave the parent device around for example. I haven't looked at how the parent/child interrupt thing is working here, but if the parent bus has a custom bus_setup_intr/bus_teardown_intr that attaches the child's interrupt handler to the interrupt handler for the "real" interrupt from the child, then it sounds like some missing synchronization between the "real" interrupt handler in the parent and the custom bus_teardown_intr method. I think if you fix that bug you won't need the DRIVER_MODULE_ORDERED change either. Comment Actions Use DRIVER_MODULE_ORDERED Let hdac_detach take down the interrupt handler before detaching child devices and order hdac_driver so hdac_detach is called first. Remove duplicate hdac_if.h from the module Makefile. Comment Actions A snd_hda driver is split into multiple child devices but there's only one PCI device with one interrupt handler. The child devices are just the way the driver organizes itself internally but they aren't real devices if you ask me. They all share the same lock for example. The bus_*_intr interface isn't used. Comment Actions LGTM. I tried hot-unloading and I haven't come across any panic so far. Since the Makefile paragraph in the commit message can be removed now. |