RE: [PATCH v2 1/4] coresight: syscfg: Fail to enable a configuration not loaded on the device

[Mike Leach <Mike.Leach@arm.com>]

Hi,

> -----Original Message-----
> From: Amir Ayupov <aaupov@fb.com>
> Sent: Tuesday, October 13, 2026 12:05 AM
> To: James Clark <james.clark@linaro.org>; Leo Yan <Leo.Yan@arm.com>;
> Suzuki Poulose <Suzuki.Poulose@arm.com>; Mike Leach
> <Mike.Leach@arm.com>; Alexander Shishkin
> <alexander.shishkin@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jonathan Corbet
> <corbet@lwn.net>; Mathieu Poirier <mathieu.poirier@linaro.org>; Randy
> Dunlap <rdunlap@infradead.org>; Shuah Khan <skhan@linuxfoundation.org>;
> coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-
> doc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH v2 1/4] coresight: syscfg: Fail to enable a configuration not
> loaded on the device
> 
> cscfg_csdev_enable_active_config() looks up the selected configuration in
> the device's loaded configurations, and returns success when it is not
> there. A configuration is only loaded on devices that match at least one
> of its features, so a perf session or sysfs enable that selects it on any
> other device runs without the configuration, with no error.
> 
> Return -EINVAL instead, so enabling the source fails on that device. The
> function is only called with a non-zero configuration hash, so sources
> enabled without a configuration are unaffected.
> 

If you look at the original comments - the idea of this function is to enable a configuration on a device if it is present on that device.
Not enabling a configuration is _not_ an error. This is a generic function that can cover multiple classes of coresight device.

Moving forwards matching configurations to devices can omit certain devices from the configuration. 

For example, a system will have many CTIs, but we may only want to use a specific one or two - possibly to connect etm external outputs to trigger a PE interrupt.
Thus the remaining CTIs would not be active in this configuration.

This conflicts with your purpose which I assume is to prevent perf from using ETM sources that cannot load the required configuration.

I think what is actually needed is a "can_activate" function to be run on the ETM/ETE sources at etm_setup_aux time so that none compliant devices can be filtered out of the active cpu mask at that point.

Regards

Mike

> Fixes: f8cce2ff3c04 ("coresight: syscfg: Add API to activate and enable
> configurations")
> Assisted-by: Devmate:Opus-5.5
> Signed-off-by: Amir Ayupov <aaupov@fb.com>
> ---
>  .../hwtracing/coresight/coresight-syscfg.c    | 53 ++++++++++---------
>  1 file changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c
> b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 2bfdd7b45e49c..a816c22c87bc8 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -1088,36 +1088,39 @@ int cscfg_csdev_enable_active_config(struct
> coresight_device *csdev,
>  	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> 
>  	/*
> -	 * If found, attempt to enable
> +	 * The configuration is active but not loaded on this device, because
> +	 * none of its features match the device. Fail rather than trace
> +	 * without it.
>  	 */
> -	if (config_csdev_active) {
> +	if (!config_csdev_active)
> +		return -EINVAL;
> +
> +	/*
> +	 * Call the generic routine that will program up the internal
> +	 * driver structures prior to programming up the hardware.
> +	 * This routine takes the driver spinlock saved in the configs.
> +	 */
> +	err = cscfg_csdev_enable_config(config_csdev_active, preset);
> +	if (!err) {
>  		/*
> -		 * Call the generic routine that will program up the internal
> -		 * driver structures prior to programming up the hardware.
> -		 * This routine takes the driver spinlock saved in the configs.
> +		 * Successful programming. Check the active_cscfg_ctxt
> +		 * pointer to ensure no pre-emption disabled it via
> +		 * cscfg_csdev_disable_active_config() before
> +		 * we could start.
> +		 *
> +		 * Set enabled if OK, err if not.
>  		 */
> -		err = cscfg_csdev_enable_config(config_csdev_active, preset);
> -		if (!err) {
> -			/*
> -			 * Successful programming. Check the
> active_cscfg_ctxt
> -			 * pointer to ensure no pre-emption disabled it via
> -			 * cscfg_csdev_disable_active_config() before
> -			 * we could start.
> -			 *
> -			 * Set enabled if OK, err if not.
> -			 */
> -			raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock,
> flags);
> -			if (csdev->active_cscfg_ctxt)
> -				config_csdev_active->enabled = true;
> -			else
> -				err = -EBUSY;
> -			raw_spin_unlock_irqrestore(&csdev-
> >cscfg_csdev_lock, flags);
> -		}
> -
> -		if (err)
> -			cscfg_config_desc_put(config_desc);
> +		raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> +		if (csdev->active_cscfg_ctxt)
> +			config_csdev_active->enabled = true;
> +		else
> +			err = -EBUSY;
> +		raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
>  	}
> 
> +	if (err)
> +		cscfg_config_desc_put(config_desc);
> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> --
> 2.52.0