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
Changes in v2
- icc_node_destroy() can be called with an arbitrary node id so add the
missing sanity check to handle potential attempts to destroy a
non-existing node (patch 01/23). Reported by Dan Carpenter and the
kernel test robot:
https://lore.kernel.org/oe-kbuild/[email protected]/
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 | 152 +++++---------------------
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, 157 insertions(+), 265 deletions(-)
--
2.39.2
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")
Reviewed-by: Abel Vesa <[email protected]>
Reviewed-by: Konrad Dybcio <[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.2
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
Reviewed-by: Konrad Dybcio <[email protected]>
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.2
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
Reviewed-by: Konrad Dybcio <[email protected]>
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.2
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
Reviewed-by: Konrad Dybcio <[email protected]>
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 5217f449eeec..cabb6f5df83e 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1065,18 +1065,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.2
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]>
Acked-by: Krzysztof Kozlowski <[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.2
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: f0d8048525d7 ("interconnect: Add imx core driver")
Cc: [email protected] # 5.8
Cc: Alexandre Bailon <[email protected]>
Reviewed-by: Konrad Dybcio <[email protected]>
Tested-by: Luca Ceresoli <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/imx/imx.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
index 823d9be9771a..979ed610f704 100644
--- a/drivers/interconnect/imx/imx.c
+++ b/drivers/interconnect/imx/imx.c
@@ -295,6 +295,9 @@ int imx_icc_register(struct platform_device *pdev,
provider->xlate = of_icc_xlate_onecell;
provider->data = data;
provider->dev = dev->parent;
+
+ icc_provider_init(provider);
+
platform_set_drvdata(pdev, imx_provider);
if (settings) {
@@ -306,20 +309,18 @@ int imx_icc_register(struct platform_device *pdev,
}
}
- ret = icc_provider_add(provider);
- if (ret) {
- dev_err(dev, "error adding interconnect provider: %d\n", ret);
+ ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count, settings);
+ if (ret)
return ret;
- }
- ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count, settings);
+ ret = icc_provider_register(provider);
if (ret)
- goto provider_del;
+ goto err_unregister_nodes;
return 0;
-provider_del:
- icc_provider_del(provider);
+err_unregister_nodes:
+ imx_icc_unregister_nodes(&imx_provider->provider);
return ret;
}
EXPORT_SYMBOL_GPL(imx_icc_register);
@@ -328,9 +329,8 @@ void imx_icc_unregister(struct platform_device *pdev)
{
struct imx_icc_provider *imx_provider = platform_get_drvdata(pdev);
+ icc_provider_deregister(&imx_provider->provider);
imx_icc_unregister_nodes(&imx_provider->provider);
-
- icc_provider_del(&imx_provider->provider);
}
EXPORT_SYMBOL_GPL(imx_icc_unregister);
--
2.39.2
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]>
Acked-by: Krzysztof Kozlowski <[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.2
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.
Reviewed-by: Konrad Dybcio <[email protected]>
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 ebefd263ac4b..fd12f109c05e 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.2
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]>
Acked-by: Krzysztof Kozlowski <[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.2
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]>
Reviewed-by: Krzysztof Kozlowski <[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.2
Now that all interconnect drivers have been converted to the new
provider registration API, the old racy interface can be removed.
Reviewed-by: Konrad Dybcio <[email protected]>
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 7a24c1444ace..ebefd263ac4b 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1081,22 +1081,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.2
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]>
Reviewed-by: Krzysztof Kozlowski <[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.2
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 fd12f109c05e..6315d4256ac1 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -862,52 +862,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.2
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]>
Acked-by: Krzysztof Kozlowski <[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.2
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
Reviewed-by: Konrad Dybcio <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/interconnect/core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 0f392f59b135..5217f449eeec 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -850,6 +850,10 @@ void icc_node_destroy(int id)
mutex_unlock(&icc_lock);
+ if (!node)
+ return;
+
+ kfree(node->links);
kfree(node);
}
EXPORT_SYMBOL_GPL(icc_node_destroy);
--
2.39.2
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]>
Reviewed-by: Konrad Dybcio <[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.2
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
Reviewed-by: Konrad Dybcio <[email protected]>
Reviewed-by: 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.2
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().
Reviewed-by: Krzysztof Kozlowski <[email protected]>
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.2
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
Reviewed-by: Brian Masney <[email protected]>
Reviewed-by: Konrad Dybcio <[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.2
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
Reviewed-by: Konrad Dybcio <[email protected]>
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 cabb6f5df83e..7a24c1444ace 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1033,44 +1033,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.2
On 23-03-06 08:56:41, 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")
> Reviewed-by: Abel Vesa <[email protected]>
> Reviewed-by: Konrad Dybcio <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Any changes since v1 or is it just a resend?
> 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.2
>
On Mon, Mar 06, 2023 at 11:03:14AM +0200, Abel Vesa wrote:
> On 23-03-06 08:56:41, 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")
> > Reviewed-by: Abel Vesa <[email protected]>
> > Reviewed-by: Konrad Dybcio <[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
>
> Any changes since v1 or is it just a resend?
Please see the cover letter:
https://lore.kernel.org/lkml/[email protected]/
Only the first patch of the series was updated in v2.
Johan
On 23-03-06 10:34:34, Johan Hovold wrote:
> On Mon, Mar 06, 2023 at 11:03:14AM +0200, Abel Vesa wrote:
> > On 23-03-06 08:56:41, 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")
> > > Reviewed-by: Abel Vesa <[email protected]>
> > > Reviewed-by: Konrad Dybcio <[email protected]>
> > > Signed-off-by: Johan Hovold <[email protected]>
> > > ---
> >
> > Any changes since v1 or is it just a resend?
>
> Please see the cover letter:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Only the first patch of the series was updated in v2.
Right, my bad. Though I wasn't CC'ed on that as well.
>
> Johan
On Mon, Mar 06, 2023 at 11:39:33AM +0200, Abel Vesa wrote:
> On 23-03-06 10:34:34, Johan Hovold wrote:
> > On Mon, Mar 06, 2023 at 11:03:14AM +0200, Abel Vesa wrote:
> > > On 23-03-06 08:56:41, 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")
> > > > Reviewed-by: Abel Vesa <[email protected]>
> > > > Reviewed-by: Konrad Dybcio <[email protected]>
> > > > Signed-off-by: Johan Hovold <[email protected]>
> > > > ---
> > >
> > > Any changes since v1 or is it just a resend?
> >
> > Please see the cover letter:
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > Only the first patch of the series was updated in v2.
>
> Right, my bad. Though I wasn't CC'ed on that as well.
Yeah, sorry about that. I copied the CC list from v1 and forgot to make
sure that all reviewers were copied on the cover letter.
Johan
Hello Johan,
thanks for respinning.
On Mon, 6 Mar 2023 08:56:29 +0100
Johan Hovold <[email protected]> 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
> Reviewed-by: Konrad Dybcio <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
[Tested on i.MX8MP using an MSC SM2-MB-EP1 Board]
Tested-by: Luca Ceresoli <[email protected]>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, 6 Mar 2023 08:56:30 +0100
Johan Hovold <[email protected]> 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.
>
> Fixes: 11f1ceca7031 ("interconnect: Add generic on-chip interconnect API")
> Cc: [email protected] # 5.1: 680f8666baf6: interconnect: Make icc_provider_del() return void
> Reviewed-by: Konrad Dybcio <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
[Tested on i.MX8MP using an MSC SM2-MB-EP1 Board]
Tested-by: Luca Ceresoli <[email protected]>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, 6 Mar 2023 08:56:31 +0100
Johan Hovold <[email protected]> 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
> Reviewed-by: Konrad Dybcio <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
[Tested on i.MX8MP using an MSC SM2-MB-EP1 Board]
Tested-by: Luca Ceresoli <[email protected]>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, 6 Mar 2023 08:56:32 +0100
Johan Hovold <[email protected]> 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: f0d8048525d7 ("interconnect: Add imx core driver")
> Cc: [email protected] # 5.8
> Cc: Alexandre Bailon <[email protected]>
> Reviewed-by: Konrad Dybcio <[email protected]>
> Tested-by: Luca Ceresoli <[email protected]>
v2 works just as well, so my Tested-by is confirmed. Maybe it's useful
mentioning the hardware used for testing so:
[Tested on i.MX8MP using an MSC SM2-MB-EP1 Board]
Tested-by: Luca Ceresoli <[email protected]>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Tue, Mar 07, 2023 at 10:43:24AM +0100, Luca Ceresoli wrote:
> On Mon, 6 Mar 2023 08:56:32 +0100
> Johan Hovold <[email protected]> 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: f0d8048525d7 ("interconnect: Add imx core driver")
> > Cc: [email protected] # 5.8
> > Cc: Alexandre Bailon <[email protected]>
> > Reviewed-by: Konrad Dybcio <[email protected]>
> > Tested-by: Luca Ceresoli <[email protected]>
>
> v2 works just as well, so my Tested-by is confirmed. Maybe it's useful
> mentioning the hardware used for testing so:
>
> [Tested on i.MX8MP using an MSC SM2-MB-EP1 Board]
> Tested-by: Luca Ceresoli <[email protected]>
Thanks for reconfirming. Looks like Georgi has picked these up for
6.3-rc now.
Johan