| 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
| ||
|
Message-ID: <11dee606-9c8a-7e6e-4514-d03979c118bd@quicinc.com>
Date: Wed, 3 Jul 2024 17:04:07 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Shivnandan Kumar <quic_kshivnan@...cinc.com>, <sudeep.holla@....com>,
<cristian.marussi@....com>, <andersson@...nel.org>,
<konrad.dybcio@...aro.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>, <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/3/24 14:14, Shivnandan Kumar wrote:
>
>
> On 7/3/2024 12:44 AM, 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).
>>
Hey Shiv,
Thanks for taking time to review the series :)
>> 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>
>> ---
>>
>> V2:
>> * Make driver changes to the accommodate bindings changes. [Rob]
>> * Replace explicit of_node_put with _free.
>>
>> drivers/soc/qcom/Kconfig | 12 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom_scmi_client.c | 583 ++++++++++++++++++++++++++++
>> 3 files changed, 596 insertions(+)
>> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 432c85bd8ad4..b253504bd386 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -294,4 +294,16 @@ config QCOM_PBS
>> This module provides the APIs to the client drivers that wants
>> to send the
>> PBS trigger event to the PBS RAM.
>> +config QCOM_SCMI_CLIENT
>> + tristate "Qualcomm Technologies Inc. SCMI client driver"
>> + depends on ARM_SCMI_PROTOCOL_VENDOR_QCOM || COMPILE_TEST
>> + default n
>> + help
>> + This driver uses the memlat algorithm string hosted on QCOM SCMI
>> + Vendor Protocol to detect memory latency workloads and control
>> + frequency/level of the various memory buses (DDR/LLCC/DDR_QOS).
>> +
>> + This driver defines/documents the parameter IDs used while
>> configuring
>> + the memory buses.
>> +
>> endmenu
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index d3560f861085..8a2e832d1d5d 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -35,6 +35,7 @@ obj-$(CONFIG_QCOM_APR) += apr.o
>> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
>> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
>> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
>> qcom_ice-objs += ice.o
>> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
>> obj-$(CONFIG_QCOM_PBS) += qcom-pbs.o
>> diff --git a/drivers/soc/qcom/qcom_scmi_client.c
>> b/drivers/soc/qcom/qcom_scmi_client.c
>> new file mode 100644
>> index 000000000000..8369b415c0ab
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom_scmi_client.c
>> @@ -0,0 +1,583 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights
>> reserved.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/qcom_scmi_vendor.h>
>> +#include <linux/scmi_protocol.h>
>> +#include <linux/units.h>
>> +#include <dt-bindings/soc/qcom,scmi-vendor.h>
>> +
>> +#define MEMLAT_ALGO_STR 0x4D454D4C4154 /* MEMLAT */
>> +#define INVALID_IDX 0xff
>> +#define MAX_MEMORY_TYPES 3
>> +#define MAX_MONITOR_CNT 4
>> +#define MAX_NAME_LEN 20
>> +#define MAX_MAP_ENTRIES 7
>> +#define CPUCP_DEFAULT_SAMPLING_PERIOD_MS 4
>> +#define CPUCP_DEFAULT_FREQ_METHOD 1
>> +
>> +/**
>> + * scmi_memlat_protocol_cmd - parameter_ids supported by the "MEMLAT"
>> algo_str hosted
>> + * by the Qualcomm SCMI Vendor Protocol on
>> the SCMI controller.
>> + *
>> + * MEMLAT (Memory Latency) monitors the counters to detect memory
>> latency bound workloads
>> + * and scales the frequency/levels of the memory buses accordingly.
>> + *
>> + * @MEMLAT_SET_MEM_GROUP: initializes the frequency/level scaling
>> functions for the memory bus.
>> + * @MEMLAT_SET_MONITOR: configures the monitor to work on a specific
>> memory bus.
>> + * @MEMLAT_SET_COMMON_EV_MAP: set up common counters used to monitor
>> the cpu frequency.
>> + * @MEMLAT_SET_GRP_EV_MAP: set up any specific counters used to
>> monitor the memory bus.
>> + * @MEMLAT_IPM_CEIL: set the IPM (Instruction Per Misses) ceiling per
>> monitor.
>> + * @MEMLAT_SAMPLE_MS: set the sampling period for all the monitors.
>> + * MEMLAT_MON_FREQ_MAP: setup the cpufreq to memfreq map.
>> + * MEMLAT_SET_MIN_FREQ: set the max frequency of the memory bus.
>> + * MEMLAT_SET_MAX_FREQ: set the min frequency of the memory bus.
>> + * MEMLAT_START_TIMER: start all the monitors with the requested
>> sampling period.
>> + * MEMLAT_START_TIMER: stop all the running monitors.
>
> Typo above, it should be MEMLAT_STOP_TIMER
ack, thanks for catching this.
>
>> + * MEMLAT_SET_EFFECTIVE_FREQ_METHOD: set the method used to determine
>> cpu frequency.
>> + */
>> +enum scmi_memlat_protocol_cmd {
>> + MEMLAT_SET_MEM_GROUP = 16,
>> + MEMLAT_SET_MONITOR,
>> + MEMLAT_SET_COMMON_EV_MAP,
>> + MEMLAT_SET_GRP_EV_MAP,
>> + MEMLAT_IPM_CEIL = 23,
>> + MEMLAT_SAMPLE_MS = 31,
>> + MEMLAT_MON_FREQ_MAP,
>> + MEMLAT_SET_MIN_FREQ,
>> + MEMLAT_SET_MAX_FREQ,
>> + MEMLAT_START_TIMER = 36,
>> + MEMLAT_STOP_TIMER,
>> + MEMLAT_SET_EFFECTIVE_FREQ_METHOD = 39,
>> +};
>> +
>> +struct map_table {
[...]
>> +
>> +static int populate_physical_mask(struct device_node *np, u32 *mask,
>> u32 *cluster_info)
>> +{
>> + struct device_node *dev_phandle;
>> + int cpu, i = 0, physical_id;
>> +
>> + do {
>> + dev_phandle = of_parse_phandle(np, "cpus", i++);
>> + cpu = of_cpu_node_to_id(dev_phandle);
>> + if (cpu != -ENODEV) {
>> + physical_id = topology_core_id(cpu);
>> + for (int j = 0; j < topology_cluster_id(cpu); j++)
>> + physical_id += *(cluster_info + j);
>> + *mask |= BIT(physical_id);
>> + }
>> + } while (dev_phandle);
>> +
>> + return 0;
>> +}
>> +
>> +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;
>
> This is not needed as for allocating memory, devm_kzalloc is used.
ack
>
>> +
>> + return tbl;
>> +}
>> +
>> +static int process_scmi_memlat_of_node(struct scmi_device *sdev,
>> struct scmi_memlat_info *info)
>> +{
>> + struct scmi_monitor_info *monitor;
>> + struct scmi_memory_info *memory;
>> + char name[MAX_NAME_LEN];
>> + u64 memfreq[2];
>> + int ret;
>> +
>> + ret = populate_cluster_info(info->cluster_info);
>> + if (ret < 0) {
>> + dev_err_probe(&sdev->dev, ret, "failed to populate cluster
>> info\n");
>> + goto err;
>> + }
>> +
>> + of_node_get(sdev->handle->dev->of_node);
>> + do {
>> + snprintf(name, sizeof(name), "memory-%d", info->memory_cnt);
>> + struct device_node *memory_np __free(device_node) =
>> + of_find_node_by_name(sdev->handle->dev->of_node, name);
>> +
>> + if (!memory_np)
>> + break;
>> +
>> + memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
>> + if (!memory) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + ret = of_property_read_u32(memory_np, "qcom,memory-type",
>> &memory->hw_type);
>> + if (ret) {
>> + dev_err_probe(&sdev->dev, ret, "failed to read memory
>> type\n");
>> + goto err;
>> + }
>> +
>> + ret = of_property_read_u64_array(memory_np, "freq-table-hz",
>> memfreq, 2);
>> + if (ret && (ret != -EINVAL)) {
>> + dev_err_probe(&sdev->dev, ret, "failed to read min/max
>> freq\n");
>> + goto err;
>> + }
>> +
>> + if (memory->hw_type != QCOM_MEM_TYPE_DDR_QOS) {
>> + memory->min_freq = memfreq[0] / HZ_PER_KHZ;
>> + memory->max_freq = memfreq[1] / HZ_PER_KHZ;
>> + } else {
>> + memory->min_freq = memfreq[0];
>> + memory->max_freq = memfreq[1];
>> + }
>> + info->memory[info->memory_cnt++] = memory;
>> +
>> + do {
>> + snprintf(name, sizeof(name), "monitor-%d",
>> memory->monitor_cnt);
>> + struct device_node *monitor_np __free(device_node) =
>> + of_get_child_by_name(memory_np, name);
>> +
>> + if (!monitor_np)
>> + break;
>> +
>> + monitor = devm_kzalloc(&sdev->dev, sizeof(*monitor),
>> GFP_KERNEL);
>> + if (!monitor) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + monitor->mon_type = of_property_read_bool(monitor_np,
>> "qcom,compute-type");
>> + if (!monitor->mon_type) {
>> + ret = of_property_read_u32(monitor_np, "qcom,ipm-ceil",
>> + &monitor->ipm_ceil);
>> + if (ret) {
>> + dev_err_probe(&sdev->dev, ret,
>> + "failed to read IPM ceiling\n");
>> + goto err;
>> + }
>> + }
>> +
>> + /*
>> + * 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;
>> + }
>> +
>
> Above error check is not needed as populate_physical_mask always return 0.
ack, will fix this in the next re-spin.
>
>
>> + monitor->freq_map = init_cpufreq_memfreq_map(&sdev->dev,
>> memory, monitor_np,
>> + &monitor->freq_map_len);
>> + if (IS_ERR(monitor->freq_map)) {
>> + dev_err_probe(&sdev->dev, PTR_ERR(monitor->freq_map),
>> + "failed to populate cpufreq-memfreq map\n");
>> + goto err;
>> + }
>> +
>> + strscpy(monitor->mon_name, name, sizeof(monitor->mon_name));
>> + monitor->mon_idx = memory->monitor_cnt;
>> +
>> + memory->monitor[memory->monitor_cnt++] = monitor;
>
> If from dt, more than 4 monitor are passed then this may lead to
> overflow, adding a check and giving warning in case number of monitor
> exceeds MAX_MONITOR_CNT will be better idea.
I'll make sure to account for ^^ by either switching to dynamic
allocation or by using MAX_MONITOR_CNT.
>
>
>> + } while (1);
>> +
[...]
>> +MODULE_DESCRIPTION("QTI SCMI client driver");
>> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists