| 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: <68a5d81a-5de8-798a-c150-d74c8ad38cb7@linaro.org>
Date: Sat, 11 Mar 2023 16:01:24 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Georgi Djakov <djakov@...nel.org>
Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks
On 08/03/2023 23:40, Konrad Dybcio wrote:
> Some (but not all) providers (or their specific nodes) require
> specific clocks to be turned on before they can be accessed. Failure
> to ensure that results in a seemingly random system crash (which
> would usually happen at boot with the interconnect driver built-in),
> resulting in the platform not booting up properly.
>
> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> RPM interconnect supports anyway) and handle non-scaling clocks
> separately. Update MSM8996 and SDM660 drivers to make sure they do
> not regress with this change.
>
> This unfortunately has to be done in one patch to prevent either
> compile errors or broken bisect.
Can we determine somehow if the intf clocks are required for the whole
NoC or just for a single node? I don't think that clocks like a0noc_ufs
are requiered to be up for the whole aggre_noc. Instead they probably
have to be enabled only when UFS makes use of the NoC (in other words
when is has voted for the bandwidth).
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> ---
> drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
> drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
> drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
> drivers/interconnect/qcom/sdm660.c | 16 +++++-------
> 4 files changed, 70 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index b52f788d8f3d..ca932ed720fb 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -369,6 +369,17 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>
> qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
>
> + /* If we're powering on the bus, ensure the necessary clocks are on */
> + if (unlikely(!qp->is_on)) {
> + if (agg_peak[0] || agg_peak[1] || max_agg_avg) {
> + /* If this fails, bus accesses will crash the platform! */
> + ret = clk_bulk_prepare_enable(qp->num_intf_clks, qp->intf_clks);
> + if (ret)
> + return ret;
> + qp->is_on = true;
> + }
> + }
> +
> sum_bw = icc_units_to_bps(max_agg_avg);
>
> ret = __qcom_icc_set(src, src_qn, sum_bw);
> @@ -409,6 +420,14 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> qp->bus_clk_rate[i] = rate;
> }
>
> + /* Turn off the interface clocks if the bus was shut down so as not to leak power */
> + if (!qp->bus_clk_rate[0] && !qp->bus_clk_rate[1]) {
> + if (!agg_peak[0] && !agg_peak[1] && !max_agg_avg) {
> + clk_bulk_disable_unprepare(qp->num_intf_clks, qp->intf_clks);
> + qp->is_on = false;
> + }
> + }
> +
> return 0;
> }
>
> @@ -441,21 +460,20 @@ int qnoc_probe(struct platform_device *pdev)
> qnodes = desc->nodes;
> num_nodes = desc->num_nodes;
>
> - if (desc->num_bus_clocks) {
> - cds = desc->bus_clocks;
> - cd_num = desc->num_bus_clocks;
> + if (desc->num_intf_clocks) {
> + cds = desc->intf_clocks;
> + cd_num = desc->num_intf_clocks;
> } else {
> - cds = bus_clocks;
> - cd_num = ARRAY_SIZE(bus_clocks);
> + /* 0 intf clocks is perfectly fine */
> + cd_num = 0;
> }
>
> - qp = devm_kzalloc(dev, struct_size(qp, bus_clks, cd_num), GFP_KERNEL);
> + qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
> if (!qp)
> return -ENOMEM;
>
> - qp->bus_clk_rate = devm_kcalloc(dev, cd_num, sizeof(*qp->bus_clk_rate),
> - GFP_KERNEL);
> - if (!qp->bus_clk_rate)
> + qp->intf_clks = devm_kzalloc(dev, sizeof(qp->intf_clks), GFP_KERNEL);
> + if (!qp->intf_clks)
> return -ENOMEM;
>
> data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
> @@ -463,6 +481,18 @@ int qnoc_probe(struct platform_device *pdev)
> if (!data)
> return -ENOMEM;
>
> + qp->num_intf_clks = cd_num;
> + for (i = 0; i < cd_num; i++)
> + qp->intf_clks[i].id = cds[i];
> +
> + if (desc->num_bus_clocks) {
> + cds = desc->bus_clocks;
> + cd_num = desc->num_bus_clocks;
> + } else {
> + cds = bus_clocks;
> + cd_num = ARRAY_SIZE(bus_clocks);
> + }
> +
> for (i = 0; i < cd_num; i++)
> qp->bus_clks[i].id = cds[i];
> qp->num_bus_clks = cd_num;
> @@ -503,6 +533,10 @@ int qnoc_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
> + if (ret)
> + return ret;
> +
> if (desc->has_bus_pd) {
> ret = dev_pm_domain_attach(dev, true);
> goto err_disable_clks;
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index d4401f35f6d2..a4ef45b4a9e0 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -20,24 +20,32 @@ enum qcom_icc_type {
> QCOM_ICC_QNOC,
> };
>
> +#define NUM_BUS_CLKS 2
> +
> /**
> * struct qcom_icc_provider - Qualcomm specific interconnect provider
> * @provider: generic interconnect provider
> * @num_bus_clks: the total number of bus_clks clk_bulk_data entries
> + * @num_intf_clks: the total number of intf_clks clk_bulk_data entries
> * @type: the ICC provider type
> * @regmap: regmap for QoS registers read/write access
> * @qos_offset: offset to QoS registers
> * @bus_clk_rate: bus clock rate in Hz
> * @bus_clks: the clk_bulk_data table of bus clocks
> + * @intf_clks: a clk_bulk_data array of interface clocks
> + * @is_on: whether the bus is powered on
> */
> struct qcom_icc_provider {
> struct icc_provider provider;
> int num_bus_clks;
> + int num_intf_clks;
> enum qcom_icc_type type;
> struct regmap *regmap;
> unsigned int qos_offset;
> - u64 *bus_clk_rate;
> - struct clk_bulk_data bus_clks[];
> + u64 bus_clk_rate[NUM_BUS_CLKS];
> + struct clk_bulk_data bus_clks[NUM_BUS_CLKS];
> + struct clk_bulk_data *intf_clks;
> + bool is_on;
> };
>
> /**
> @@ -93,6 +101,8 @@ struct qcom_icc_desc {
> size_t num_nodes;
> const char * const *bus_clocks;
> size_t num_bus_clocks;
> + const char * const *intf_clocks;
> + size_t num_intf_clocks;
> bool has_bus_pd;
> enum qcom_icc_type type;
> const struct regmap_config *regmap_cfg;
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 69fc50a6fa5c..1a5e0ad36cc4 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -21,21 +21,17 @@
> #include "smd-rpm.h"
> #include "msm8996.h"
>
> -static const char * const bus_mm_clocks[] = {
> - "bus",
> - "bus_a",
> +static const char * const mm_intf_clocks[] = {
> "iface"
> };
>
> -static const char * const bus_a0noc_clocks[] = {
> +static const char * const a0noc_intf_clocks[] = {
> "aggre0_snoc_axi",
> "aggre0_cnoc_ahb",
> "aggre0_noc_mpu_cfg"
> };
>
> -static const char * const bus_a2noc_clocks[] = {
> - "bus",
> - "bus_a",
> +static const char * const a2noc_intf_clocks[] = {
> "aggre2_ufs_axi",
> "ufs_axi"
> };
> @@ -1821,8 +1817,8 @@ static const struct qcom_icc_desc msm8996_a0noc = {
> .type = QCOM_ICC_NOC,
> .nodes = a0noc_nodes,
> .num_nodes = ARRAY_SIZE(a0noc_nodes),
> - .bus_clocks = bus_a0noc_clocks,
> - .num_bus_clocks = ARRAY_SIZE(bus_a0noc_clocks),
> + .intf_clocks = a0noc_intf_clocks,
> + .num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
> .has_bus_pd = true,
> .regmap_cfg = &msm8996_a0noc_regmap_config
> };
> @@ -1866,8 +1862,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
> .type = QCOM_ICC_NOC,
> .nodes = a2noc_nodes,
> .num_nodes = ARRAY_SIZE(a2noc_nodes),
> - .bus_clocks = bus_a2noc_clocks,
> - .num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
> + .intf_clocks = a2noc_intf_clocks,
> + .num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
> .regmap_cfg = &msm8996_a2noc_regmap_config
> };
>
> @@ -2005,8 +2001,8 @@ static const struct qcom_icc_desc msm8996_mnoc = {
> .type = QCOM_ICC_NOC,
> .nodes = mnoc_nodes,
> .num_nodes = ARRAY_SIZE(mnoc_nodes),
> - .bus_clocks = bus_mm_clocks,
> - .num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
> + .intf_clocks = mm_intf_clocks,
> + .num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
> .regmap_cfg = &msm8996_mnoc_regmap_config
> };
>
> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> index a22ba821efbf..0e8a96f4ce90 100644
> --- a/drivers/interconnect/qcom/sdm660.c
> +++ b/drivers/interconnect/qcom/sdm660.c
> @@ -127,15 +127,11 @@ enum {
> SDM660_SNOC,
> };
>
> -static const char * const bus_mm_clocks[] = {
> - "bus",
> - "bus_a",
> +static const char * const mm_intf_clocks[] = {
> "iface",
> };
>
> -static const char * const bus_a2noc_clocks[] = {
> - "bus",
> - "bus_a",
> +static const char * const a2noc_intf_clocks[] = {
> "ipa",
> "ufs_axi",
> "aggre2_ufs_axi",
> @@ -1516,8 +1512,8 @@ static const struct qcom_icc_desc sdm660_a2noc = {
> .type = QCOM_ICC_NOC,
> .nodes = sdm660_a2noc_nodes,
> .num_nodes = ARRAY_SIZE(sdm660_a2noc_nodes),
> - .bus_clocks = bus_a2noc_clocks,
> - .num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
> + .intf_clocks = a2noc_intf_clocks,
> + .num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
> .regmap_cfg = &sdm660_a2noc_regmap_config,
> };
>
> @@ -1659,8 +1655,8 @@ static const struct qcom_icc_desc sdm660_mnoc = {
> .type = QCOM_ICC_NOC,
> .nodes = sdm660_mnoc_nodes,
> .num_nodes = ARRAY_SIZE(sdm660_mnoc_nodes),
> - .bus_clocks = bus_mm_clocks,
> - .num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
> + .intf_clocks = mm_intf_clocks,
> + .num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
> .regmap_cfg = &sdm660_mnoc_regmap_config,
> };
>
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists