linux-kernel - Re: [PATCH] platform/x86/intel-uncore-freq: Do not present separate package-die domain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cd7cc6b8-4993-937b-072b-8fbc1e309a4d@linux.intel.com>
Date: Mon, 12 Aug 2024 14:16:46 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
cc: Hans de Goede <hdegoede@...hat.com>, 
    Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86/intel-uncore-freq: Do not present separate
 package-die domain

On Wed, 31 Jul 2024, Srinivas Pandruvada wrote:

> The scope of uncore control is per power domain in a package and die.
> A package-die can have multiple power domains on some processors. In this
> case package-die domain (root domain) aggregates all information from
> power domains in it.
> 
> On some processors, CPUID enumerates the die number same as power domain
> ID. In this case there is one to one relationship between package-die and
> power domain ID. There is no use of aggregating information from all
> power domain IDs as the information will be duplicate and confusing. In
> this case do not create separate package-die domain.

Hi Srinivas,

I got confused by this changelog because its order is quite illogical.

First paragraph talks about case A. When you say "all information" 
is "aggregated", I immediately make the assumption that the aggregated 
information is what is wanted because, well, you normally want "all 
information" and nothing else is being told here.

Second paragraph starts to talk about case B and then suddenly switches to 
talk what should have been done in case A (that aggregated information is 
useless/confusing).

So I think some reorganization of the sentences would be useful to not 
jump between cases mid-paragraph without any hints.

(I hope my explanation is enough to highlight why it was hard to follow).

When I finally understood what the changelog was saying, I found out the 
code change is fine too but it first looked like it was doing exactly the 
opposite to my expectations/reasonale given in the changelog.

-- 
 i.


> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> ---
>  .../x86/intel/uncore-frequency/uncore-frequency-tpmi.c     | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> 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 9fa3037c03d1..6c2e607968f2 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
> @@ -427,6 +427,9 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
>  
>  	auxiliary_set_drvdata(auxdev, tpmi_uncore);
>  
> +	if (topology_max_dies_per_package() > 1)
> +		return 0;
> +
>  	tpmi_uncore->root_cluster.root_domain = true;
>  	tpmi_uncore->root_cluster.uncore_root = tpmi_uncore;
>  
> @@ -450,7 +453,9 @@ static void uncore_remove(struct auxiliary_device *auxdev)
>  {
>  	struct tpmi_uncore_struct *tpmi_uncore = auxiliary_get_drvdata(auxdev);
>  
> -	uncore_freq_remove_die_entry(&tpmi_uncore->root_cluster.uncore_data);
> +	if (tpmi_uncore->root_cluster.root_domain)
> +		uncore_freq_remove_die_entry(&tpmi_uncore->root_cluster.uncore_data);
> +
>  	remove_cluster_entries(tpmi_uncore);
>  
>  	uncore_freq_common_exit();
>