linux-kernel - Re: [RFC V3 3/4] soc: qcom: Introduce SCMI based Memlat (Memory Latency) governor

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: <32904576-e048-6858-a213-f00f92e9e434@quicinc.com>
Date: Mon, 7 Oct 2024 12:52:46 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@...aro.org>, <sudeep.holla@....com>,
        <cristian.marussi@....com>, <andersson@...nel.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>
CC: <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <quic_rgottimu@...cinc.com>, <quic_kshivnan@...cinc.com>,
        <conor+dt@...nel.org>, Amir Vajid <avajid@...cinc.com>
Subject: Re: [RFC V3 3/4] soc: qcom: Introduce SCMI based Memlat (Memory
 Latency) governor



On 7/9/24 16:21, Konrad Dybcio wrote:
> On 2.07.2024 9:14 PM, Sibi Sankar wrote:
>> Introduce a client driver that uses the memlat algorithm string hosted
>> on ARM SCMI QCOM Vendor Protocol to detect memory latency workloads and
>> control frequency/level of the various memory buses (DDR/LLCC/DDR_QOS).
>>
>> Co-developed-by: Shivnandan Kumar <quic_kshivnan@...cinc.com>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@...cinc.com>
>> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@...cinc.com>
>> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@...cinc.com>
>> Co-developed-by: Amir Vajid <avajid@...cinc.com>
>> Signed-off-by: Amir Vajid <avajid@...cinc.com>
>> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
>> ---
> 
> [...]
> 
>> +/**
>> + * scmi_memlat_protocol_cmd - parameter_ids supported by the "MEMLAT" algo_str hosted
>> + *                            by the Qualcomm SCMI Vendor Protocol on the SCMI controller.
> 
> 'enum scmi_mem..'
> 
>> +static int populate_cluster_info(u32 *cluster_info)
>> +{
>> +	char name[MAX_NAME_LEN];
>> +	int i = 0;
>> +
>> +	struct device_node *cn __free(device_node) = of_find_node_by_path("/cpus");
>> +	if (!cn)
>> +		return -ENODEV;
>> +
>> +	struct device_node *map __free(device_node) = of_get_child_by_name(cn, "cpu-map");
>> +	if (!map)
>> +		return -ENODEV;
>> +
>> +	do {
>> +		snprintf(name, sizeof(name), "cluster%d", i);
>> +		struct device_node *c __free(device_node) = of_get_child_by_name(map, name);
>> +		if (!c)
>> +			break;
>> +
>> +		*(cluster_info + i) = of_get_child_count(c);
>> +		i++;
>> +	} while (1);
> 
> of_cpu_device_node_get(0) + of_get_next_cpu_node() +
> of_get_cpu_hwid() & MPIDR_EL1.Aff2 [1]
> 
> [...]
> 
>> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
>> +							    struct scmi_memory_info *memory,
>> +							    struct device_node *of_node,
>> +							    u32 *cnt)
>> +{
>> +	struct device_node *tbl_np, *opp_np;
>> +	struct cpufreq_memfreq_map *tbl;
>> +	int ret, i = 0;
>> +	u32 level, len;
>> +	u64 rate;
>> +
>> +	tbl_np = of_parse_phandle(of_node, "operating-points-v2", 0);
>> +	if (!tbl_np)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	len = min(of_get_available_child_count(tbl_np), MAX_MAP_ENTRIES);
>> +	if (len == 0)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	tbl = devm_kzalloc(dev, (len + 1) * sizeof(struct cpufreq_memfreq_map),
>> +			   GFP_KERNEL);
>> +	if (!tbl)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for_each_available_child_of_node(tbl_np, opp_np) {
>> +		ret = of_property_read_u64_index(opp_np, "opp-hz", 0, &rate);
>> +		if (ret < 0)
>> +			return ERR_PTR(ret);
>> +
>> +		tbl[i].cpufreq_mhz = rate / HZ_PER_MHZ;
>> +
>> +		if (memory->hw_type != QCOM_MEM_TYPE_DDR_QOS) {
>> +			ret = of_property_read_u64_index(opp_np, "opp-hz", 1, &rate);
>> +			if (ret < 0)
>> +				return ERR_PTR(ret);
>> +
>> +			tbl[i].memfreq_khz = rate / HZ_PER_KHZ;
>> +		} else {
>> +			ret = of_property_read_u32(opp_np, "opp-level", &level);
>> +			if (ret < 0)
>> +				return ERR_PTR(ret);
>> +
>> +			tbl[i].memfreq_khz = level;
>> +		}
>> +
>> +		dev_dbg(dev, "Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz, tbl[i].memfreq_khz);
>> +		i++;
>> +	}
>> +	*cnt = len;
>> +	tbl[i].cpufreq_mhz = 0;
> 
> missing of_node_put, or even better __free(device_node)
> 
> [...]
> 
>> +			/*
>> +			 * Variants of the SoC having reduced number of cpus operate
>> +			 * with the same number of logical cpus but the physical
>> +			 * cpu disabled will differ between parts. Calculate the
>> +			 * physical cpu number using cluster information instead.
>> +			 */
>> +			ret = populate_physical_mask(monitor_np, &monitor->mask,
>> +						     info->cluster_info);
>> +			if (ret < 0) {
>> +				dev_err_probe(&sdev->dev, ret, "failed to populate cpu mask\n");
>> +				goto err;
>> +			}
> 
> err.. the same number of logical CPUs? as in, PSCI will happily report that
> the inexistent cores have been booted? or some cores start doing some sort
> of hyperthreading to make up for the missing ones? this sounds sketchy..

Hey Konrad,

The problem we are describing here is about a scenario where the
bootloaders disable faulty cpus after reading fuse values by fixing
up the cpu node in the device tree by marking them as "failed". In
this case the logical cpu number of the cpus that booted up never
align with the physical numbers. Hence relying on any logical
number for this calculation doesn't work. Will address all your
other comments in v4.

-Sibi

> 
> Konrad