Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Thu, 30 Nov 2023 14:24:55 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
cc:     Hans de Goede <hdegoede@redhat.com>, markgross@kernel.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        platform-driver-x86@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 6/6] platform/x86/intel-uncore-freq: Process read/write
 blocked feature status
In-Reply-To: <20231128185605.3027653-7-srinivas.pandruvada@linux.intel.com>
Message-ID: <80b89411-4633-4b8f-72b1-bf262174566e@linux.intel.com>
References: <20231128185605.3027653-1-srinivas.pandruvada@linux.intel.com> <20231128185605.3027653-7-srinivas.pandruvada@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-1161852847-1701347099=:1808"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1161852847-1701347099=:1808
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 8BIT

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.

--8323329-1161852847-1701347099=:1808--