The branch main has been updated by bz: URL: https://cgit.FreeBSD.org/src/commit/?id=66b25ddf9125b2f3707e0f22b01b47bdff463fa7
commit 66b25ddf9125b2f3707e0f22b01b47bdff463fa7 Author: Bjoern A. Zeeb <[email protected]> AuthorDate: 2026-06-01 23:59:01 +0000 Commit: Bjoern A. Zeeb <[email protected]> CommitDate: 2026-07-13 14:58:23 +0000 LinuxKPI: pci detach: implement a proper detach (release) path There are two paths in the LinuxKPI PCI code to instantiate a "pdev" (LinuxKPI pci_dev). One is using the FreeBSD bus framework and the pdev will be the softc. This commit starts cleaning up the detach path for just that case to the best possible. So far we did a lot of the work in linux_pci_detach_device(), which is the internal handler of the detach function and little in the (*release) callback (devres cleanup only). The problem with that is, that we tear down resources which later in the devres cleanup are needed. With them not being there anymore we panic, e.g., in lkpi_dma_unmap < lkpi_dmam_free_coherent < lkpi_devres_release_free_list. The solution is to migrate most of the cleanup work into the (*release) callback, which will automatically be called when the device (kobj) reference drops to zero. The only work which should be done immediately is to let the dirver do its cleanup; this has to happen before we try to teardown the resources, but also we do want this to happen when detach is called (the first time). One problem we have with the deferred cleanup of the remaining parts is that we do not know upon calling pci_dev_put() whether this cleared the last reference and triggered the cleanup or not but we cannot return from the detach function with pending resources and dangling pointers, which then may be used. In order to work around this, we clear the (*release) callback function when it is run and check for that in the detach routine. If the (*release) callback was not run, we refuse to detach (force would be needed) as we'd rather keep the device than risk a follow-up panic on leaked resources. Given this should not happen in a well programmed world, I believe it is fine to take that and log it to let the user know. Try to leave a few comments behind to help with understanding in the future. With this we can unload the mt7921 driver (or shutdown the system) without panic. Sponsored by: The FreeBSD Foundation MFC after: 3 days Reviewed by: dumbbell Differential Revision: https://reviews.freebsd.org/D57429 --- sys/compat/linuxkpi/common/src/linux_pci.c | 143 ++++++++++++++++++++++++++--- 1 file changed, 129 insertions(+), 14 deletions(-) diff --git a/sys/compat/linuxkpi/common/src/linux_pci.c b/sys/compat/linuxkpi/common/src/linux_pci.c index cdf7f00184ce..1622ecb8b8ff 100644 --- a/sys/compat/linuxkpi/common/src/linux_pci.c +++ b/sys/compat/linuxkpi/common/src/linux_pci.c @@ -1,7 +1,7 @@ /*- * Copyright (c) 2015-2016 Mellanox Technologies, Ltd. * All rights reserved. - * Copyright (c) 2020-2025 The FreeBSD Foundation + * Copyright (c) 2020-2026 The FreeBSD Foundation * * Portions of this software were developed by Björn Zeeb * under sponsorship from the FreeBSD Foundation. @@ -28,6 +28,15 @@ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ +/* + * We have two ways to create a pci_dev (pdev): + * (1) coming from the device_attach DEVMETHOD, and + * (2) the other from manual creation via lkpinew_pci_dev(). + * + * Only devices from (1) end up on our LinuxKPI global pci_devices list. + * All others are "place fillers" -- XXX if only "place filler" was always true. + */ + #include <sys/param.h> #include <sys/systm.h> #include <sys/bus.h> @@ -100,6 +109,7 @@ static int linux_backlight_get_status(device_t dev, struct backlight_props *prop static int linux_backlight_update_status(device_t dev, struct backlight_props *props); static int linux_backlight_get_info(device_t dev, struct backlight_info *info); static void lkpi_pcim_iomap_table_release(struct device *, void *); +static void lkpinew_pci_dev_release(struct device *); static device_method_t pci_methods[] = { DEVMETHOD(device_probe, linux_pci_probe), @@ -337,9 +347,82 @@ lkpi_pci_get_device(uint32_t vendor, uint32_t device, struct pci_dev *odev) static void lkpi_pci_dev_release(struct device *dev) { + struct pci_dev *pdev; + /* + * Before anything else, we have to free all the dynamic + * resource which are on the devres list. + * Otherwise we risk that supporting infrastructure + * is gone and we panic 'randomly'. + */ lkpi_devres_release_free_list(dev); + + /* + * Now undo linux_pci_attach_device() in reverse-ish + * order. + */ + pdev = to_pci_dev(dev); + + /* + * pdrv->remove happens before pci_put_dev() in + * linux_pci_detach_device(), which means the driver should have + * cleaned up before we get here; see irqents and mmio below. + */ + + /* Clear the hierarchy recursively to root. */ + if (pdev->bus->self != pdev) { + pci_dev_put(pdev->bus->self); + pdev->bus->self = NULL; + } + + if (pdev->root != NULL) { + lkpinew_pci_dev_release(&pdev->root->dev); /* pci_dev_put(pdev->root); ? */ + pdev->root = NULL; + } + + spin_lock(&pci_lock); + list_del(&pdev->links); + spin_unlock(&pci_lock); + + linux_pdev_dma_uninit(pdev); + + /* irq? */ + + /* Undo lkpifill_pci_dev(). */ + /* devres is gone already; went at the very top. */ + if (!list_empty_careful(&pdev->dev.irqents)) { + dev_warn(&pdev->dev, "%s: driver did not clean up; " + "leaking IRQs\n", __func__); + /* + * XXX add private function to interrupt.h/linux_interrupt.c + * to walk the list and call free_irq on each if we have to. + */ + } + spin_lock_destroy(&dev->devres_lock); + spin_lock_destroy(&pdev->pcie_cap_lock); + + if (!TAILQ_EMPTY(&pdev->mmio)) { + dev_warn(&pdev->dev, "%s: driver did not clean up; " + "leaking mmio resources\n", __func__); + /* XXX we have two functions to walk and release in here. */ + } + + if (pdev->msi_desc != NULL) { + for (int i = pci_msi_count(pdev->dev.bsddev) - 1; i >= 0; i--) + free(pdev->msi_desc[i], M_DEVBUF); + free(pdev->msi_desc, M_DEVBUF); + } + + free(pdev->bus, M_DEVBUF); + kfree(pdev->path_name); + + /* + * Lastly, apply an internal hack in order to signal + * that this was run (device reference fully dropped). + * See comment in linux_pci_detach_device(). + */ + pdev->dev.release = NULL; } static int @@ -666,14 +749,16 @@ static int linux_pci_detach(device_t dev) { struct pci_dev *pdev; + int error; pdev = device_get_softc(dev); - MPASS(pdev != NULL); - device_set_desc(dev, NULL); + error = linux_pci_detach_device(pdev); + if (error == 0) + device_set_desc(dev, NULL); - return (linux_pci_detach_device(pdev)); + return (error); } int @@ -682,21 +767,51 @@ linux_pci_detach_device(struct pci_dev *pdev) linux_set_current(curthread); + /* + * We cannot do much here as almost everything will have + * to happen as the last reference to the LinuxKPI device + * goes away. That will call the release function, + * which lkpifill_pci_dev() set. That is were most + * of the cleanup will happen. But before that give + * the driver a chance to cleanup. + * The big problem is that the Linux KPI does not + * report back if it was the last kref (well kref + * does report back but then kobj, dev, pdev do not). + * So we have little way of knowing if the release + * happened or not. We have to play tricks for that + * and we can given the softc (pdev) is still valid + * until we return from here. + */ + if (pdev->pdrv != NULL) pdev->pdrv->remove(pdev); - if (pdev->root != NULL) - pci_dev_put(pdev->root); - free(pdev->bus, M_DEVBUF); - linux_pdev_dma_uninit(pdev); + pci_dev_put(pdev); - spin_lock(&pci_lock); - list_del(&pdev->links); - spin_unlock(&pci_lock); - spin_lock_destroy(&pdev->pcie_cap_lock); - put_device(&pdev->dev); + /* + * We (ab)use the release function as a guard to + * know if we made it there and the device is gone. + */ + if (pdev->dev.release != lkpi_pci_dev_release) + return (0); - return (0); + /* + * Detach failed. + * We need to re-acquire the ref and wait for + * the other refs to be gone... In theory this + * should never happen, so log it! + * XXX I wish there was a KPI to query the ref. + * + * If we do not error and wait, we will have a + * LinuxKPI device dangling active with pointers + * but the FreeBSD device_t will be 'gone'. + */ + device_printf(pdev->dev.bsddev, "%s failed due to %u other pending " + "references on the LinuxKPI device.\n", __func__, + kref_read(&pdev->dev.kobj.kref)); + pci_dev_get(pdev); + + return (EBUSY); } static int
