Re: git: eda36ae09dd1 - main - asmc: resource cleanup simplifications

On 2/21/26 21:00, Enji Cooper wrote:
> The branch main has been updated by ngie:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=eda36ae09dd1fab78bd377739fc5d6c65c61f5d7
> 
> commit eda36ae09dd1fab78bd377739fc5d6c65c61f5d7
> Author:     Enji Cooper <ngie@FreeBSD.org>
> AuthorDate: 2026-01-30 06:55:08 +0000
> Commit:     Enji Cooper <ngie@FreeBSD.org>
> CommitDate: 2026-02-22 01:59:06 +0000
> 
>      asmc: resource cleanup simplifications
>      
>      This change makes `asmc_detach(..)` reentrant by setting freed resources
>      to known invalid values when done, and makes `asmc_attach(..)` call
>      `asmc_detach(..)` instead of attempting to the semi-equivalent way of
>      cleaning up the driver resources allocated in `asmc_detach(..)`.
>      
>      MFC after:      1 week
>      Differential Revision:  https://reviews.freebsd.org/D55413
> ---
>   sys/dev/asmc/asmc.c | 33 +++++++++++++++++++--------------
>   1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/sys/dev/asmc/asmc.c b/sys/dev/asmc/asmc.c
> index bff214c084c5..084b57331dd9 100644
> --- a/sys/dev/asmc/asmc.c
> +++ b/sys/dev/asmc/asmc.c
> @@ -795,25 +795,21 @@ asmc_attach(device_t dev)
>   	if (sc->sc_irq == NULL) {
>   		device_printf(dev, "unable to allocate IRQ resource\n");
>   		ret = ENXIO;
> -		goto err2;
> +		goto err;
>   	}
>   
>   	ret = bus_setup_intr(dev, sc->sc_irq, INTR_TYPE_MISC | INTR_MPSAFE,
>   	    asmc_sms_intrfast, NULL, dev, &sc->sc_cookie);
>   	if (ret) {
>   		device_printf(dev, "unable to setup SMS IRQ\n");
> -		goto err1;
> +		goto err;
>   	}
> +
>   nosms:
>   	return (0);
> -err1:
> -	bus_release_resource(dev, SYS_RES_IRQ, sc->sc_rid_irq, sc->sc_irq);
> -err2:
> -	bus_release_resource(dev, SYS_RES_IOPORT, sc->sc_rid_port,
> -	    sc->sc_ioport);
> -	mtx_destroy(&sc->sc_mtx);
> -	if (sc->sc_sms_tq)
> -		taskqueue_free(sc->sc_sms_tq);
> +
> +err:
> +	asmc_detach(dev);

This cleanup is good.

>   	return (ret);
>   }
> @@ -826,16 +822,25 @@ asmc_detach(device_t dev)
>   	if (sc->sc_sms_tq) {
>   		taskqueue_drain(sc->sc_sms_tq, &sc->sc_sms_task);
>   		taskqueue_free(sc->sc_sms_tq);
> +		sc->sc_sms_tq = NULL;

Does this actually matter?  That is, I don't think there is any reason for
asmc_detach to be reentrant.  New-bus (specifically device_attach()) does not
invoke DEVICE_DETACH if DEVICE_ATTACH fails.  Instead, DEVICE_ATTACH routines
are expected to fully cleanup on failure.  The changes to be reentrant here
don't hurt, but they generally just add clutter.

>   	}
> -	if (sc->sc_cookie)
> +	if (sc->sc_cookie) {
>   		bus_teardown_intr(dev, sc->sc_irq, sc->sc_cookie);
> -	if (sc->sc_irq)
> +		sc->sc_cookie = NULL;
> +	}
> +	if (sc->sc_irq) {
>   		bus_release_resource(dev, SYS_RES_IRQ, sc->sc_rid_irq,
>   		    sc->sc_irq);
> -	if (sc->sc_ioport)
> +		sc->sc_irq = NULL;
> +	}
> +	if (sc->sc_ioport) {
>   		bus_release_resource(dev, SYS_RES_IOPORT, sc->sc_rid_port,
>   		    sc->sc_ioport);
> -	mtx_destroy(&sc->sc_mtx);
> +		sc->sc_ioport = NULL;
> +	}
> +	if (mtx_initialized(&sc->sc_mtx)) {

Please do not use this.  Code should know if the mutex is initialized or not.
That is, always initalize the mutex in attach before any failures that could
enter this path.

> +		mtx_destroy(&sc->sc_mtx);
> +	}

-- 
John Baldwin