Re: [PATCH 6/6] platform/x86/intel-uncore-freq: Process read/write blocked feature status

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2932 bytes --]

On Tue, 28 Nov 2023, Srinivas Pandruvada wrote:

> When a feature is read blocked, don't continue to read uncore information
> and register with uncore core.
> 
> When the feature is write blocked, continue to offer read interface but
> block setting uncore limits.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  .../uncore-frequency/uncore-frequency-tpmi.c      | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> index 4fb790552c47..de5db49a9afe 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> @@ -66,6 +66,7 @@ struct tpmi_uncore_struct {
>  	int min_ratio;
>  	struct tpmi_uncore_power_domain_info *pd_info;
>  	struct tpmi_uncore_cluster_info root_cluster;
> +	bool write_blocked;
>  };
>  
>  #define UNCORE_GENMASK_MIN_RATIO	GENMASK_ULL(21, 15)
> @@ -157,6 +158,9 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
>  	cluster_info = container_of(data, struct tpmi_uncore_cluster_info, uncore_data);
>  	uncore_root = cluster_info->uncore_root;
>  
> +	if (uncore_root->write_blocked)
> +		return -EPERM;
> +
>  	/* Update each cluster in a package */
>  	if (cluster_info->root_domain) {
>  		struct tpmi_uncore_struct *uncore_root = cluster_info->uncore_root;
> @@ -233,11 +237,21 @@ static void remove_cluster_entries(struct tpmi_uncore_struct *tpmi_uncore)
>  
>  static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
>  {
> +	int read_blocked = 0, write_blocked = 0;
>  	struct intel_tpmi_plat_info *plat_info;
>  	struct tpmi_uncore_struct *tpmi_uncore;
>  	int ret, i, pkg = 0;
>  	int num_resources;
>  
> +	ret = tpmi_get_feature_status(auxdev, TPMI_ID_UNCORE, &read_blocked, &write_blocked);
> +	if (ret)
> +		dev_info(&auxdev->dev, "Can't read feature status: ignoring blocked status\n");
> +
> +	if (read_blocked) {
> +		dev_info(&auxdev->dev, "Firmware has blocked reads, exiting\n");
> +		return -ENODEV;
> +	}
> +
>  	/* Get number of power domains, which is equal to number of resources */
>  	num_resources = tpmi_get_resource_count(auxdev);
>  	if (!num_resources)
> @@ -266,6 +280,7 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
>  	}
>  
>  	tpmi_uncore->power_domain_count = num_resources;
> +	tpmi_uncore->write_blocked = write_blocked;
>  
>  	/* Get the package ID from the TPMI core */
>  	plat_info = tpmi_get_platform_data(auxdev);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

While reviewing this, I begun to wonder why's the 
tpmi_get_feature_status() interface using int * as in practice these will 
always be converted to bool by the users?

-- 
 i.