Re: [PATCH v12 17/25] platform/x86/intel/pmt: Register enumeration functions with resctrl

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 9/16/26 4:13 PM, Tony Luck wrote:
> INTEL_PMT_TELEMETRY is a loadable module

This context is not true, at this point it is required to be built in for AET use.

, but resctrl is built-in and cannot
> call PMT functions directly.

This is becoming increasingly confusing since clearly resctrl currently calls
the functions directly because it requires the module to be built in. The changelogs
need to support these changes but just causes confusion. 

This changelog states 
	
	"INTEL_PMT_TELEMETRY is a loadable module"
	
	and
	
	"resctrl is built-in and cannot call PMT functions directly"

Next patch then contradicts this patch right away:

	Use the function pointers instead of direct function calls in
	preparation for the telemetry driver to be a loadable module.

> 
> Use ".probe_type = PROBE_FORCE_SYNCHRONOUS" to ensure sequential, synchronous
> calls to per-device .probe() functions.  Register the telemetry enumeration
> function pointers at the end of pmt_telem_init() when all .probe() functions
> have run and enumeration is complete.

why?

> 
> Unregister at the start of pmt_telem_exit() before teardown of the
> auxiliary drivers.

How is this sentence different from:

  static void __exit pmt_telem_exit(void)
  {
 +	intel_aet_unregister_enumeration();
  	auxiliary_driver_unregister(&pmt_telem_aux_driver);
  	xa_destroy(&telem_array);
  }


> 
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> v12:
> 	No change
> ---
>  drivers/platform/x86/intel/pmt/telemetry.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/telemetry.c b/drivers/platform/x86/intel/pmt/telemetry.c
> index 953f35b6daec..bf0c9295feb3 100644
> --- a/drivers/platform/x86/intel/pmt/telemetry.c
> +++ b/drivers/platform/x86/intel/pmt/telemetry.c
> @@ -427,16 +427,30 @@ static struct auxiliary_driver pmt_telem_aux_driver = {
>  	.id_table	= pmt_telem_id_table,
>  	.remove		= pmt_telem_remove,
>  	.probe		= pmt_telem_probe,
> +	.driver		= {
> +		.probe_type = PROBE_FORCE_SYNCHRONOUS,
> +	},
>  };
>  
>  static int __init pmt_telem_init(void)
>  {
> -	return auxiliary_driver_register(&pmt_telem_aux_driver);
> +	int ret;
> +
> +	ret = auxiliary_driver_register(&pmt_telem_aux_driver);
> +

Style is up to this owner but I'd suggest dropping this empty line.

> +	if (ret)
> +		return ret;
> +
> +	intel_aet_register_enumeration(THIS_MODULE, intel_pmt_get_regions_by_feature,
> +				       intel_pmt_put_feature_group);
> +
> +	return 0;
>  }
>  module_init(pmt_telem_init);
>  
>  static void __exit pmt_telem_exit(void)
>  {
> +	intel_aet_unregister_enumeration();
>  	auxiliary_driver_unregister(&pmt_telem_aux_driver);
>  	xa_destroy(&telem_array);
>  }

Reinette