Re: git: 96e500a3f9f0 - main - hda: Push giant / newbus topo lock down a layer

It looks like bus_topo_unlock() is called twice in hdaa_sysctl_reconfig()

On 2021/12/10 9:05, Warner Losh wrote:
> The branch main has been updated by imp:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=96e500a3f9f0e317875b5d5c2097df804106bc3f
> 
> commit 96e500a3f9f0e317875b5d5c2097df804106bc3f
> Author:     Warner Losh <imp@FreeBSD.org>
> AuthorDate: 2021-12-09 23:52:55 +0000
> Commit:     Warner Losh <imp@FreeBSD.org>
> CommitDate: 2021-12-10 00:04:58 +0000
> 
>      hda: Push giant / newbus topo lock down a layer
>      
>      Rather than picking up Giant at the start of these sysctl handlers, push
>      acquiring Giant down a layer to protect walking the children and also
>      to add/delete children for the reconfigure.
>      
>      Sponsored by:           Netflix
>      Reviewed by:            mav, jhb
>      Differential Revision:  https://reviews.freebsd.org/D33057
> ---
>   sys/dev/sound/pci/hda/hdaa.c | 13 +++++++++++--
>   sys/dev/sound/pci/hda/hdac.c | 14 +++++++++++---
>   2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/sys/dev/sound/pci/hda/hdaa.c b/sys/dev/sound/pci/hda/hdaa.c
> index 82c309996d75..ddc22c3bc139 100644
> --- a/sys/dev/sound/pci/hda/hdaa.c
> +++ b/sys/dev/sound/pci/hda/hdaa.c
> @@ -6463,16 +6463,25 @@ hdaa_sysctl_reconfig(SYSCTL_HANDLER_ARGS)
>   	HDA_BOOTHVERBOSE(
>   		device_printf(dev, "Reconfiguration...\n");
>   	);
> -	if ((error = device_delete_children(dev)) != 0)
> +
> +	bus_topo_lock();
> +
> +	if ((error = device_delete_children(dev)) != 0) {
> +		bus_topo_unlock();
>   		return (error);
> +	}
>   	hdaa_lock(devinfo);
>   	hdaa_unconfigure(dev);
>   	hdaa_configure(dev);
>   	hdaa_unlock(devinfo);
>   	bus_generic_attach(dev);
> +	bus_topo_unlock();
>   	HDA_BOOTHVERBOSE(
>   		device_printf(dev, "Reconfiguration done\n");
>   	);
> +
> +	bus_topo_unlock();
> +
>   	return (0);
>   }
>   
> @@ -6669,7 +6678,7 @@ hdaa_attach(device_t dev)
>   	    devinfo, 0, hdaa_sysctl_gpo_config, "A", "GPO configuration");
>   	SYSCTL_ADD_PROC(device_get_sysctl_ctx(dev),
>   	    SYSCTL_CHILDREN(device_get_sysctl_tree(dev)), OID_AUTO,
> -	    "reconfig", CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT,
> +	    "reconfig", CTLTYPE_INT | CTLFLAG_RW,
>   	    dev, 0, hdaa_sysctl_reconfig, "I", "Reprocess configuration");
>   	SYSCTL_ADD_INT(device_get_sysctl_ctx(dev),
>   	    SYSCTL_CHILDREN(device_get_sysctl_tree(dev)), OID_AUTO,
> diff --git a/sys/dev/sound/pci/hda/hdac.c b/sys/dev/sound/pci/hda/hdac.c
> index 5d3f1d85a2ad..058eb63e268d 100644
> --- a/sys/dev/sound/pci/hda/hdac.c
> +++ b/sys/dev/sound/pci/hda/hdac.c
> @@ -1387,12 +1387,20 @@ sysctl_hdac_pindump(SYSCTL_HANDLER_ARGS)
>   		return (0);
>   	}
>   
> -	if ((err = device_get_children(dev, &devlist, &devcount)) != 0)
> +	bus_topo_lock();
> +
> +	if ((err = device_get_children(dev, &devlist, &devcount)) != 0) {
> +		bus_topo_unlock();
>   		return (err);
> +	}
> +
>   	hdac_lock(sc);
>   	for (i = 0; i < devcount; i++)
>   		HDAC_PINDUMP(devlist[i]);
>   	hdac_unlock(sc);
> +
> +	bus_topo_unlock();
> +
>   	free(devlist, M_TEMP);
>   	return (0);
>   }
> @@ -1607,11 +1615,11 @@ hdac_attach2(void *arg)
>   
>   	SYSCTL_ADD_PROC(device_get_sysctl_ctx(sc->dev),
>   	    SYSCTL_CHILDREN(device_get_sysctl_tree(sc->dev)), OID_AUTO,
> -	    "pindump", CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, sc->dev,
> +	    "pindump", CTLTYPE_INT | CTLFLAG_RW, sc->dev,
>   	    sizeof(sc->dev), sysctl_hdac_pindump, "I", "Dump pin states/data");
>   	SYSCTL_ADD_PROC(device_get_sysctl_ctx(sc->dev),
>   	    SYSCTL_CHILDREN(device_get_sysctl_tree(sc->dev)), OID_AUTO,
> -	    "polling", CTLTYPE_INT | CTLFLAG_RW | CTLFLAG_NEEDGIANT, sc->dev,
> +	    "polling", CTLTYPE_INT | CTLFLAG_RW, sc->dev,
>   	    sizeof(sc->dev), sysctl_hdac_polling, "I", "Enable polling mode");
>   }
>   
> 

-- 
-|-__   YAMAMOTO, Taku
  | __ <     <taku@tackymt.homeip.net>