The current interconnect provider interface is inherently racy as
providers are expected to be registered before being fully initialised.
This can specifically cause racing DT lookups to fail as I recently
noticed when the Qualcomm cpufreq driver failed to probe:
of_icc_xlate_onecell: invalid index 0
cpu cpu0: error -EINVAL: error finding src node
cpu cpu0: dev_pm_opp_of_find_icc_paths: Unable to get path0: -22
qcom-cpufreq-hw: probe of 18591000.cpufreq failed with error -22
This only happens very rarely, but the bug is easily reproduced by
increasing the race window by adding an msleep() after registering
osm-l3 interconnect provider.
Note that the Qualcomm cpufreq driver is especially susceptible to this
race as the interconnect path is looked up from the CPU nodes so that
driver core does not guarantee the probe order even when device links
are enabled (which they not always are).
This series adds a new interconnect provider registration API which is
used to fix up the interconnect drivers before removing the old racy
API.
Included are also a number of fixes for other bugs found while preparing
the series.
Johan
Johan Hovold (23):
interconnect: fix mem leak when freeing nodes
interconnect: fix icc_provider_del() error handling
interconnect: fix provider registration API
interconnect: imx: fix registration race
interconnect: qcom: osm-l3: fix registration race
interconnect: qcom: rpm: fix probe child-node error handling
interconnect: qcom: rpm: fix probe PM domain error handling
interconnect: qcom: rpm: fix registration race
interconnect: qcom: rpmh: fix probe child-node error handling
interconnect: qcom: rpmh: fix registration race
interconnect: qcom: msm8974: fix registration race
interconnect: qcom: sm8450: fix registration race
interconnect: qcom: sm8550: fix registration race
interconnect: exynos: fix node leak in probe PM QoS error path
interconnect: exynos: fix registration race
interconnect: exynos: drop redundant link destroy
memory: tegra: fix interconnect registration race
memory: tegra124-emc: fix interconnect registration race
memory: tegra20-emc: fix interconnect registration race
memory: tegra30-emc: fix interconnect registration race
interconnect: drop racy registration API
interconnect: drop unused icc_get() interface
interconnect: drop unused icc_link_destroy() interface
drivers/interconnect/core.c | 149 +++++---------------------
drivers/interconnect/imx/imx.c | 20 ++--
drivers/interconnect/qcom/icc-rpm.c | 33 +++---
drivers/interconnect/qcom/icc-rpmh.c | 30 ++++--
drivers/interconnect/qcom/msm8974.c | 20 ++--
drivers/interconnect/qcom/osm-l3.c | 14 ++-
drivers/interconnect/qcom/sm8450.c | 22 ++--
drivers/interconnect/qcom/sm8550.c | 22 ++--
drivers/interconnect/samsung/exynos.c | 30 +++---
drivers/memory/tegra/mc.c | 16 ++-
drivers/memory/tegra/tegra124-emc.c | 12 +--
drivers/memory/tegra/tegra20-emc.c | 12 +--
drivers/memory/tegra/tegra30-emc.c | 12 +--
include/linux/interconnect-provider.h | 19 ++--
include/linux/interconnect.h | 8 --
15 files changed, 154 insertions(+), 265 deletions(-)
--
2.39.1
Make sure to clean up and release resources properly also in case probe
fails when populating child devices.
Fixes: e39bf2972c6e ("interconnect: icc-rpm: Support child NoC device probe")
Cc: [email protected] # 5.17
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/icc-rpm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index df3196f72536..91778cfcbc65 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -541,8 +541,11 @@ int qnoc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, qp);
/* Populate child NoC devices if any */
- if (of_get_child_count(dev->of_node) > 0)
- return of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (of_get_child_count(dev->of_node) > 0) {
+ ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (ret)
+ goto err;
+ }
return 0;
err:
--
2.39.1
Make sure to disable clocks also in case attaching the power domain
fails.
Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
Cc: [email protected] # 5.17
Cc: Yassine Oudjana <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/icc-rpm.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 91778cfcbc65..da595059cafd 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -498,8 +498,7 @@ int qnoc_probe(struct platform_device *pdev)
if (desc->has_bus_pd) {
ret = dev_pm_domain_attach(dev, true);
- if (ret)
- return ret;
+ goto err_disable_clks;
}
provider = &qp->provider;
@@ -514,8 +513,7 @@ int qnoc_probe(struct platform_device *pdev)
ret = icc_provider_add(provider);
if (ret) {
dev_err(dev, "error adding interconnect provider: %d\n", ret);
- clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
- return ret;
+ goto err_disable_clks;
}
for (i = 0; i < num_nodes; i++) {
@@ -550,8 +548,9 @@ int qnoc_probe(struct platform_device *pdev)
return 0;
err:
icc_nodes_remove(provider);
- clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
icc_provider_del(provider);
+err_disable_clks:
+ clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
return ret;
}
--
2.39.1
Make sure to clean up and release resources properly also in case probe
fails when populating child devices.
Fixes: 57eb14779dfd ("interconnect: qcom: icc-rpmh: Support child NoC device probe")
Cc: [email protected] # 6.0
Cc: Luca Weiss <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/icc-rpmh.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index fd17291c61eb..5168bbf3d92f 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -235,8 +235,11 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, qp);
/* Populate child NoC devices if any */
- if (of_get_child_count(dev->of_node) > 0)
- return of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (of_get_child_count(dev->of_node) > 0) {
+ ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (ret)
+ goto err;
+ }
return 0;
err:
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to trigger a NULL-pointer
deference when either a NULL pointer or not fully initialised node is
returned from exynos_generic_icc_xlate().
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 2f95b9d5cf0b ("interconnect: Add generic interconnect driver for Exynos SoCs")
Cc: [email protected] # 5.11
Cc: Sylwester Nawrocki <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/samsung/exynos.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
index e70665899482..72e42603823b 100644
--- a/drivers/interconnect/samsung/exynos.c
+++ b/drivers/interconnect/samsung/exynos.c
@@ -98,12 +98,13 @@ static int exynos_generic_icc_remove(struct platform_device *pdev)
struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
struct icc_node *parent_node, *node = priv->node;
+ icc_provider_deregister(&priv->provider);
+
parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
if (parent_node && !IS_ERR(parent_node))
icc_link_destroy(node, parent_node);
icc_nodes_remove(&priv->provider);
- icc_provider_del(&priv->provider);
return 0;
}
@@ -132,15 +133,11 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
provider->inter_set = true;
provider->data = priv;
- ret = icc_provider_add(provider);
- if (ret < 0)
- return ret;
+ icc_provider_init(provider);
icc_node = icc_node_create(pdev->id);
- if (IS_ERR(icc_node)) {
- ret = PTR_ERR(icc_node);
- goto err_prov_del;
- }
+ if (IS_ERR(icc_node))
+ return PTR_ERR(icc_node);
priv->node = icc_node;
icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
@@ -171,14 +168,17 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
goto err_pmqos_del;
}
+ ret = icc_provider_register(provider);
+ if (ret < 0)
+ goto err_pmqos_del;
+
return 0;
err_pmqos_del:
dev_pm_qos_remove_request(&priv->qos_req);
err_node_del:
icc_nodes_remove(provider);
-err_prov_del:
- icc_provider_del(provider);
+
return ret;
}
--
2.39.1
There is no longer any need to explicitly destroy node links as this is
now done when the node is destroyed as part of icc_nodes_remove().
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/samsung/exynos.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
index 72e42603823b..ebf09bbf725b 100644
--- a/drivers/interconnect/samsung/exynos.c
+++ b/drivers/interconnect/samsung/exynos.c
@@ -96,14 +96,8 @@ static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
static int exynos_generic_icc_remove(struct platform_device *pdev)
{
struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
- struct icc_node *parent_node, *node = priv->node;
icc_provider_deregister(&priv->provider);
-
- parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
- if (parent_node && !IS_ERR(parent_node))
- icc_link_destroy(node, parent_node);
-
icc_nodes_remove(&priv->provider);
return 0;
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 380def2d4cf2 ("memory: tegra124: Support interconnect framework")
Cc: [email protected] # 5.12
Cc: Dmitry Osipenko <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/memory/tegra/tegra124-emc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
index 85bc936c02f9..00ed2b6a0d1b 100644
--- a/drivers/memory/tegra/tegra124-emc.c
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -1351,15 +1351,13 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
emc->provider.aggregate = soc->icc_ops->aggregate;
emc->provider.xlate_extended = emc_of_icc_xlate_extended;
- err = icc_provider_add(&emc->provider);
- if (err)
- goto err_msg;
+ icc_provider_init(&emc->provider);
/* create External Memory Controller node */
node = icc_node_create(TEGRA_ICC_EMC);
if (IS_ERR(node)) {
err = PTR_ERR(node);
- goto del_provider;
+ goto err_msg;
}
node->name = "External Memory Controller";
@@ -1380,12 +1378,14 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
node->name = "External Memory (DRAM)";
icc_node_add(node, &emc->provider);
+ err = icc_provider_register(&emc->provider);
+ if (err)
+ goto remove_nodes;
+
return 0;
remove_nodes:
icc_nodes_remove(&emc->provider);
-del_provider:
- icc_provider_del(&emc->provider);
err_msg:
dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
--
2.39.1
The node link array is allocated when adding links to a node but is not
deallocated when nodes are destroyed.
Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
Cc: [email protected] # 5.1
Cc: Georgi Djakov <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 423f875d4b54..dc61620a0191 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -850,6 +850,7 @@ void icc_node_destroy(int id)
mutex_unlock(&icc_lock);
+ kfree(node->links);
kfree(node);
}
EXPORT_SYMBOL_GPL(icc_node_destroy);
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail:
of_icc_xlate_onecell: invalid index 0
cpu cpu0: error -EINVAL: error finding src node
cpu cpu0: dev_pm_opp_of_find_icc_paths: Unable to get path0: -22
qcom-cpufreq-hw: probe of 18591000.cpufreq failed with error -22
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 5bc9900addaf ("interconnect: qcom: Add OSM L3 interconnect provider support")
Cc: [email protected] # 5.7
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/osm-l3.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index 5fa171087425..3a1cbfe3e481 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -158,8 +158,8 @@ static int qcom_osm_l3_remove(struct platform_device *pdev)
{
struct qcom_osm_l3_icc_provider *qp = platform_get_drvdata(pdev);
+ icc_provider_deregister(&qp->provider);
icc_nodes_remove(&qp->provider);
- icc_provider_del(&qp->provider);
return 0;
}
@@ -245,14 +245,9 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
provider->set = qcom_osm_l3_set;
provider->aggregate = icc_std_aggregate;
provider->xlate = of_icc_xlate_onecell;
- INIT_LIST_HEAD(&provider->nodes);
provider->data = data;
- ret = icc_provider_add(provider);
- if (ret) {
- dev_err(&pdev->dev, "error adding interconnect provider\n");
- return ret;
- }
+ icc_provider_init(provider);
for (i = 0; i < num_nodes; i++) {
size_t j;
@@ -275,12 +270,15 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+ ret = icc_provider_register(provider);
+ if (ret)
+ goto err;
+
platform_set_drvdata(pdev, qp);
return 0;
err:
icc_nodes_remove(provider);
- icc_provider_del(provider);
return ret;
}
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 06f079816d4c ("memory: tegra-mc: Add interconnect framework")
Cc: [email protected] # 5.11
Cc: Dmitry Osipenko <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/memory/tegra/mc.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 592907546ee6..5cd28619ea9f 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -794,16 +794,12 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
mc->provider.aggregate = mc->soc->icc_ops->aggregate;
mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended;
- err = icc_provider_add(&mc->provider);
- if (err)
- return err;
+ icc_provider_init(&mc->provider);
/* create Memory Controller node */
node = icc_node_create(TEGRA_ICC_MC);
- if (IS_ERR(node)) {
- err = PTR_ERR(node);
- goto del_provider;
- }
+ if (IS_ERR(node))
+ return PTR_ERR(node);
node->name = "Memory Controller";
icc_node_add(node, &mc->provider);
@@ -830,12 +826,14 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
goto remove_nodes;
}
+ err = icc_provider_register(&mc->provider);
+ if (err)
+ goto remove_nodes;
+
return 0;
remove_nodes:
icc_nodes_remove(&mc->provider);
-del_provider:
- icc_provider_del(&mc->provider);
return err;
}
--
2.39.1
The interconnect framework currently expects that providers are only
removed when there are no users and after all nodes have been removed.
There is currently nothing that guarantees this to be the case and the
framework does not do any reference counting, but refusing to remove the
provider is never correct as that would leave a dangling pointer to a
resource that is about to be released in the global provider list (e.g.
accessible through debugfs).
Replace the current sanity checks with WARN_ON() so that the provider is
always removed.
Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
Cc: [email protected] # 5.1: 680f8666baf6: interconnect: Make icc_provider_del() return void
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index dc61620a0191..43c5c8503ee8 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1062,18 +1062,8 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
void icc_provider_del(struct icc_provider *provider)
{
mutex_lock(&icc_lock);
- if (provider->users) {
- pr_warn("interconnect provider still has %d users\n",
- provider->users);
- mutex_unlock(&icc_lock);
- return;
- }
-
- if (!list_empty(&provider->nodes)) {
- pr_warn("interconnect provider still has nodes\n");
- mutex_unlock(&icc_lock);
- return;
- }
+ WARN_ON(provider->users);
+ WARN_ON(!list_empty(&provider->nodes));
list_del(&provider->provider_list);
mutex_unlock(&icc_lock);
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 62feb14ee8a3 ("interconnect: qcom: Consolidate interconnect RPM support")
Fixes: 30c8fa3ec61a ("interconnect: qcom: Add MSM8916 interconnect provider driver")
Cc: [email protected] # 5.7
Cc: Jun Nie <[email protected]>
Cc: Georgi Djakov <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/icc-rpm.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index da595059cafd..4d0997b210f7 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -502,7 +502,6 @@ int qnoc_probe(struct platform_device *pdev)
}
provider = &qp->provider;
- INIT_LIST_HEAD(&provider->nodes);
provider->dev = dev;
provider->set = qcom_icc_set;
provider->pre_aggregate = qcom_icc_pre_bw_aggregate;
@@ -510,11 +509,7 @@ int qnoc_probe(struct platform_device *pdev)
provider->xlate_extended = qcom_icc_xlate_extended;
provider->data = data;
- ret = icc_provider_add(provider);
- if (ret) {
- dev_err(dev, "error adding interconnect provider: %d\n", ret);
- goto err_disable_clks;
- }
+ icc_provider_init(provider);
for (i = 0; i < num_nodes; i++) {
size_t j;
@@ -522,7 +517,7 @@ int qnoc_probe(struct platform_device *pdev)
node = icc_node_create(qnodes[i]->id);
if (IS_ERR(node)) {
ret = PTR_ERR(node);
- goto err;
+ goto err_remove_nodes;
}
node->name = qnodes[i]->name;
@@ -536,19 +531,25 @@ int qnoc_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+ ret = icc_provider_register(provider);
+ if (ret)
+ goto err_remove_nodes;
+
platform_set_drvdata(pdev, qp);
/* Populate child NoC devices if any */
if (of_get_child_count(dev->of_node) > 0) {
ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
if (ret)
- goto err;
+ goto err_deregister_provider;
}
return 0;
-err:
+
+err_deregister_provider:
+ icc_provider_deregister(provider);
+err_remove_nodes:
icc_nodes_remove(provider);
- icc_provider_del(provider);
err_disable_clks:
clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
@@ -560,9 +561,9 @@ int qnoc_remove(struct platform_device *pdev)
{
struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
+ icc_provider_deregister(&qp->provider);
icc_nodes_remove(&qp->provider);
clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
- icc_provider_del(&qp->provider);
return 0;
}
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: fafc114a468e ("interconnect: qcom: Add SM8450 interconnect provider driver")
Cc: [email protected] # 5.17
Cc: Vinod Koul <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/sm8450.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/interconnect/qcom/sm8450.c b/drivers/interconnect/qcom/sm8450.c
index e3a12e3d6e06..c7a8bbf102a3 100644
--- a/drivers/interconnect/qcom/sm8450.c
+++ b/drivers/interconnect/qcom/sm8450.c
@@ -1876,9 +1876,10 @@ static int qnoc_probe(struct platform_device *pdev)
provider->pre_aggregate = qcom_icc_pre_aggregate;
provider->aggregate = qcom_icc_aggregate;
provider->xlate_extended = qcom_icc_xlate_extended;
- INIT_LIST_HEAD(&provider->nodes);
provider->data = data;
+ icc_provider_init(provider);
+
qp->dev = &pdev->dev;
qp->bcms = desc->bcms;
qp->num_bcms = desc->num_bcms;
@@ -1887,12 +1888,6 @@ static int qnoc_probe(struct platform_device *pdev)
if (IS_ERR(qp->voter))
return PTR_ERR(qp->voter);
- ret = icc_provider_add(provider);
- if (ret) {
- dev_err(&pdev->dev, "error adding interconnect provider\n");
- return ret;
- }
-
for (i = 0; i < qp->num_bcms; i++)
qcom_icc_bcm_init(qp->bcms[i], &pdev->dev);
@@ -1905,7 +1900,7 @@ static int qnoc_probe(struct platform_device *pdev)
node = icc_node_create(qnodes[i]->id);
if (IS_ERR(node)) {
ret = PTR_ERR(node);
- goto err;
+ goto err_remove_nodes;
}
node->name = qnodes[i]->name;
@@ -1919,12 +1914,17 @@ static int qnoc_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+ ret = icc_provider_register(provider);
+ if (ret)
+ goto err_remove_nodes;
+
platform_set_drvdata(pdev, qp);
return 0;
-err:
+
+err_remove_nodes:
icc_nodes_remove(provider);
- icc_provider_del(provider);
+
return ret;
}
@@ -1932,8 +1932,8 @@ static int qnoc_remove(struct platform_device *pdev)
{
struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
+ icc_provider_deregister(&qp->provider);
icc_nodes_remove(&qp->provider);
- icc_provider_del(&qp->provider);
return 0;
}
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 976daac4a1c5 ("interconnect: qcom: Consolidate interconnect RPMh support")
Cc: [email protected] # 5.7
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/icc-rpmh.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index 5168bbf3d92f..fdb5e58e408b 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -192,9 +192,10 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
provider->pre_aggregate = qcom_icc_pre_aggregate;
provider->aggregate = qcom_icc_aggregate;
provider->xlate_extended = qcom_icc_xlate_extended;
- INIT_LIST_HEAD(&provider->nodes);
provider->data = data;
+ icc_provider_init(provider);
+
qp->dev = dev;
qp->bcms = desc->bcms;
qp->num_bcms = desc->num_bcms;
@@ -203,10 +204,6 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
if (IS_ERR(qp->voter))
return PTR_ERR(qp->voter);
- ret = icc_provider_add(provider);
- if (ret)
- return ret;
-
for (i = 0; i < qp->num_bcms; i++)
qcom_icc_bcm_init(qp->bcms[i], dev);
@@ -218,7 +215,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
node = icc_node_create(qn->id);
if (IS_ERR(node)) {
ret = PTR_ERR(node);
- goto err;
+ goto err_remove_nodes;
}
node->name = qn->name;
@@ -232,19 +229,27 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+
+ ret = icc_provider_register(provider);
+ if (ret)
+ goto err_remove_nodes;
+
platform_set_drvdata(pdev, qp);
/* Populate child NoC devices if any */
if (of_get_child_count(dev->of_node) > 0) {
ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
if (ret)
- goto err;
+ goto err_deregister_provider;
}
return 0;
-err:
+
+err_deregister_provider:
+ icc_provider_deregister(provider);
+err_remove_nodes:
icc_nodes_remove(provider);
- icc_provider_del(provider);
+
return ret;
}
EXPORT_SYMBOL_GPL(qcom_icc_rpmh_probe);
@@ -253,8 +258,8 @@ int qcom_icc_rpmh_remove(struct platform_device *pdev)
{
struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
+ icc_provider_deregister(&qp->provider);
icc_nodes_remove(&qp->provider);
- icc_provider_del(&qp->provider);
return 0;
}
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: d5ef16ba5fbe ("memory: tegra20: Support interconnect framework")
Cc: [email protected] # 5.11
Cc: Dmitry Osipenko <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/memory/tegra/tegra20-emc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index bd4e37b6552d..fd595c851a27 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -1021,15 +1021,13 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
emc->provider.aggregate = soc->icc_ops->aggregate;
emc->provider.xlate_extended = emc_of_icc_xlate_extended;
- err = icc_provider_add(&emc->provider);
- if (err)
- goto err_msg;
+ icc_provider_init(&emc->provider);
/* create External Memory Controller node */
node = icc_node_create(TEGRA_ICC_EMC);
if (IS_ERR(node)) {
err = PTR_ERR(node);
- goto del_provider;
+ goto err_msg;
}
node->name = "External Memory Controller";
@@ -1050,12 +1048,14 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
node->name = "External Memory (DRAM)";
icc_node_add(node, &emc->provider);
+ err = icc_provider_register(&emc->provider);
+ if (err)
+ goto remove_nodes;
+
return 0;
remove_nodes:
icc_nodes_remove(&emc->provider);
-del_provider:
- icc_provider_del(&emc->provider);
err_msg:
dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: d5ef16ba5fbe ("memory: tegra20: Support interconnect framework")
Cc: [email protected] # 5.11
Cc: Dmitry Osipenko <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/memory/tegra/tegra30-emc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
index 77706e9bc543..c91e9b7e2e01 100644
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -1533,15 +1533,13 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
emc->provider.aggregate = soc->icc_ops->aggregate;
emc->provider.xlate_extended = emc_of_icc_xlate_extended;
- err = icc_provider_add(&emc->provider);
- if (err)
- goto err_msg;
+ icc_provider_init(&emc->provider);
/* create External Memory Controller node */
node = icc_node_create(TEGRA_ICC_EMC);
if (IS_ERR(node)) {
err = PTR_ERR(node);
- goto del_provider;
+ goto err_msg;
}
node->name = "External Memory Controller";
@@ -1562,12 +1560,14 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
node->name = "External Memory (DRAM)";
icc_node_add(node, &emc->provider);
+ err = icc_provider_register(&emc->provider);
+ if (err)
+ goto remove_nodes;
+
return 0;
remove_nodes:
icc_nodes_remove(&emc->provider);
-del_provider:
- icc_provider_del(&emc->provider);
err_msg:
dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
--
2.39.1
The icc_get() interface can be used to lookup an interconnect path based
on global node ids. There has never been any users of this interface and
all lookups are currently done from the devicetree.
Remove the unused icc_get() interface.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 52 ++----------------------------------
include/linux/interconnect.h | 8 ------
2 files changed, 2 insertions(+), 58 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index b8917823fd95..8cca5e9a9d5f 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -588,7 +588,7 @@ EXPORT_SYMBOL_GPL(icc_set_tag);
/**
* icc_get_name() - Get name of the icc path
- * @path: reference to the path returned by icc_get()
+ * @path: interconnect path
*
* This function is used by an interconnect consumer to get the name of the icc
* path.
@@ -606,7 +606,7 @@ EXPORT_SYMBOL_GPL(icc_get_name);
/**
* icc_set_bw() - set bandwidth constraints on an interconnect path
- * @path: reference to the path returned by icc_get()
+ * @path: interconnect path
* @avg_bw: average bandwidth in kilobytes per second
* @peak_bw: peak bandwidth in kilobytes per second
*
@@ -705,54 +705,6 @@ int icc_disable(struct icc_path *path)
}
EXPORT_SYMBOL_GPL(icc_disable);
-/**
- * icc_get() - return a handle for path between two endpoints
- * @dev: the device requesting the path
- * @src_id: source device port id
- * @dst_id: destination device port id
- *
- * This function will search for a path between two endpoints and return an
- * icc_path handle on success. Use icc_put() to release
- * constraints when they are not needed anymore.
- * If the interconnect API is disabled, NULL is returned and the consumer
- * drivers will still build. Drivers are free to handle this specifically,
- * but they don't have to.
- *
- * Return: icc_path pointer on success, ERR_PTR() on error or NULL if the
- * interconnect API is disabled.
- */
-struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
-{
- struct icc_node *src, *dst;
- struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
-
- mutex_lock(&icc_lock);
-
- src = node_find(src_id);
- if (!src)
- goto out;
-
- dst = node_find(dst_id);
- if (!dst)
- goto out;
-
- path = path_find(dev, src, dst);
- if (IS_ERR(path)) {
- dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
- goto out;
- }
-
- path->name = kasprintf(GFP_KERNEL, "%s-%s", src->name, dst->name);
- if (!path->name) {
- kfree(path);
- path = ERR_PTR(-ENOMEM);
- }
-out:
- mutex_unlock(&icc_lock);
- return path;
-}
-EXPORT_SYMBOL_GPL(icc_get);
-
/**
* icc_put() - release the reference to the icc_path
* @path: interconnect path
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index 2b0e784ba771..97ac253df62c 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -40,8 +40,6 @@ struct icc_bulk_data {
#if IS_ENABLED(CONFIG_INTERCONNECT)
-struct icc_path *icc_get(struct device *dev, const int src_id,
- const int dst_id);
struct icc_path *of_icc_get(struct device *dev, const char *name);
struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
int devm_of_icc_bulk_get(struct device *dev, int num_paths, struct icc_bulk_data *paths);
@@ -61,12 +59,6 @@ void icc_bulk_disable(int num_paths, const struct icc_bulk_data *paths);
#else
-static inline struct icc_path *icc_get(struct device *dev, const int src_id,
- const int dst_id)
-{
- return NULL;
-}
-
static inline struct icc_path *of_icc_get(struct device *dev,
const char *name)
{
--
2.39.1
Now that all interconnect drivers have been converted to the new
provider registration API, the old racy interface can be removed.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 16 ----------------
include/linux/interconnect-provider.h | 11 -----------
2 files changed, 27 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 93d27ff8eef6..b8917823fd95 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1078,22 +1078,6 @@ void icc_provider_deregister(struct icc_provider *provider)
}
EXPORT_SYMBOL_GPL(icc_provider_deregister);
-int icc_provider_add(struct icc_provider *provider)
-{
- icc_provider_init(provider);
-
- return icc_provider_register(provider);
-}
-EXPORT_SYMBOL_GPL(icc_provider_add);
-
-void icc_provider_del(struct icc_provider *provider)
-{
- WARN_ON(!list_empty(&provider->nodes));
-
- icc_provider_deregister(provider);
-}
-EXPORT_SYMBOL_GPL(icc_provider_del);
-
static const struct of_device_id __maybe_unused ignore_list[] = {
{ .compatible = "qcom,sc7180-ipa-virt" },
{ .compatible = "qcom,sc8180x-ipa-virt" },
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index d12cd18aab3f..b9af9016a95e 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -125,8 +125,6 @@ int icc_nodes_remove(struct icc_provider *provider);
void icc_provider_init(struct icc_provider *provider);
int icc_provider_register(struct icc_provider *provider);
void icc_provider_deregister(struct icc_provider *provider);
-int icc_provider_add(struct icc_provider *provider);
-void icc_provider_del(struct icc_provider *provider);
struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
void icc_sync_state(struct device *dev);
@@ -179,15 +177,6 @@ static inline int icc_provider_register(struct icc_provider *provider)
static inline void icc_provider_deregister(struct icc_provider *provider) { }
-static inline int icc_provider_add(struct icc_provider *provider)
-{
- return -ENOTSUPP;
-}
-
-static inline void icc_provider_del(struct icc_provider *provider)
-{
-}
-
static inline struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
{
return ERR_PTR(-ENOTSUPP);
--
2.39.1
Make sure to add the newly allocated interconnect node to the provider
before adding the PM QoS request so that the node is freed on errors.
Fixes: 2f95b9d5cf0b ("interconnect: Add generic interconnect driver for Exynos SoCs")
Cc: [email protected] # 5.11
Cc: Sylwester Nawrocki <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/samsung/exynos.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
index 6559d8cf8068..e70665899482 100644
--- a/drivers/interconnect/samsung/exynos.c
+++ b/drivers/interconnect/samsung/exynos.c
@@ -149,6 +149,9 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
&priv->bus_clk_ratio))
priv->bus_clk_ratio = EXYNOS_ICC_DEFAULT_BUS_CLK_RATIO;
+ icc_node->data = priv;
+ icc_node_add(icc_node, provider);
+
/*
* Register a PM QoS request for the parent (devfreq) device.
*/
@@ -157,9 +160,6 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
if (ret < 0)
goto err_node_del;
- icc_node->data = priv;
- icc_node_add(icc_node, provider);
-
icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
if (IS_ERR(icc_parent_node)) {
ret = PTR_ERR(icc_parent_node);
--
2.39.1
The current interconnect provider interface is inherently racy as
providers are expected to be added before being fully initialised.
Specifically, nodes are currently not added and the provider data is not
initialised until after registering the provider which can cause racing
DT lookups to fail.
Add a new provider API which will be used to fix up the interconnect
drivers.
The old API is reimplemented using the new interface and will be removed
once all drivers have been fixed.
Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
Fixes: 87e3031b6fbd ("interconnect: Allow endpoints translation via DT")
Cc: [email protected] # 5.1
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 52 +++++++++++++++++++--------
include/linux/interconnect-provider.h | 12 +++++++
2 files changed, 50 insertions(+), 14 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 43c5c8503ee8..93d27ff8eef6 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1030,44 +1030,68 @@ int icc_nodes_remove(struct icc_provider *provider)
EXPORT_SYMBOL_GPL(icc_nodes_remove);
/**
- * icc_provider_add() - add a new interconnect provider
- * @provider: the interconnect provider that will be added into topology
+ * icc_provider_init() - initialize a new interconnect provider
+ * @provider: the interconnect provider to initialize
+ *
+ * Must be called before adding nodes to the provider.
+ */
+void icc_provider_init(struct icc_provider *provider)
+{
+ WARN_ON(!provider->set);
+
+ INIT_LIST_HEAD(&provider->nodes);
+}
+EXPORT_SYMBOL_GPL(icc_provider_init);
+
+/**
+ * icc_provider_register() - register a new interconnect provider
+ * @provider: the interconnect provider to register
*
* Return: 0 on success, or an error code otherwise
*/
-int icc_provider_add(struct icc_provider *provider)
+int icc_provider_register(struct icc_provider *provider)
{
- if (WARN_ON(!provider->set))
- return -EINVAL;
if (WARN_ON(!provider->xlate && !provider->xlate_extended))
return -EINVAL;
mutex_lock(&icc_lock);
-
- INIT_LIST_HEAD(&provider->nodes);
list_add_tail(&provider->provider_list, &icc_providers);
-
mutex_unlock(&icc_lock);
- dev_dbg(provider->dev, "interconnect provider added to topology\n");
+ dev_dbg(provider->dev, "interconnect provider registered\n");
return 0;
}
-EXPORT_SYMBOL_GPL(icc_provider_add);
+EXPORT_SYMBOL_GPL(icc_provider_register);
/**
- * icc_provider_del() - delete previously added interconnect provider
- * @provider: the interconnect provider that will be removed from topology
+ * icc_provider_deregister() - deregister an interconnect provider
+ * @provider: the interconnect provider to deregister
*/
-void icc_provider_del(struct icc_provider *provider)
+void icc_provider_deregister(struct icc_provider *provider)
{
mutex_lock(&icc_lock);
WARN_ON(provider->users);
- WARN_ON(!list_empty(&provider->nodes));
list_del(&provider->provider_list);
mutex_unlock(&icc_lock);
}
+EXPORT_SYMBOL_GPL(icc_provider_deregister);
+
+int icc_provider_add(struct icc_provider *provider)
+{
+ icc_provider_init(provider);
+
+ return icc_provider_register(provider);
+}
+EXPORT_SYMBOL_GPL(icc_provider_add);
+
+void icc_provider_del(struct icc_provider *provider)
+{
+ WARN_ON(!list_empty(&provider->nodes));
+
+ icc_provider_deregister(provider);
+}
EXPORT_SYMBOL_GPL(icc_provider_del);
static const struct of_device_id __maybe_unused ignore_list[] = {
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index cd5c5a27557f..d12cd18aab3f 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -122,6 +122,9 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst);
void icc_node_add(struct icc_node *node, struct icc_provider *provider);
void icc_node_del(struct icc_node *node);
int icc_nodes_remove(struct icc_provider *provider);
+void icc_provider_init(struct icc_provider *provider);
+int icc_provider_register(struct icc_provider *provider);
+void icc_provider_deregister(struct icc_provider *provider);
int icc_provider_add(struct icc_provider *provider);
void icc_provider_del(struct icc_provider *provider);
struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
@@ -167,6 +170,15 @@ static inline int icc_nodes_remove(struct icc_provider *provider)
return -ENOTSUPP;
}
+static inline void icc_provider_init(struct icc_provider *provider) { }
+
+static inline int icc_provider_register(struct icc_provider *provider)
+{
+ return -ENOTSUPP;
+}
+
+static inline void icc_provider_deregister(struct icc_provider *provider) { }
+
static inline int icc_provider_add(struct icc_provider *provider)
{
return -ENOTSUPP;
--
2.39.1
Now that the link array is deallocated when destroying nodes and the
explicit link removal has been dropped from the exynos driver there are
no further users of and no need for the icc_link_destroy() interface.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 46 ---------------------------
include/linux/interconnect-provider.h | 6 ----
2 files changed, 52 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 8cca5e9a9d5f..d40b35bdc6ef 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -859,52 +859,6 @@ int icc_link_create(struct icc_node *node, const int dst_id)
}
EXPORT_SYMBOL_GPL(icc_link_create);
-/**
- * icc_link_destroy() - destroy a link between two nodes
- * @src: pointer to source node
- * @dst: pointer to destination node
- *
- * Return: 0 on success, or an error code otherwise
- */
-int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
-{
- struct icc_node **new;
- size_t slot;
- int ret = 0;
-
- if (IS_ERR_OR_NULL(src))
- return -EINVAL;
-
- if (IS_ERR_OR_NULL(dst))
- return -EINVAL;
-
- mutex_lock(&icc_lock);
-
- for (slot = 0; slot < src->num_links; slot++)
- if (src->links[slot] == dst)
- break;
-
- if (WARN_ON(slot == src->num_links)) {
- ret = -ENXIO;
- goto out;
- }
-
- src->links[slot] = src->links[--src->num_links];
-
- new = krealloc(src->links, src->num_links * sizeof(*src->links),
- GFP_KERNEL);
- if (new)
- src->links = new;
- else
- ret = -ENOMEM;
-
-out:
- mutex_unlock(&icc_lock);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(icc_link_destroy);
-
/**
* icc_node_add() - add interconnect node to interconnect provider
* @node: pointer to the interconnect node
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index b9af9016a95e..e6d8aca6886d 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -118,7 +118,6 @@ int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
struct icc_node *icc_node_create(int id);
void icc_node_destroy(int id);
int icc_link_create(struct icc_node *node, const int dst_id);
-int icc_link_destroy(struct icc_node *src, struct icc_node *dst);
void icc_node_add(struct icc_node *node, struct icc_provider *provider);
void icc_node_del(struct icc_node *node);
int icc_nodes_remove(struct icc_provider *provider);
@@ -150,11 +149,6 @@ static inline int icc_link_create(struct icc_node *node, const int dst_id)
return -ENOTSUPP;
}
-static inline int icc_link_destroy(struct icc_node *src, struct icc_node *dst)
-{
- return -ENOTSUPP;
-}
-
static inline void icc_node_add(struct icc_node *node, struct icc_provider *provider)
{
}
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: 4e60a9568dc6 ("interconnect: qcom: add msm8974 driver")
Cc: [email protected] # 5.5
Cc: Brian Masney <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/msm8974.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/interconnect/qcom/msm8974.c b/drivers/interconnect/qcom/msm8974.c
index 5ea192f1141d..1828deaca443 100644
--- a/drivers/interconnect/qcom/msm8974.c
+++ b/drivers/interconnect/qcom/msm8974.c
@@ -692,7 +692,6 @@ static int msm8974_icc_probe(struct platform_device *pdev)
return ret;
provider = &qp->provider;
- INIT_LIST_HEAD(&provider->nodes);
provider->dev = dev;
provider->set = msm8974_icc_set;
provider->aggregate = icc_std_aggregate;
@@ -700,11 +699,7 @@ static int msm8974_icc_probe(struct platform_device *pdev)
provider->data = data;
provider->get_bw = msm8974_get_bw;
- ret = icc_provider_add(provider);
- if (ret) {
- dev_err(dev, "error adding interconnect provider: %d\n", ret);
- goto err_disable_clks;
- }
+ icc_provider_init(provider);
for (i = 0; i < num_nodes; i++) {
size_t j;
@@ -712,7 +707,7 @@ static int msm8974_icc_probe(struct platform_device *pdev)
node = icc_node_create(qnodes[i]->id);
if (IS_ERR(node)) {
ret = PTR_ERR(node);
- goto err_del_icc;
+ goto err_remove_nodes;
}
node->name = qnodes[i]->name;
@@ -729,15 +724,16 @@ static int msm8974_icc_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+ ret = icc_provider_register(provider);
+ if (ret)
+ goto err_remove_nodes;
+
platform_set_drvdata(pdev, qp);
return 0;
-err_del_icc:
+err_remove_nodes:
icc_nodes_remove(provider);
- icc_provider_del(provider);
-
-err_disable_clks:
clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
return ret;
@@ -747,9 +743,9 @@ static int msm8974_icc_remove(struct platform_device *pdev)
{
struct msm8974_icc_provider *qp = platform_get_drvdata(pdev);
+ icc_provider_deregister(&qp->provider);
icc_nodes_remove(&qp->provider);
clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
- icc_provider_del(&qp->provider);
return 0;
}
--
2.39.1
The current interconnect provider registration interface is inherently
racy as nodes are not added until the after adding the provider. This
can specifically cause racing DT lookups to fail.
Switch to using the new API where the provider is not registered until
after it has been fully initialised.
Fixes: e6f0d6a30f73 ("interconnect: qcom: Add SM8550 interconnect provider driver")
Cc: Abel Vesa <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/qcom/sm8550.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/interconnect/qcom/sm8550.c b/drivers/interconnect/qcom/sm8550.c
index 54fa027ab961..7ab492ca8fe0 100644
--- a/drivers/interconnect/qcom/sm8550.c
+++ b/drivers/interconnect/qcom/sm8550.c
@@ -2197,9 +2197,10 @@ static int qnoc_probe(struct platform_device *pdev)
provider->pre_aggregate = qcom_icc_pre_aggregate;
provider->aggregate = qcom_icc_aggregate;
provider->xlate_extended = qcom_icc_xlate_extended;
- INIT_LIST_HEAD(&provider->nodes);
provider->data = data;
+ icc_provider_init(provider);
+
qp->dev = &pdev->dev;
qp->bcms = desc->bcms;
qp->num_bcms = desc->num_bcms;
@@ -2208,12 +2209,6 @@ static int qnoc_probe(struct platform_device *pdev)
if (IS_ERR(qp->voter))
return PTR_ERR(qp->voter);
- ret = icc_provider_add(provider);
- if (ret) {
- dev_err_probe(&pdev->dev, ret,
- "error adding interconnect provider\n");
- return ret;
- }
for (i = 0; i < qp->num_bcms; i++)
qcom_icc_bcm_init(qp->bcms[i], &pdev->dev);
@@ -2227,7 +2222,7 @@ static int qnoc_probe(struct platform_device *pdev)
node = icc_node_create(qnodes[i]->id);
if (IS_ERR(node)) {
ret = PTR_ERR(node);
- goto err;
+ goto err_remove_nodes;
}
node->name = qnodes[i]->name;
@@ -2241,12 +2236,17 @@ static int qnoc_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+ ret = icc_provider_register(provider);
+ if (ret)
+ goto err_remove_nodes;
+
platform_set_drvdata(pdev, qp);
return 0;
-err:
+
+err_remove_nodes:
icc_nodes_remove(provider);
- icc_provider_del(provider);
+
return ret;
}
@@ -2254,8 +2254,8 @@ static int qnoc_remove(struct platform_device *pdev)
{
struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
+ icc_provider_deregister(&qp->provider);
icc_nodes_remove(&qp->provider);
- icc_provider_del(&qp->provider);
return 0;
}
--
2.39.1
On 23-02-01 11:15:49, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
Sounds good to me.
>
> Fixes: e6f0d6a30f73 ("interconnect: qcom: Add SM8550 interconnect provider driver")
> Cc: Abel Vesa <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Abel Vesa <[email protected]>
> ---
> drivers/interconnect/qcom/sm8550.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/sm8550.c b/drivers/interconnect/qcom/sm8550.c
> index 54fa027ab961..7ab492ca8fe0 100644
> --- a/drivers/interconnect/qcom/sm8550.c
> +++ b/drivers/interconnect/qcom/sm8550.c
> @@ -2197,9 +2197,10 @@ static int qnoc_probe(struct platform_device *pdev)
> provider->pre_aggregate = qcom_icc_pre_aggregate;
> provider->aggregate = qcom_icc_aggregate;
> provider->xlate_extended = qcom_icc_xlate_extended;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->data = data;
>
> + icc_provider_init(provider);
> +
> qp->dev = &pdev->dev;
> qp->bcms = desc->bcms;
> qp->num_bcms = desc->num_bcms;
> @@ -2208,12 +2209,6 @@ static int qnoc_probe(struct platform_device *pdev)
> if (IS_ERR(qp->voter))
> return PTR_ERR(qp->voter);
>
> - ret = icc_provider_add(provider);
> - if (ret) {
> - dev_err_probe(&pdev->dev, ret,
> - "error adding interconnect provider\n");
> - return ret;
> - }
>
> for (i = 0; i < qp->num_bcms; i++)
> qcom_icc_bcm_init(qp->bcms[i], &pdev->dev);
> @@ -2227,7 +2222,7 @@ static int qnoc_probe(struct platform_device *pdev)
> node = icc_node_create(qnodes[i]->id);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> - goto err;
> + goto err_remove_nodes;
> }
>
> node->name = qnodes[i]->name;
> @@ -2241,12 +2236,17 @@ static int qnoc_probe(struct platform_device *pdev)
> }
> data->num_nodes = num_nodes;
>
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err_remove_nodes;
> +
> platform_set_drvdata(pdev, qp);
>
> return 0;
> -err:
> +
> +err_remove_nodes:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
> +
> return ret;
> }
>
> @@ -2254,8 +2254,8 @@ static int qnoc_remove(struct platform_device *pdev)
> {
> struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
> --
> 2.39.1
>
On 1.02.2023 11:15, Johan Hovold wrote:
> The interconnect framework currently expects that providers are only
> removed when there are no users and after all nodes have been removed.
>
> There is currently nothing that guarantees this to be the case and the
> framework does not do any reference counting, but refusing to remove the
> provider is never correct as that would leave a dangling pointer to a
> resource that is about to be released in the global provider list (e.g.
> accessible through debugfs).
>
> Replace the current sanity checks with WARN_ON() so that the provider is
> always removed.
I spent a considerable amount of time scratching my head what WARN_ON has
to do with removing list items.. I suppose "don't return early and replace
pr_warn with WARN_ON" would have been clearer, but maybe that's just me
in the morning..
>
> Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
> Cc: [email protected] # 5.1: 680f8666baf6: interconnect: Make icc_provider_del() return void
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/core.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index dc61620a0191..43c5c8503ee8 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1062,18 +1062,8 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
> void icc_provider_del(struct icc_provider *provider)
> {
> mutex_lock(&icc_lock);
> - if (provider->users) {
> - pr_warn("interconnect provider still has %d users\n",
> - provider->users);
> - mutex_unlock(&icc_lock);
> - return;
> - }
> -
> - if (!list_empty(&provider->nodes)) {
> - pr_warn("interconnect provider still has nodes\n");
> - mutex_unlock(&icc_lock);
> - return;
> - }
> + WARN_ON(provider->users);
> + WARN_ON(!list_empty(&provider->nodes));
>
> list_del(&provider->provider_list);
> mutex_unlock(&icc_lock);
On 1.02.2023 11:15, Johan Hovold wrote:
> The node link array is allocated when adding links to a node but is not
> deallocated when nodes are destroyed.
>
> Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
> Cc: [email protected] # 5.1
> Cc: Georgi Djakov <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 423f875d4b54..dc61620a0191 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -850,6 +850,7 @@ void icc_node_destroy(int id)
>
> mutex_unlock(&icc_lock);
>
> + kfree(node->links);
> kfree(node);
> }
> EXPORT_SYMBOL_GPL(icc_node_destroy);
On 01/02/2023 11:15, Johan Hovold wrote:
> Make sure to add the newly allocated interconnect node to the provider
> before adding the PM QoS request so that the node is freed on errors.
>
> Fixes: 2f95b9d5cf0b ("interconnect: Add generic interconnect driver for Exynos SoCs")
> Cc: [email protected] # 5.11
> Cc: Sylwester Nawrocki <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/interconnect/samsung/exynos.c | 6 +++---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to trigger a NULL-pointer
> deference when either a NULL pointer or not fully initialised node is
> returned from exynos_generic_icc_xlate().
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 2f95b9d5cf0b ("interconnect: Add generic interconnect driver for Exynos SoCs")
> Cc: [email protected] # 5.11
> Cc: Sylwester Nawrocki <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/interconnect/samsung/exynos.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/interconnect/samsung/exynos.c b/drivers/interconnect/samsung/exynos.c
> index e70665899482..72e42603823b 100644
> --- a/drivers/interconnect/samsung/exynos.c
> +++ b/drivers/interconnect/samsung/exynos.c
> @@ -98,12 +98,13 @@ static int exynos_generic_icc_remove(struct platform_device *pdev)
> struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
> struct icc_node *parent_node, *node = priv->node;
>
> + icc_provider_deregister(&priv->provider);
> +
> parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
> if (parent_node && !IS_ERR(parent_node))
> icc_link_destroy(node, parent_node);
>
> icc_nodes_remove(&priv->provider);
> - icc_provider_del(&priv->provider);
>
> return 0;
> }
> @@ -132,15 +133,11 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
> provider->inter_set = true;
> provider->data = priv;
>
> - ret = icc_provider_add(provider);
> - if (ret < 0)
> - return ret;
> + icc_provider_init(provider);
>
> icc_node = icc_node_create(pdev->id);
> - if (IS_ERR(icc_node)) {
> - ret = PTR_ERR(icc_node);
> - goto err_prov_del;
> - }
> + if (IS_ERR(icc_node))
> + return PTR_ERR(icc_node);
>
> priv->node = icc_node;
> icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
> @@ -171,14 +168,17 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
> goto err_pmqos_del;
> }
>
> + ret = icc_provider_register(provider);
> + if (ret < 0)
> + goto err_pmqos_del;
If I understand correctly there is no need for icc_link_destroy() in
error path here, right? Even in case of probe retry (defer or whatever
reason) - the link will be removed with icc_nodes_remove()?
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> There is no longer any need to explicitly destroy node links as this is
> now done when the node is destroyed as part of icc_nodes_remove().
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/interconnect/samsung/exynos.c | 6 ------
> 1 file changed, 6 deletions(-)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> The current interconnect provider interface is inherently racy as
> providers are expected to be registered before being fully initialised.
>
> This can specifically cause racing DT lookups to fail as I recently
> noticed when the Qualcomm cpufreq driver failed to probe:
>
> of_icc_xlate_onecell: invalid index 0
> cpu cpu0: error -EINVAL: error finding src node
> cpu cpu0: dev_pm_opp_of_find_icc_paths: Unable to get path0: -22
> qcom-cpufreq-hw: probe of 18591000.cpufreq failed with error -22
>
> This only happens very rarely, but the bug is easily reproduced by
> increasing the race window by adding an msleep() after registering
> osm-l3 interconnect provider.
>
> Note that the Qualcomm cpufreq driver is especially susceptible to this
> race as the interconnect path is looked up from the CPU nodes so that
> driver core does not guarantee the probe order even when device links
> are enabled (which they not always are).
>
> This series adds a new interconnect provider registration API which is
> used to fix up the interconnect drivers before removing the old racy
> API.
>
So is there a dependency or not? Can you make it clear that I shouldn't
take memory controller bits?
Best regards,
Krzysztof
On Thu, Feb 02, 2023 at 12:04:49PM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2023 11:15, Johan Hovold wrote:
> > @@ -98,12 +98,13 @@ static int exynos_generic_icc_remove(struct platform_device *pdev)
> > struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
> > struct icc_node *parent_node, *node = priv->node;
> >
> > + icc_provider_deregister(&priv->provider);
> > +
> > parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
> > if (parent_node && !IS_ERR(parent_node))
> > icc_link_destroy(node, parent_node);
> >
> > icc_nodes_remove(&priv->provider);
> > - icc_provider_del(&priv->provider);
> >
> > return 0;
> > }
> > @@ -132,15 +133,11 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
> > provider->inter_set = true;
> > provider->data = priv;
> >
> > - ret = icc_provider_add(provider);
> > - if (ret < 0)
> > - return ret;
> > + icc_provider_init(provider);
> >
> > icc_node = icc_node_create(pdev->id);
> > - if (IS_ERR(icc_node)) {
> > - ret = PTR_ERR(icc_node);
> > - goto err_prov_del;
> > - }
> > + if (IS_ERR(icc_node))
> > + return PTR_ERR(icc_node);
> >
> > priv->node = icc_node;
> > icc_node->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%pOFn",
> > @@ -171,14 +168,17 @@ static int exynos_generic_icc_probe(struct platform_device *pdev)
> > goto err_pmqos_del;
> > }
> >
> > + ret = icc_provider_register(provider);
> > + if (ret < 0)
> > + goto err_pmqos_del;
>
> If I understand correctly there is no need for icc_link_destroy() in
> error path here, right? Even in case of probe retry (defer or whatever
> reason) - the link will be removed with icc_nodes_remove()?
Correct, it is no longer needed after the first patch in this series.
The exynos driver was the only driver that bothered to remove links
explicitly, all the others expected the interconnect framework to do so
when destroying nodes even if that was not case until now.
Johan
On Thu, Feb 02, 2023 at 12:13:33PM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2023 11:15, Johan Hovold wrote:
> > The current interconnect provider interface is inherently racy as
> > providers are expected to be registered before being fully initialised.
> >
> > This can specifically cause racing DT lookups to fail as I recently
> > noticed when the Qualcomm cpufreq driver failed to probe:
> >
> > of_icc_xlate_onecell: invalid index 0
> > cpu cpu0: error -EINVAL: error finding src node
> > cpu cpu0: dev_pm_opp_of_find_icc_paths: Unable to get path0: -22
> > qcom-cpufreq-hw: probe of 18591000.cpufreq failed with error -22
> >
> > This only happens very rarely, but the bug is easily reproduced by
> > increasing the race window by adding an msleep() after registering
> > osm-l3 interconnect provider.
> >
> > Note that the Qualcomm cpufreq driver is especially susceptible to this
> > race as the interconnect path is looked up from the CPU nodes so that
> > driver core does not guarantee the probe order even when device links
> > are enabled (which they not always are).
> >
> > This series adds a new interconnect provider registration API which is
> > used to fix up the interconnect drivers before removing the old racy
> > API.
> >
>
> So is there a dependency or not? Can you make it clear that I shouldn't
> take memory controller bits?
As the fixes depend on the new API it is best if these could all go
through Georgi's tree.
Johan
On 02/02/2023 13:17, Johan Hovold wrote:
> On Thu, Feb 02, 2023 at 12:04:49PM +0100, Krzysztof Kozlowski wrote:
>> On 01/02/2023 11:15, Johan Hovold wrote:
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 06f079816d4c ("memory: tegra-mc: Add interconnect framework")
> Cc: [email protected] # 5.11
> Cc: Dmitry Osipenko <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
(or tell me if I should take it via memory-controllers)
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 380def2d4cf2 ("memory: tegra124: Support interconnect framework")
> Cc: [email protected] # 5.12
> Cc: Dmitry Osipenko <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: d5ef16ba5fbe ("memory: tegra20: Support interconnect framework")
> Cc: [email protected] # 5.11
> Cc: Dmitry Osipenko <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 01/02/2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: d5ef16ba5fbe ("memory: tegra20: Support interconnect framework")
> Cc: [email protected] # 5.11
> Cc: Dmitry Osipenko <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Wed, Feb 01, 2023 at 11:15:47AM +0100, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 4e60a9568dc6 ("interconnect: qcom: add msm8974 driver")
> Cc: [email protected] # 5.5
> Cc: Brian Masney <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Brian Masney <[email protected]>
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider interface is inherently racy as
> providers are expected to be added before being fully initialised.
>
> Specifically, nodes are currently not added and the provider data is not
> initialised until after registering the provider which can cause racing
> DT lookups to fail.
>
> Add a new provider API which will be used to fix up the interconnect
> drivers.
>
> The old API is reimplemented using the new interface and will be removed
> once all drivers have been fixed.
>
> Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
> Fixes: 87e3031b6fbd ("interconnect: Allow endpoints translation via DT")
> Cc: [email protected] # 5.1
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/core.c | 52 +++++++++++++++++++--------
> include/linux/interconnect-provider.h | 12 +++++++
> 2 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 43c5c8503ee8..93d27ff8eef6 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1030,44 +1030,68 @@ int icc_nodes_remove(struct icc_provider *provider)
> EXPORT_SYMBOL_GPL(icc_nodes_remove);
>
> /**
> - * icc_provider_add() - add a new interconnect provider
> - * @provider: the interconnect provider that will be added into topology
> + * icc_provider_init() - initialize a new interconnect provider
> + * @provider: the interconnect provider to initialize
> + *
> + * Must be called before adding nodes to the provider.
> + */
> +void icc_provider_init(struct icc_provider *provider)
> +{
> + WARN_ON(!provider->set);
> +
> + INIT_LIST_HEAD(&provider->nodes);
> +}
> +EXPORT_SYMBOL_GPL(icc_provider_init);
> +
> +/**
> + * icc_provider_register() - register a new interconnect provider
> + * @provider: the interconnect provider to register
> *
> * Return: 0 on success, or an error code otherwise
> */
> -int icc_provider_add(struct icc_provider *provider)
> +int icc_provider_register(struct icc_provider *provider)
> {
> - if (WARN_ON(!provider->set))
> - return -EINVAL;
> if (WARN_ON(!provider->xlate && !provider->xlate_extended))
> return -EINVAL;
>
> mutex_lock(&icc_lock);
> -
> - INIT_LIST_HEAD(&provider->nodes);
> list_add_tail(&provider->provider_list, &icc_providers);
> -
> mutex_unlock(&icc_lock);
>
> - dev_dbg(provider->dev, "interconnect provider added to topology\n");
> + dev_dbg(provider->dev, "interconnect provider registered\n");
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(icc_provider_add);
> +EXPORT_SYMBOL_GPL(icc_provider_register);
>
> /**
> - * icc_provider_del() - delete previously added interconnect provider
> - * @provider: the interconnect provider that will be removed from topology
> + * icc_provider_deregister() - deregister an interconnect provider
> + * @provider: the interconnect provider to deregister
> */
> -void icc_provider_del(struct icc_provider *provider)
> +void icc_provider_deregister(struct icc_provider *provider)
> {
> mutex_lock(&icc_lock);
> WARN_ON(provider->users);
> - WARN_ON(!list_empty(&provider->nodes));
>
> list_del(&provider->provider_list);
> mutex_unlock(&icc_lock);
> }
> +EXPORT_SYMBOL_GPL(icc_provider_deregister);
> +
> +int icc_provider_add(struct icc_provider *provider)
> +{
> + icc_provider_init(provider);
> +
> + return icc_provider_register(provider);
> +}
> +EXPORT_SYMBOL_GPL(icc_provider_add);
> +
> +void icc_provider_del(struct icc_provider *provider)
> +{
> + WARN_ON(!list_empty(&provider->nodes));
> +
> + icc_provider_deregister(provider);
> +}
> EXPORT_SYMBOL_GPL(icc_provider_del);
>
> static const struct of_device_id __maybe_unused ignore_list[] = {
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index cd5c5a27557f..d12cd18aab3f 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -122,6 +122,9 @@ int icc_link_destroy(struct icc_node *src, struct icc_node *dst);
> void icc_node_add(struct icc_node *node, struct icc_provider *provider);
> void icc_node_del(struct icc_node *node);
> int icc_nodes_remove(struct icc_provider *provider);
> +void icc_provider_init(struct icc_provider *provider);
> +int icc_provider_register(struct icc_provider *provider);
> +void icc_provider_deregister(struct icc_provider *provider);
> int icc_provider_add(struct icc_provider *provider);
> void icc_provider_del(struct icc_provider *provider);
> struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
> @@ -167,6 +170,15 @@ static inline int icc_nodes_remove(struct icc_provider *provider)
> return -ENOTSUPP;
> }
>
> +static inline void icc_provider_init(struct icc_provider *provider) { }
> +
> +static inline int icc_provider_register(struct icc_provider *provider)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline void icc_provider_deregister(struct icc_provider *provider) { }
> +
> static inline int icc_provider_add(struct icc_provider *provider)
> {
> return -ENOTSUPP;
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail:
>
> of_icc_xlate_onecell: invalid index 0
> cpu cpu0: error -EINVAL: error finding src node
> cpu cpu0: dev_pm_opp_of_find_icc_paths: Unable to get path0: -22
> qcom-cpufreq-hw: probe of 18591000.cpufreq failed with error -22
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 5bc9900addaf ("interconnect: qcom: Add OSM L3 interconnect provider support")
> Cc: [email protected] # 5.7
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/osm-l3.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
> index 5fa171087425..3a1cbfe3e481 100644
> --- a/drivers/interconnect/qcom/osm-l3.c
> +++ b/drivers/interconnect/qcom/osm-l3.c
> @@ -158,8 +158,8 @@ static int qcom_osm_l3_remove(struct platform_device *pdev)
> {
> struct qcom_osm_l3_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
> @@ -245,14 +245,9 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> provider->set = qcom_osm_l3_set;
> provider->aggregate = icc_std_aggregate;
> provider->xlate = of_icc_xlate_onecell;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->data = data;
>
> - ret = icc_provider_add(provider);
> - if (ret) {
> - dev_err(&pdev->dev, "error adding interconnect provider\n");
> - return ret;
> - }
> + icc_provider_init(provider);
>
> for (i = 0; i < num_nodes; i++) {
> size_t j;
> @@ -275,12 +270,15 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> }
> data->num_nodes = num_nodes;
>
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err;
> +
> platform_set_drvdata(pdev, qp);
>
> return 0;
> err:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
>
> return ret;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> Make sure to clean up and release resources properly also in case probe
> fails when populating child devices.
>
> Fixes: e39bf2972c6e ("interconnect: icc-rpm: Support child NoC device probe")
> Cc: [email protected] # 5.17
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/icc-rpm.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index df3196f72536..91778cfcbc65 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -541,8 +541,11 @@ int qnoc_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, qp);
>
> /* Populate child NoC devices if any */
> - if (of_get_child_count(dev->of_node) > 0)
> - return of_platform_populate(dev->of_node, NULL, NULL, dev);
> + if (of_get_child_count(dev->of_node) > 0) {
> + ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> + if (ret)
> + goto err;
> + }
>
> return 0;
> err:
On 1.02.2023 11:15, Johan Hovold wrote:
> Make sure to disable clocks also in case attaching the power domain
> fails.
>
> Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
> Cc: [email protected] # 5.17
> Cc: Yassine Oudjana <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/icc-rpm.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 91778cfcbc65..da595059cafd 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -498,8 +498,7 @@ int qnoc_probe(struct platform_device *pdev)
>
> if (desc->has_bus_pd) {
> ret = dev_pm_domain_attach(dev, true);
> - if (ret)
> - return ret;
> + goto err_disable_clks;
> }
>
> provider = &qp->provider;
> @@ -514,8 +513,7 @@ int qnoc_probe(struct platform_device *pdev)
> ret = icc_provider_add(provider);
> if (ret) {
> dev_err(dev, "error adding interconnect provider: %d\n", ret);
> - clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> - return ret;
> + goto err_disable_clks;
> }
>
> for (i = 0; i < num_nodes; i++) {
> @@ -550,8 +548,9 @@ int qnoc_probe(struct platform_device *pdev)
> return 0;
> err:
> icc_nodes_remove(provider);
> - clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> icc_provider_del(provider);
> +err_disable_clks:
> + clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
>
> return ret;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 62feb14ee8a3 ("interconnect: qcom: Consolidate interconnect RPM support")
> Fixes: 30c8fa3ec61a ("interconnect: qcom: Add MSM8916 interconnect provider driver")
> Cc: [email protected] # 5.7
> Cc: Jun Nie <[email protected]>
> Cc: Georgi Djakov <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/icc-rpm.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index da595059cafd..4d0997b210f7 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -502,7 +502,6 @@ int qnoc_probe(struct platform_device *pdev)
> }
>
> provider = &qp->provider;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->dev = dev;
> provider->set = qcom_icc_set;
> provider->pre_aggregate = qcom_icc_pre_bw_aggregate;
> @@ -510,11 +509,7 @@ int qnoc_probe(struct platform_device *pdev)
> provider->xlate_extended = qcom_icc_xlate_extended;
> provider->data = data;
>
> - ret = icc_provider_add(provider);
> - if (ret) {
> - dev_err(dev, "error adding interconnect provider: %d\n", ret);
> - goto err_disable_clks;
> - }
> + icc_provider_init(provider);
>
> for (i = 0; i < num_nodes; i++) {
> size_t j;
> @@ -522,7 +517,7 @@ int qnoc_probe(struct platform_device *pdev)
> node = icc_node_create(qnodes[i]->id);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> - goto err;
> + goto err_remove_nodes;
> }
>
> node->name = qnodes[i]->name;
> @@ -536,19 +531,25 @@ int qnoc_probe(struct platform_device *pdev)
> }
> data->num_nodes = num_nodes;
>
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err_remove_nodes;
> +
> platform_set_drvdata(pdev, qp);
>
> /* Populate child NoC devices if any */
> if (of_get_child_count(dev->of_node) > 0) {
> ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> if (ret)
> - goto err;
> + goto err_deregister_provider;
> }
>
> return 0;
> -err:
> +
> +err_deregister_provider:
> + icc_provider_deregister(provider);
> +err_remove_nodes:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
> err_disable_clks:
> clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
>
> @@ -560,9 +561,9 @@ int qnoc_remove(struct platform_device *pdev)
> {
> struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> Make sure to clean up and release resources properly also in case probe
> fails when populating child devices.
>
> Fixes: 57eb14779dfd ("interconnect: qcom: icc-rpmh: Support child NoC device probe")
> Cc: [email protected] # 6.0
> Cc: Luca Weiss <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/icc-rpmh.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index fd17291c61eb..5168bbf3d92f 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -235,8 +235,11 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, qp);
>
> /* Populate child NoC devices if any */
> - if (of_get_child_count(dev->of_node) > 0)
> - return of_platform_populate(dev->of_node, NULL, NULL, dev);
> + if (of_get_child_count(dev->of_node) > 0) {
> + ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> + if (ret)
> + goto err;
> + }
>
> return 0;
> err:
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 976daac4a1c5 ("interconnect: qcom: Consolidate interconnect RPMh support")
> Cc: [email protected] # 5.7
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/icc-rpmh.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index 5168bbf3d92f..fdb5e58e408b 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -192,9 +192,10 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> provider->pre_aggregate = qcom_icc_pre_aggregate;
> provider->aggregate = qcom_icc_aggregate;
> provider->xlate_extended = qcom_icc_xlate_extended;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->data = data;
>
> + icc_provider_init(provider);
> +
> qp->dev = dev;
> qp->bcms = desc->bcms;
> qp->num_bcms = desc->num_bcms;
> @@ -203,10 +204,6 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> if (IS_ERR(qp->voter))
> return PTR_ERR(qp->voter);
>
> - ret = icc_provider_add(provider);
> - if (ret)
> - return ret;
> -
> for (i = 0; i < qp->num_bcms; i++)
> qcom_icc_bcm_init(qp->bcms[i], dev);
>
> @@ -218,7 +215,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> node = icc_node_create(qn->id);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> - goto err;
> + goto err_remove_nodes;
> }
>
> node->name = qn->name;
> @@ -232,19 +229,27 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> }
>
> data->num_nodes = num_nodes;
> +
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err_remove_nodes;
> +
> platform_set_drvdata(pdev, qp);
>
> /* Populate child NoC devices if any */
> if (of_get_child_count(dev->of_node) > 0) {
> ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> if (ret)
> - goto err;
> + goto err_deregister_provider;
> }
>
> return 0;
> -err:
> +
> +err_deregister_provider:
> + icc_provider_deregister(provider);
> +err_remove_nodes:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(qcom_icc_rpmh_probe);
> @@ -253,8 +258,8 @@ int qcom_icc_rpmh_remove(struct platform_device *pdev)
> {
> struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 4e60a9568dc6 ("interconnect: qcom: add msm8974 driver")
> Cc: [email protected] # 5.5
> Cc: Brian Masney <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/msm8974.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/msm8974.c b/drivers/interconnect/qcom/msm8974.c
> index 5ea192f1141d..1828deaca443 100644
> --- a/drivers/interconnect/qcom/msm8974.c
> +++ b/drivers/interconnect/qcom/msm8974.c
> @@ -692,7 +692,6 @@ static int msm8974_icc_probe(struct platform_device *pdev)
> return ret;
>
> provider = &qp->provider;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->dev = dev;
> provider->set = msm8974_icc_set;
> provider->aggregate = icc_std_aggregate;
> @@ -700,11 +699,7 @@ static int msm8974_icc_probe(struct platform_device *pdev)
> provider->data = data;
> provider->get_bw = msm8974_get_bw;
>
> - ret = icc_provider_add(provider);
> - if (ret) {
> - dev_err(dev, "error adding interconnect provider: %d\n", ret);
> - goto err_disable_clks;
> - }
> + icc_provider_init(provider);
>
> for (i = 0; i < num_nodes; i++) {
> size_t j;
> @@ -712,7 +707,7 @@ static int msm8974_icc_probe(struct platform_device *pdev)
> node = icc_node_create(qnodes[i]->id);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> - goto err_del_icc;
> + goto err_remove_nodes;
> }
>
> node->name = qnodes[i]->name;
> @@ -729,15 +724,16 @@ static int msm8974_icc_probe(struct platform_device *pdev)
> }
> data->num_nodes = num_nodes;
>
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err_remove_nodes;
> +
> platform_set_drvdata(pdev, qp);
>
> return 0;
>
> -err_del_icc:
> +err_remove_nodes:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
> -
> -err_disable_clks:
> clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
>
> return ret;
> @@ -747,9 +743,9 @@ static int msm8974_icc_remove(struct platform_device *pdev)
> {
> struct msm8974_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: fafc114a468e ("interconnect: qcom: Add SM8450 interconnect provider driver")
> Cc: [email protected] # 5.17
> Cc: Vinod Koul <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/sm8450.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/sm8450.c b/drivers/interconnect/qcom/sm8450.c
> index e3a12e3d6e06..c7a8bbf102a3 100644
> --- a/drivers/interconnect/qcom/sm8450.c
> +++ b/drivers/interconnect/qcom/sm8450.c
> @@ -1876,9 +1876,10 @@ static int qnoc_probe(struct platform_device *pdev)
> provider->pre_aggregate = qcom_icc_pre_aggregate;
> provider->aggregate = qcom_icc_aggregate;
> provider->xlate_extended = qcom_icc_xlate_extended;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->data = data;
>
> + icc_provider_init(provider);
> +
> qp->dev = &pdev->dev;
> qp->bcms = desc->bcms;
> qp->num_bcms = desc->num_bcms;
> @@ -1887,12 +1888,6 @@ static int qnoc_probe(struct platform_device *pdev)
> if (IS_ERR(qp->voter))
> return PTR_ERR(qp->voter);
>
> - ret = icc_provider_add(provider);
> - if (ret) {
> - dev_err(&pdev->dev, "error adding interconnect provider\n");
> - return ret;
> - }
> -
> for (i = 0; i < qp->num_bcms; i++)
> qcom_icc_bcm_init(qp->bcms[i], &pdev->dev);
>
> @@ -1905,7 +1900,7 @@ static int qnoc_probe(struct platform_device *pdev)
> node = icc_node_create(qnodes[i]->id);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> - goto err;
> + goto err_remove_nodes;
> }
>
> node->name = qnodes[i]->name;
> @@ -1919,12 +1914,17 @@ static int qnoc_probe(struct platform_device *pdev)
> }
> data->num_nodes = num_nodes;
>
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err_remove_nodes;
> +
> platform_set_drvdata(pdev, qp);
>
> return 0;
> -err:
> +
> +err_remove_nodes:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
> +
> return ret;
> }
>
> @@ -1932,8 +1932,8 @@ static int qnoc_remove(struct platform_device *pdev)
> {
> struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: e6f0d6a30f73 ("interconnect: qcom: Add SM8550 interconnect provider driver")
> Cc: Abel Vesa <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/qcom/sm8550.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/sm8550.c b/drivers/interconnect/qcom/sm8550.c
> index 54fa027ab961..7ab492ca8fe0 100644
> --- a/drivers/interconnect/qcom/sm8550.c
> +++ b/drivers/interconnect/qcom/sm8550.c
> @@ -2197,9 +2197,10 @@ static int qnoc_probe(struct platform_device *pdev)
> provider->pre_aggregate = qcom_icc_pre_aggregate;
> provider->aggregate = qcom_icc_aggregate;
> provider->xlate_extended = qcom_icc_xlate_extended;
> - INIT_LIST_HEAD(&provider->nodes);
> provider->data = data;
>
> + icc_provider_init(provider);
> +
> qp->dev = &pdev->dev;
> qp->bcms = desc->bcms;
> qp->num_bcms = desc->num_bcms;
> @@ -2208,12 +2209,6 @@ static int qnoc_probe(struct platform_device *pdev)
> if (IS_ERR(qp->voter))
> return PTR_ERR(qp->voter);
>
> - ret = icc_provider_add(provider);
> - if (ret) {
> - dev_err_probe(&pdev->dev, ret,
> - "error adding interconnect provider\n");
> - return ret;
> - }
>
> for (i = 0; i < qp->num_bcms; i++)
> qcom_icc_bcm_init(qp->bcms[i], &pdev->dev);
> @@ -2227,7 +2222,7 @@ static int qnoc_probe(struct platform_device *pdev)
> node = icc_node_create(qnodes[i]->id);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> - goto err;
> + goto err_remove_nodes;
> }
>
> node->name = qnodes[i]->name;
> @@ -2241,12 +2236,17 @@ static int qnoc_probe(struct platform_device *pdev)
> }
> data->num_nodes = num_nodes;
>
> + ret = icc_provider_register(provider);
> + if (ret)
> + goto err_remove_nodes;
> +
> platform_set_drvdata(pdev, qp);
>
> return 0;
> -err:
> +
> +err_remove_nodes:
> icc_nodes_remove(provider);
> - icc_provider_del(provider);
> +
> return ret;
> }
>
> @@ -2254,8 +2254,8 @@ static int qnoc_remove(struct platform_device *pdev)
> {
> struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
>
> + icc_provider_deregister(&qp->provider);
> icc_nodes_remove(&qp->provider);
> - icc_provider_del(&qp->provider);
>
> return 0;
> }
On 1.02.2023 11:15, Johan Hovold wrote:
> Now that all interconnect drivers have been converted to the new
> provider registration API, the old racy interface can be removed.
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/core.c | 16 ----------------
> include/linux/interconnect-provider.h | 11 -----------
> 2 files changed, 27 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 93d27ff8eef6..b8917823fd95 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1078,22 +1078,6 @@ void icc_provider_deregister(struct icc_provider *provider)
> }
> EXPORT_SYMBOL_GPL(icc_provider_deregister);
>
> -int icc_provider_add(struct icc_provider *provider)
> -{
> - icc_provider_init(provider);
> -
> - return icc_provider_register(provider);
> -}
> -EXPORT_SYMBOL_GPL(icc_provider_add);
> -
> -void icc_provider_del(struct icc_provider *provider)
> -{
> - WARN_ON(!list_empty(&provider->nodes));
> -
> - icc_provider_deregister(provider);
> -}
> -EXPORT_SYMBOL_GPL(icc_provider_del);
> -
> static const struct of_device_id __maybe_unused ignore_list[] = {
> { .compatible = "qcom,sc7180-ipa-virt" },
> { .compatible = "qcom,sc8180x-ipa-virt" },
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index d12cd18aab3f..b9af9016a95e 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -125,8 +125,6 @@ int icc_nodes_remove(struct icc_provider *provider);
> void icc_provider_init(struct icc_provider *provider);
> int icc_provider_register(struct icc_provider *provider);
> void icc_provider_deregister(struct icc_provider *provider);
> -int icc_provider_add(struct icc_provider *provider);
> -void icc_provider_del(struct icc_provider *provider);
> struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec);
> void icc_sync_state(struct device *dev);
>
> @@ -179,15 +177,6 @@ static inline int icc_provider_register(struct icc_provider *provider)
>
> static inline void icc_provider_deregister(struct icc_provider *provider) { }
>
> -static inline int icc_provider_add(struct icc_provider *provider)
> -{
> - return -ENOTSUPP;
> -}
> -
> -static inline void icc_provider_del(struct icc_provider *provider)
> -{
> -}
> -
> static inline struct icc_node_data *of_icc_get_from_provider(struct of_phandle_args *spec)
> {
> return ERR_PTR(-ENOTSUPP);
On 1.02.2023 11:15, Johan Hovold wrote:
> The icc_get() interface can be used to lookup an interconnect path based
> on global node ids. There has never been any users of this interface and
> all lookups are currently done from the devicetree.
>
> Remove the unused icc_get() interface.
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/interconnect/core.c | 52 ++----------------------------------
> include/linux/interconnect.h | 8 ------
> 2 files changed, 2 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index b8917823fd95..8cca5e9a9d5f 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -588,7 +588,7 @@ EXPORT_SYMBOL_GPL(icc_set_tag);
>
> /**
> * icc_get_name() - Get name of the icc path
> - * @path: reference to the path returned by icc_get()
> + * @path: interconnect path
> *
> * This function is used by an interconnect consumer to get the name of the icc
> * path.
> @@ -606,7 +606,7 @@ EXPORT_SYMBOL_GPL(icc_get_name);
>
> /**
> * icc_set_bw() - set bandwidth constraints on an interconnect path
> - * @path: reference to the path returned by icc_get()
> + * @path: interconnect path
> * @avg_bw: average bandwidth in kilobytes per second
> * @peak_bw: peak bandwidth in kilobytes per second
> *
> @@ -705,54 +705,6 @@ int icc_disable(struct icc_path *path)
> }
> EXPORT_SYMBOL_GPL(icc_disable);
>
> -/**
> - * icc_get() - return a handle for path between two endpoints
> - * @dev: the device requesting the path
> - * @src_id: source device port id
> - * @dst_id: destination device port id
> - *
> - * This function will search for a path between two endpoints and return an
> - * icc_path handle on success. Use icc_put() to release
> - * constraints when they are not needed anymore.
> - * If the interconnect API is disabled, NULL is returned and the consumer
> - * drivers will still build. Drivers are free to handle this specifically,
> - * but they don't have to.
> - *
> - * Return: icc_path pointer on success, ERR_PTR() on error or NULL if the
> - * interconnect API is disabled.
> - */
> -struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> -{
> - struct icc_node *src, *dst;
> - struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> -
> - mutex_lock(&icc_lock);
> -
> - src = node_find(src_id);
> - if (!src)
> - goto out;
> -
> - dst = node_find(dst_id);
> - if (!dst)
> - goto out;
> -
> - path = path_find(dev, src, dst);
> - if (IS_ERR(path)) {
> - dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> - goto out;
> - }
> -
> - path->name = kasprintf(GFP_KERNEL, "%s-%s", src->name, dst->name);
> - if (!path->name) {
> - kfree(path);
> - path = ERR_PTR(-ENOMEM);
> - }
> -out:
> - mutex_unlock(&icc_lock);
> - return path;
> -}
> -EXPORT_SYMBOL_GPL(icc_get);
> -
> /**
> * icc_put() - release the reference to the icc_path
> * @path: interconnect path
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index 2b0e784ba771..97ac253df62c 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -40,8 +40,6 @@ struct icc_bulk_data {
>
> #if IS_ENABLED(CONFIG_INTERCONNECT)
>
> -struct icc_path *icc_get(struct device *dev, const int src_id,
> - const int dst_id);
> struct icc_path *of_icc_get(struct device *dev, const char *name);
> struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
> int devm_of_icc_bulk_get(struct device *dev, int num_paths, struct icc_bulk_data *paths);
> @@ -61,12 +59,6 @@ void icc_bulk_disable(int num_paths, const struct icc_bulk_data *paths);
>
> #else
>
> -static inline struct icc_path *icc_get(struct device *dev, const int src_id,
> - const int dst_id)
> -{
> - return NULL;
> -}
> -
> static inline struct icc_path *of_icc_get(struct device *dev,
> const char *name)
> {
Johan Hovold <[email protected]> 于2023年2月1日周三 18:16写道：
>
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
>
> Fixes: 62feb14ee8a3 ("interconnect: qcom: Consolidate interconnect RPM support")
> Fixes: 30c8fa3ec61a ("interconnect: qcom: Add MSM8916 interconnect provider driver")
> Cc: [email protected] # 5.7
> Cc: Jun Nie <[email protected]>
> Cc: Georgi Djakov <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/interconnect/qcom/icc-rpm.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
Reviewed-by: Jun Nie <[email protected]>
On 01-02-23, 11:15, Johan Hovold wrote:
> The current interconnect provider registration interface is inherently
> racy as nodes are not added until the after adding the provider. This
> can specifically cause racing DT lookups to fail.
>
> Switch to using the new API where the provider is not registered until
> after it has been fully initialised.
Reviewed-by: Vinod Koul <[email protected]>
--
~Vinod
Le 01/02/2023 à 11:15, Johan Hovold a écrit :
> Make sure to disable clocks also in case attaching the power domain
> fails.
>
> Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
> Cc: [email protected] # 5.17
> Cc: Yassine Oudjana <y.oudjana-g/[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/interconnect/qcom/icc-rpm.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 91778cfcbc65..da595059cafd 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -498,8 +498,7 @@ int qnoc_probe(struct platform_device *pdev)
>
> if (desc->has_bus_pd) {
> ret = dev_pm_domain_attach(dev, true);
> - if (ret)
> - return ret;
> + goto err_disable_clks;
Hi,
this change looks strange because we now skip the rest of the function.
Is it really intended?
Also, should dev_pm_domain_detach() be called somewhere in the error
handling path and remove function ?
CJ
> }
>
> provider = &qp->provider;
> @@ -514,8 +513,7 @@ int qnoc_probe(struct platform_device *pdev)
> ret = icc_provider_add(provider);
> if (ret) {
> dev_err(dev, "error adding interconnect provider: %d\n", ret);
> - clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> - return ret;
> + goto err_disable_clks;
> }
>
> for (i = 0; i < num_nodes; i++) {
> @@ -550,8 +548,9 @@ int qnoc_probe(struct platform_device *pdev)
> return 0;
> err:
> icc_nodes_remove(provider);
> - clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> icc_provider_del(provider);
> +err_disable_clks:
> + clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
>
> return ret;
> }
On Sat, Mar 11, 2023 at 07:17:50PM +0100, Christophe JAILLET wrote:
> Le 01/02/2023 à 11:15, Johan Hovold a écrit :
> > Make sure to disable clocks also in case attaching the power domain
> > fails.
> >
> > Fixes: 7de109c0abe9 ("interconnect: icc-rpm: Add support for bus power domain")
> > Cc: [email protected] # 5.17
> > Cc: Yassine Oudjana <y.oudjana-g/[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > drivers/interconnect/qcom/icc-rpm.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> > index 91778cfcbc65..da595059cafd 100644
> > --- a/drivers/interconnect/qcom/icc-rpm.c
> > +++ b/drivers/interconnect/qcom/icc-rpm.c
> > @@ -498,8 +498,7 @@ int qnoc_probe(struct platform_device *pdev)
> >
> > if (desc->has_bus_pd) {
> > ret = dev_pm_domain_attach(dev, true);
> > - if (ret)
> > - return ret;
> > + goto err_disable_clks;
>
> Hi,
> this change looks strange because we now skip the rest of the function.
>
> Is it really intended?
No, this was definitely not intentional. Thanks for catching this. I'll
send a follow up fix for Georgi to fold in or apply on top.
> Also, should dev_pm_domain_detach() be called somewhere in the error
> handling path and remove function ?
In principle, yes. (I think read the above as being another device
managed resource.)
It turns out, however, that this code is totally bogus as any power
domain would already have been attached by the platform bus code and the
above call would always just succeed. The platform code would also
handle detach on errors.
I'll send a patch to remove this.
Johan