| 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: <29ec10fa-1ca4-43eb-a865-7219d39c7140@kernel.org>
Date: Wed, 10 Sep 2025 11:39:41 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: webgeek1234@...il.com, Rob Herring <robh@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Thierry Reding
<thierry.reding@...il.com>, Jonathan Hunter <jonathanh@...dia.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>, Dmitry Osipenko <digetx@...il.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 5/9] memory: tegra210: Support interconnect framework
On 06/09/2025 22:16, Aaron Kling via B4 Relay wrote:
> +
> +static int tegra_emc_interconnect_init(struct tegra210_emc *emc)
> +{
> + const struct tegra_mc_soc *soc = emc->mc->soc;
> + struct icc_node *node;
> + int err;
> +
> + emc->icc_provider.dev = emc->dev;
> + emc->icc_provider.set = emc_icc_set;
> + emc->icc_provider.data = &emc->icc_provider;
> + emc->icc_provider.aggregate = soc->icc_ops->aggregate;
> + emc->icc_provider.xlate_extended = emc_of_icc_xlate_extended;
> + emc->icc_provider.get_bw = tegra_emc_icc_get_init_bw;
> +
> + icc_provider_init(&emc->icc_provider);
> +
> + /* create External Memory Controller node */
> + node = icc_node_create(TEGRA_ICC_EMC);
> + if (IS_ERR(node)) {
> + err = PTR_ERR(node);
> + goto err_msg;
return dev_err_probe
> + }
> +
> + node->name = "External Memory Controller";
> + icc_node_add(node, &emc->icc_provider);
> +
> + /* link External Memory Controller to External Memory (DRAM) */
> + err = icc_link_create(node, TEGRA_ICC_EMEM);
> + if (err)
> + goto remove_nodes;
> +
> + /* create External Memory node */
> + node = icc_node_create(TEGRA_ICC_EMEM);
> + if (IS_ERR(node)) {
> + err = PTR_ERR(node);
> + goto remove_nodes;
> + }
> +
> + node->name = "External Memory (DRAM)";
> + icc_node_add(node, &emc->icc_provider);
> +
> + err = icc_provider_register(&emc->icc_provider);
> + if (err)
> + goto remove_nodes;
> +
> + return 0;
> +
> +remove_nodes:
> + icc_nodes_remove(&emc->icc_provider);
> +err_msg:
> + dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
Write descriptive error messages or skip it. It's printing errors also
on ENOMEM.
I will send a patch to fix existing code.
> +
> + return err;
> +}
> +
> +static int tegra_emc_opp_table_init(struct tegra210_emc *emc)
> +{
> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> + struct dev_pm_opp *opp;
> + unsigned long rate;
> + int opp_token, err, max_opps, i;
> +
> + err = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
> + if (err < 0) {
> + dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
> + return err;
return dev_err_probe
> + }
> + opp_token = err;
> +
> + err = dev_pm_opp_of_add_table(emc->dev);
> + if (err) {
> + if (err == -ENODEV)
> + dev_warn(emc->dev, "OPP table not found, please update your device tree\n");
> + else
> + dev_err(emc->dev, "failed to add OPP table: %d\n", err);
> +
> + goto put_hw_table;
> + }
> +
> + max_opps = dev_pm_opp_get_opp_count(emc->dev);
> + if (max_opps <= 0) {
> + dev_err(emc->dev, "Failed to add OPPs\n");
> + goto remove_table;
> + }
> +
> + if (emc->num_timings != max_opps) {
> + dev_err(emc->dev, "OPP table does not match emc table\n");
> + goto remove_table;
> + }
> +
> + for (i = 0; i < emc->num_timings; i++) {
> + rate = emc->timings[i].rate * 1000;
> + opp = dev_pm_opp_find_freq_exact(emc->dev, rate, true);
> + if (IS_ERR(opp)) {
> + dev_err(emc->dev, "Rate %lu not found in OPP table\n", rate);
> + goto remove_table;
> + }
> +
> + dev_pm_opp_put(opp);
> + }
> +
> + dev_info_once(emc->dev, "OPP HW ver. 0x%x, current clock rate %lu MHz\n",
> + hw_version, clk_get_rate(emc->clk) / 1000000);
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(emc->dev);
> +put_hw_table:
> + dev_pm_opp_put_supported_hw(opp_token);
> +
> + return err;
> +}
> +
...
> diff --git a/drivers/memory/tegra/tegra210.c b/drivers/memory/tegra/tegra210.c
> index cfa61dd885577a8fbd79c396a1316101197ca1f2..20828a07d2d0cafa739b534c20c12f065b276669 100644
> --- a/drivers/memory/tegra/tegra210.c
> +++ b/drivers/memory/tegra/tegra210.c
> @@ -3,6 +3,9 @@
> * Copyright (C) 2015 NVIDIA CORPORATION. All rights reserved.
> */
>
> +#include <linux/of.h>
> +#include <linux/device.h>
> +
> #include <dt-bindings/memory/tegra210-mc.h>
>
> #include "mc.h"
> @@ -1273,6 +1276,83 @@ static const struct tegra_mc_reset tegra210_mc_resets[] = {
> TEGRA210_MC_RESET(TSECB, 0x970, 0x974, 13),
> };
>
> +static int tegra210_mc_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> + /* TODO: program PTSA */
> + return 0;
> +}
> +
> +static int tegra210_mc_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> + u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> + /*
> + * ISO clients need to reserve extra bandwidth up-front because
> + * there could be high bandwidth pressure during initial filling
> + * of the client's FIFO buffers. Secondly, we need to take into
> + * account impurities of the memory subsystem.
> + */
> + if (tag & TEGRA_MC_ICC_TAG_ISO)
> + peak_bw = tegra_mc_scale_percents(peak_bw, 400);
> +
> + *agg_avg += avg_bw;
> + *agg_peak = max(*agg_peak, peak_bw);
> +
> + return 0;
> +}
> +
> +static struct icc_node_data *
> +tegra210_mc_of_icc_xlate_extended(const struct of_phandle_args *spec, void *data)
> +{
> + struct tegra_mc *mc = icc_provider_to_tegra_mc(data);
> + const struct tegra_mc_client *client;
> + unsigned int i, idx = spec->args[0];
> + struct icc_node_data *ndata;
> + struct icc_node *node;
> +
> + list_for_each_entry(node, &mc->provider.nodes, node_list) {
> + if (node->id != idx)
> + continue;
> +
> + ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
> + if (!ndata)
> + return ERR_PTR(-ENOMEM);
> +
> + client = &mc->soc->clients[idx];
> + ndata->node = node;
> +
> + switch (client->swgroup) {
> + case TEGRA_SWGROUP_DC:
> + case TEGRA_SWGROUP_DCB:
> + case TEGRA_SWGROUP_PTC:
> + case TEGRA_SWGROUP_VI:
> + /* these clients are isochronous by default */
> + ndata->tag = TEGRA_MC_ICC_TAG_ISO;
> + break;
> +
> + default:
> + ndata->tag = TEGRA_MC_ICC_TAG_DEFAULT;
> + break;
> + }
> +
> + return ndata;
> + }
> +
> + for (i = 0; i < mc->soc->num_clients; i++) {
> + if (mc->soc->clients[i].id == idx)
> + return ERR_PTR(-EPROBE_DEFER);
This feels like more reasonable use of deferred probe, but I wonder how
is this condition possible.
This xlate is called at earliest after MC ICC is registered. At this
tage all nodes are created, so previous list_for_each_entry() should
succeed and return. If previous list_for_each_entry() did not return,
how can you find a matching client for which the node will be added
later, thus deferred probe?
This is again existing code, of course :)
Best regards,
Krzysztof
Powered by blists - more mailing lists