* [PATCH v4 1/8] clk: renesas: r8a78000: Add clk_hw to clk_map
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 15:55 ` [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support Geert Uytterhoeven
` (6 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
Prepare for mapping to other than fixed-rate clocks by adding a clk_hw
pointer to the clk_map structure, and populating it up-front.
Move r8a78000_cpg_priv.fixed_hws into fill_clk_map(), as it is no longer
needed afterwards, and drop NUM_FIXED_CLKS.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- No changes,
v3:
- New.
---
drivers/clk/renesas/r8a78000-cpg.c | 78 +++++++++++++++++++++---------
1 file changed, 56 insertions(+), 22 deletions(-)
diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c
index b8024e76743ea53d..5ffd7b89cb48e41c 100644
--- a/drivers/clk/renesas/r8a78000-cpg.c
+++ b/drivers/clk/renesas/r8a78000-cpg.c
@@ -16,19 +16,24 @@
#include <dt-bindings/clock/renesas,r8a78000-cpg.h>
+struct clk_map_in {
+ int dt_id; /* DT binding clock ID or -1 sentinel */
+ u32 fw_id; /* FIXED_CLK() ID */
+};
+
struct clk_map {
int dt_id; /* DT binding clock ID or -1 sentinel */
u32 fw_id; /* FIXED_CLK() ID */
+ struct clk_hw *hw;
};
enum fixed_clk {
FIXED_CLK_66M,
FIXED_CLK_133M,
FIXED_CLK_266M,
- NUM_FIXED_CLKS
};
-static const unsigned long fixed_clk_rates[NUM_FIXED_CLKS] = {
+static const unsigned long fixed_clk_rates[] = {
[FIXED_CLK_66M] = 66666000,
[FIXED_CLK_133M] = 133330000,
[FIXED_CLK_266M] = 266660000,
@@ -41,12 +46,10 @@ static const unsigned long fixed_clk_rates[NUM_FIXED_CLKS] = {
*
* @dev: CPG device
* @map: Mapping from DT clock IDs to fixed-rate clocks
- * @fixed_hws: Fixed rate clocks
*/
struct r8a78000_cpg_priv {
struct device *dev;
const struct clk_map *map;
- struct clk_hw *fixed_hws[NUM_FIXED_CLKS];
};
static const struct clk_map *clk_map_find(const struct clk_map *map, u32 id)
@@ -83,8 +86,7 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec,
}
dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id, map->fw_id);
-
- hw = priv->fixed_hws[map->fw_id];
+ hw = map->hw;
dev_dbg(dev, "clock %u is %s at %lu Hz\n", id, clk_hw_get_name(hw),
clk_hw_get_rate(hw));
@@ -92,35 +94,69 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec,
return hw;
}
-static int register_fixed_clks(struct r8a78000_cpg_priv *priv)
+static struct clk_hw **register_fixed_clks(struct device *dev)
{
- struct device *dev = priv->dev;
+ unsigned int n = ARRAY_SIZE(fixed_clk_rates);
+ struct clk_hw **hws, *hw;
unsigned long rate;
- struct clk_hw *hw;
const char *name;
- for (unsigned int i = 0; i < ARRAY_SIZE(fixed_clk_rates); i++) {
+ hws = devm_kcalloc(dev, n, sizeof(*hws), GFP_KERNEL);
+ if (!hws)
+ return ERR_PTR(-ENOMEM);
+
+ for (unsigned int i = 0; i < n; i++) {
rate = fixed_clk_rates[i];
name = devm_kasprintf(dev, GFP_KERNEL, "cpg-%lu", rate);
if (!name)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
hw = devm_clk_hw_register_fixed_rate(dev, name, NULL, 0, rate);
if (IS_ERR(hw))
- return PTR_ERR(hw);
+ return ERR_CAST(hw);
- priv->fixed_hws[i] = hw;
+ hws[i] = hw;
}
- return 0;
+ return hws;
+}
+
+static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
+ const struct clk_map_in *map_in)
+{
+ struct device *dev = priv->dev;
+ struct clk_hw **fixed_hws;
+ struct clk_map *map;
+ unsigned int i;
+
+ fixed_hws = register_fixed_clks(dev);
+ if (IS_ERR(fixed_hws))
+ return ERR_CAST(fixed_hws);
+
+ for (i = 0; map_in[i].dt_id >= 0; i++) { }
+
+ map = devm_kcalloc(dev, i + 1, sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; ; i++) {
+ map[i].dt_id = map_in[i].dt_id;
+ if (map[i].dt_id < 0)
+ break;
+
+ map[i].fw_id = map_in[i].fw_id;
+ map[i].hw = fixed_hws[map[i].fw_id];
+ }
+
+ devm_kfree(dev, fixed_hws);
+ return map;
}
static int r8a78000_cpg_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct r8a78000_cpg_priv *priv;
- const struct clk_map *map;
- int ret;
+ const struct clk_map_in *map;
map = of_device_get_match_data(dev);
if (!map)
@@ -131,16 +167,14 @@ static int r8a78000_cpg_probe(struct platform_device *pdev)
return -ENOMEM;
priv->dev = dev;
- priv->map = map;
-
- ret = register_fixed_clks(priv);
- if (ret)
- return ret;
+ priv->map = fill_clk_map(priv, map);
+ if (IS_ERR(priv->map))
+ return PTR_ERR(priv->map);
return devm_of_clk_add_hw_provider(dev, r8a78000_clk_get, priv);
}
-static const struct clk_map r8a78000_cpg_default[] = {
+static const struct clk_map_in r8a78000_cpg_default[] = {
{ R8A78000_CPG_SGASYNCD4_PERW_BUS, FIXED_CLK(266M) },
{ R8A78000_CPG_SGASYNCD8_PERW_BUS, FIXED_CLK(133M) },
{ R8A78000_CPG_SGASYNCD16_PERW_BUS, FIXED_CLK(66M) },
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
2026-10-16 15:55 ` [PATCH v4 1/8] clk: renesas: r8a78000: Add clk_hw to clk_map Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 16:13 ` sashiko-bot
2026-10-16 15:55 ` [PATCH v4 3/8] reset: Extract __reset_control_get_from_provider() Geert Uytterhoeven
` (5 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
Extend the Clock Pulse Generator driver for R-Car X5H with SCMI support,
to remap DT clock IDs to abstractions (clocks) offered by the SCMI
firmware when running on a system with SCMI, and SCMI support is
enabled. Take into account some peculiarities of the current SCMI
firmware implementations, and fall-back to fixed-rate clocks as
replacements for SCMI clocks that are not available.
Support for bus clocks used by serial, I2C, and SPI devices is included.
This lets us avoid putting SCMI IDs in DT, and allows us to use the same
hardware description in SoC DTS for systems with and without SCMI
firmware.
For now, it is assumed SCMI clock IDs are stable, and match SCP FW
SDKv4.31.0 and later.
Suggested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- Drop unneeded IS_ERR() check after __clk_get_hw(),
- Mark scmi_clk_np __free(device_node),
- Assume all SCMI clock IDs are stable, and drop support for handling
different ID spaces,
- Include initial mapping table for SCP FW SDKv4.31.0 and later,
- Try SCMI clocks first, and fall back to the default fixed-frequency
clocks when not available, as recent firmware versions no longer
restrict access to some bus clocks,
- Drop renesas,scmi-firmware support, just find an arm,scmi node
instead,
- Fall back to the non-SCMI defaults when SCMI clock support is
disabled,
v3:
- Rebase,
- Consistently use /* sentinel */ for zero sentinels,
- s/firmware/renesas,scmi-firmware/,
- Reword,
v2:
- Spin off from "clk: renesas: Add R-Car X5H CPG SCMI remapping
driver",
- Use reverse xmas ordering in r8a78000_cpg_probe(),
- Propagate error codes from scmi_get_base_info() now it can return
-EPROBE_DEFER,
- Improve error message on scmi_get_base_info() failure,
- Check if SCMI firmware node is available,
- Replace the scmi_clk_np member of struct r8a78000_cpg_priv by a
local variable in r8a78000_cpg_probe(), and pass it to
fill_clk_map(),
- Join split short lines,
- Fall back to the default support when SCMI support or firmware is
not available, or when the detected SCMI firmware version is not
supported by the driver,
- Drop mapping tables.
---
drivers/clk/renesas/r8a78000-cpg.c | 149 ++++++++++++++++++++++++++---
1 file changed, 138 insertions(+), 11 deletions(-)
diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c
index 5ffd7b89cb48e41c..758cfe78ae58e0fb 100644
--- a/drivers/clk/renesas/r8a78000-cpg.c
+++ b/drivers/clk/renesas/r8a78000-cpg.c
@@ -5,6 +5,7 @@
* Copyright (C) 2026 Glider bv
*/
+#include <linux/cleanup.h>
#include <linux/clk.h>
#include <linux/clk-provider.h>
#include <linux/dev_printk.h>
@@ -12,21 +13,27 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/scmi_protocol.h>
#include <linux/slab.h>
#include <dt-bindings/clock/renesas,r8a78000-cpg.h>
struct clk_map_in {
int dt_id; /* DT binding clock ID or -1 sentinel */
- u32 fw_id; /* FIXED_CLK() ID */
+ u32 fw_id; /* SCMI firmware clock ID or FIXED_CLK() ID */
};
struct clk_map {
int dt_id; /* DT binding clock ID or -1 sentinel */
- u32 fw_id; /* FIXED_CLK() ID */
+ u32 fw_id; /* SCMI firmware clock ID or FIXED_CLK() ID */
struct clk_hw *hw;
};
+struct cpg_data {
+ const struct clk_map_in *default_map;
+ const struct clk_map_in *fw_map;
+};
+
enum fixed_clk {
FIXED_CLK_66M,
FIXED_CLK_133M,
@@ -39,13 +46,14 @@ static const unsigned long fixed_clk_rates[] = {
[FIXED_CLK_266M] = 266660000,
};
-#define FIXED_CLK(rate) FIXED_CLK_ ## rate
+#define FIXED_CLK_OFFSET 0x80000000
+#define FIXED_CLK(rate) FIXED_CLK_OFFSET + FIXED_CLK_ ## rate
/**
* struct r8a78000_cpg_priv - Clock Pulse Generator Private Data
*
* @dev: CPG device
- * @map: Mapping from DT clock IDs to fixed-rate clocks
+ * @map: Mapping from DT clock IDs to SCMI or fixed-rate clocks
*/
struct r8a78000_cpg_priv {
struct device *dev;
@@ -85,8 +93,19 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec,
return ERR_PTR(-ENOENT);
}
- dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id, map->fw_id);
+ if (map->fw_id < FIXED_CLK_OFFSET)
+ dev_dbg(dev, "Mapping DT clock %u to SCMI clock %u\n", id,
+ map->fw_id);
+ else
+ dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id,
+ map->fw_id - FIXED_CLK_OFFSET);
+
hw = map->hw;
+ if (!hw) {
+ /* CLOCK_ATTRIBUTES is not supported */
+ dev_err(dev, "Clock %u is not available\n", id);
+ return ERR_PTR(-ENOENT);
+ }
dev_dbg(dev, "clock %u is %s at %lu Hz\n", id, clk_hw_get_name(hw),
clk_hw_get_rate(hw));
@@ -94,6 +113,21 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec,
return hw;
}
+static struct device_node *scmi_find_proto(struct device_node *scmi, u32 proto)
+{
+ for_each_available_child_of_node_scoped(scmi, child) {
+ u32 reg;
+
+ if (of_property_read_u32(child, "reg", ®))
+ continue;
+
+ if (reg == proto)
+ return_ptr(child);
+ }
+
+ return NULL;
+}
+
static struct clk_hw **register_fixed_clks(struct device *dev)
{
unsigned int n = ARRAY_SIZE(fixed_clk_rates);
@@ -122,11 +156,16 @@ static struct clk_hw **register_fixed_clks(struct device *dev)
}
static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
- const struct clk_map_in *map_in)
+ const struct clk_map_in *map_in,
+ const struct clk_map_in *fallback,
+ struct device_node *scmi_clk_np)
{
+ struct of_phandle_args scmi_spec;
struct device *dev = priv->dev;
struct clk_hw **fixed_hws;
struct clk_map *map;
+ struct clk_hw *hw;
+ struct clk *clk;
unsigned int i;
fixed_hws = register_fixed_clks(dev);
@@ -145,7 +184,46 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
break;
map[i].fw_id = map_in[i].fw_id;
- map[i].hw = fixed_hws[map[i].fw_id];
+ if (map[i].fw_id >= FIXED_CLK_OFFSET) {
+ enum fixed_clk idx = map[i].fw_id - FIXED_CLK_OFFSET;
+
+ map[i].hw = fixed_hws[idx];
+ continue;
+ }
+
+ scmi_spec.np = scmi_clk_np;
+ scmi_spec.args_count = 1;
+ scmi_spec.args[0] = map[i].fw_id;
+
+ clk = of_clk_get_from_provider(&scmi_spec);
+ if (IS_ERR(clk))
+ return dev_err_cast_probe(dev, clk,
+ "Failed to get SCMI clock %u\n", map[i].fw_id);
+
+ hw = __clk_get_hw(clk);
+ if (!hw && fallback != map_in) {
+ for (unsigned int j = 0; fallback[j].dt_id >= 0; j++) {
+ if (fallback[j].dt_id == map_in[i].dt_id) {
+ enum fixed_clk idx = fallback[j].fw_id - FIXED_CLK_OFFSET;
+
+ map[i].hw = fixed_hws[idx];
+ break;
+ }
+ }
+ if (map[i].hw)
+ continue;
+ }
+
+ if (!hw) {
+ /* CLOCK_ATTRIBUTES is not supported */
+ dev_warn(dev, "SCMI clock %u is NULL\n", map[i].fw_id);
+ continue;
+ }
+
+ dev_dbg(priv->dev, "SCMI clock %u is %s at %lu Hz\n",
+ map[i].fw_id, clk_hw_get_name(hw), clk_hw_get_rate(hw));
+
+ map[i].hw = hw;
}
devm_kfree(dev, fixed_hws);
@@ -154,20 +232,55 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
static int r8a78000_cpg_probe(struct platform_device *pdev)
{
+ struct device_node *scmi_clk_np __free(device_node) = NULL;
+ struct device_node *scmi __free(device_node) = NULL;
struct device *dev = &pdev->dev;
+ const struct cpg_data *cpg_data;
struct r8a78000_cpg_priv *priv;
const struct clk_map_in *map;
- map = of_device_get_match_data(dev);
- if (!map)
+ cpg_data = of_device_get_match_data(dev);
+ if (!cpg_data)
return -ENODEV;
+ map = cpg_data->default_map;
+
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
priv->dev = dev;
- priv->map = fill_clk_map(priv, map);
+
+ if (!IS_ENABLED(CONFIG_COMMON_CLK_SCMI)) {
+ dev_dbg(dev, "SCMI clock support is disabled\n");
+ goto fallback;
+ }
+
+ scmi = of_find_compatible_node(NULL, NULL, "arm,scmi");
+ if (!scmi) {
+ dev_dbg(dev, "Cannot find SCMI firmware node\n");
+ goto fallback;
+ }
+
+ if (!of_device_is_available(scmi)) {
+ dev_dbg(dev, "SCMI firmware node is not available\n");
+ goto fallback;
+ }
+
+ scmi_clk_np = scmi_find_proto(scmi, SCMI_PROTOCOL_CLOCK);
+ if (!scmi_clk_np) {
+ dev_dbg(dev, "Cannot find SCMI clock management protocol\n");
+ goto fallback;
+ }
+
+ map = cpg_data->fw_map;
+
+fallback:
+ /*
+ * We cannot do lazy look-up in r8a78000_clk_get(), as that function is
+ * called with of_clk_mutex already held.
+ */
+ priv->map = fill_clk_map(priv, map, cpg_data->default_map, scmi_clk_np);
if (IS_ERR(priv->map))
return PTR_ERR(priv->map);
@@ -181,10 +294,24 @@ static const struct clk_map_in r8a78000_cpg_default[] = {
{ -1 }
};
+static const struct clk_map_in r8a78000_cpg_fw[] = {
+ { R8A78000_CPG_SGASYNCD4_PERW_BUS, 1661 },
+ { R8A78000_CPG_SGASYNCD8_PERW_BUS, 1662 },
+ { R8A78000_CPG_SGASYNCD16_PERW_BUS, 1663 },
+ { R8A78000_CPG_MSO_SCP_MAIN, 1459 },
+ { R8A78000_CPG_MSOCK_PERW_BUS, 1667 },
+ { -1 }
+};
+
+static const struct cpg_data r8a78000_cpg_data = {
+ .default_map = r8a78000_cpg_default,
+ .fw_map = r8a78000_cpg_fw,
+};
+
static const struct of_device_id r8a78000_cpg_match[] = {
{
.compatible = "renesas,r8a78000-cpg",
- .data = &r8a78000_cpg_default,
+ .data = &r8a78000_cpg_data,
},
{ /* sentinel */ }
};
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support
2026-10-16 15:55 ` [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support Geert Uytterhoeven
@ 2026-10-16 16:13 ` sashiko-bot
2026-10-16 16:17 ` Geert Uytterhoeven
0 siblings, 1 reply; 16+ messages in thread
From: sashiko-bot @ 2026-10-16 16:13 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: memory leak of struct clk and module refcount on success and probe deferral
- [High] clk: renesas: UAF on provider unbind and struct clk memory leak
- [Medium] clk: memory and module refcount leak in fill_clk_map()
--
commit a959fac1d9976d318a861219d6e13c9fef99da15
Author: Geert Uytterhoeven <geert+renesas@glider.be>
clk: renesas: r8a78000: Add SCMI remapping support
This commit extends the Clock Pulse Generator driver for R-Car X5H with SCMI
support. It maps DT clock IDs to abstractions offered by the SCMI firmware,
falling back to fixed-rate clocks if unavailable.
> diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c
> index 5ffd7b89cb48e..758cfe78ae58e 100644
> --- a/drivers/clk/renesas/r8a78000-cpg.c
> +++ b/drivers/clk/renesas/r8a78000-cpg.c
[ ... ]
> @@ -145,7 +184,46 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
> break;
>
> map[i].fw_id = map_in[i].fw_id;
> - map[i].hw = fixed_hws[map[i].fw_id];
> + if (map[i].fw_id >= FIXED_CLK_OFFSET) {
> + enum fixed_clk idx = map[i].fw_id - FIXED_CLK_OFFSET;
> +
> + map[i].hw = fixed_hws[idx];
> + continue;
> + }
> +
> + scmi_spec.np = scmi_clk_np;
> + scmi_spec.args_count = 1;
> + scmi_spec.args[0] = map[i].fw_id;
> +
> + clk = of_clk_get_from_provider(&scmi_spec);
> + if (IS_ERR(clk))
> + return dev_err_cast_probe(dev, clk,
> + "Failed to get SCMI clock %u\n", map[i].fw_id);
> +
> + hw = __clk_get_hw(clk);
[Severity: High]
Does this code leak the dynamically allocated struct clk and its module
reference?
of_clk_get_from_provider() returns a new struct clk and increments the
provider's module refcount. The clk pointer is discarded after extracting
the raw clk_hw pointer, so it is never passed to clk_put(). This causes a
memory leak of struct clk objects and a module refcount leak for the SCMI
provider.
If a subsequent clock lookup in the loop fails and returns -EPROBE_DEFER,
the function immediately aborts, and the previously allocated clk handles
from earlier loop iterations are not cleaned up.
Additionally, caching the dangling clk_hw pointer in map[i].hw without holding
a device link or proper reference can lead to a use-after-free. If the SCMI
provider is unbound via sysfs, devres will free the clk_hw structures, but the
CPG driver will still hold and use the stale pointers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792163045.git.geert+renesas@glider.be?part=2
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support
2026-10-16 16:13 ` sashiko-bot
@ 2026-10-16 16:17 ` Geert Uytterhoeven
0 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 16:17 UTC (permalink / raw)
To: sashiko-reviews; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
On Fri, 16 Oct 2026 at 18:13, <sashiko-bot@kernel.org> wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] clk: memory leak of struct clk and module refcount on success and probe deferral
> - [High] clk: renesas: UAF on provider unbind and struct clk memory leak
> - [Medium] clk: memory and module refcount leak in fill_clk_map()
> --
>
> commit a959fac1d9976d318a861219d6e13c9fef99da15
> Author: Geert Uytterhoeven <geert+renesas@glider.be>
>
> clk: renesas: r8a78000: Add SCMI remapping support
>
> This commit extends the Clock Pulse Generator driver for R-Car X5H with SCMI
> support. It maps DT clock IDs to abstractions offered by the SCMI firmware,
> falling back to fixed-rate clocks if unavailable.
>
> > diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c
> > index 5ffd7b89cb48e..758cfe78ae58e 100644
> > --- a/drivers/clk/renesas/r8a78000-cpg.c
> > +++ b/drivers/clk/renesas/r8a78000-cpg.c
> [ ... ]
> > @@ -145,7 +184,46 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
> > break;
> >
> > map[i].fw_id = map_in[i].fw_id;
> > - map[i].hw = fixed_hws[map[i].fw_id];
> > + if (map[i].fw_id >= FIXED_CLK_OFFSET) {
> > + enum fixed_clk idx = map[i].fw_id - FIXED_CLK_OFFSET;
> > +
> > + map[i].hw = fixed_hws[idx];
> > + continue;
> > + }
> > +
> > + scmi_spec.np = scmi_clk_np;
> > + scmi_spec.args_count = 1;
> > + scmi_spec.args[0] = map[i].fw_id;
> > +
> > + clk = of_clk_get_from_provider(&scmi_spec);
> > + if (IS_ERR(clk))
> > + return dev_err_cast_probe(dev, clk,
> > + "Failed to get SCMI clock %u\n", map[i].fw_id);
> > +
> > + hw = __clk_get_hw(clk);
>
> [Severity: High]
> Does this code leak the dynamically allocated struct clk and its module
> reference?
>
> of_clk_get_from_provider() returns a new struct clk and increments the
> provider's module refcount. The clk pointer is discarded after extracting
> the raw clk_hw pointer, so it is never passed to clk_put(). This causes a
> memory leak of struct clk objects and a module refcount leak for the SCMI
> provider.
>
> If a subsequent clock lookup in the loop fails and returns -EPROBE_DEFER,
> the function immediately aborts, and the previously allocated clk handles
> from earlier loop iterations are not cleaned up.
This cannot happen, as all of_clk_get_from_provider() calls use the
same clock provider: either the first call returns -EPROBE_DEFER,
or all calls do not return -EPROBE_DEFER.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 3/8] reset: Extract __reset_control_get_from_provider()
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
2026-10-16 15:55 ` [PATCH v4 1/8] clk: renesas: r8a78000: Add clk_hw to clk_map Geert Uytterhoeven
2026-10-16 15:55 ` [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 15:55 ` [PATCH v4 4/8] reset: Add reset_control_get_from_provider_exclusive() Geert Uytterhoeven
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
Extract the code to create a reset_control structure from a given
provider into its own function, so it can be reused later.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
v4:
- Keep RESET_CONTROL_FLAGS_BIT_OPTIONAL clearing in
__fwnode_reset_control_get(),
- Return directly instead of assigning to a local variable, letting us
drop the latter and simplify scoped_guard() to plain guard(),
- Add Reviewed-by,
v3:
- New.
---
drivers/reset/core.c | 85 +++++++++++++++++++++++---------------------
1 file changed, 45 insertions(+), 40 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09b270..0e11f4c81c10d601 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -1135,6 +1135,49 @@ __reset_find_rcdev(const struct fwnode_reference_args *args, bool gpio_fallback)
return NULL;
}
+static struct reset_control *
+__reset_control_get_from_provider(const struct fwnode_reference_args *args,
+ struct fwnode_handle *consumer, int index,
+ bool gpio_fallback,
+ enum reset_control_flags flags)
+{
+ struct reset_controller_dev *rcdev;
+ int rstc_id = -EINVAL;
+
+ guard(mutex)(&reset_list_mutex);
+
+ rcdev = __reset_find_rcdev(args, gpio_fallback);
+ if (!rcdev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ if (WARN_ON(args->nargs != rcdev->fwnode_reset_n_cells))
+ return ERR_PTR(-EINVAL);
+
+ if (rcdev->of_xlate && is_of_node(consumer)) {
+ struct device_node *np = to_of_node(consumer);
+ struct of_phandle_args of_args;
+ int ret;
+
+ ret = of_parse_phandle_with_args(np,
+ gpio_fallback ? "reset-gpios" : "resets",
+ gpio_fallback ? "#gpio-cells" : "#reset-cells",
+ gpio_fallback ? 0 : index,
+ &of_args);
+ if (ret)
+ return ERR_PTR(ret);
+
+ rstc_id = rcdev->of_xlate(rcdev, &of_args);
+ of_node_put(of_args.np);
+ } else if (rcdev->fwnode_xlate) {
+ rstc_id = rcdev->fwnode_xlate(rcdev, args);
+ }
+ if (rstc_id < 0)
+ return ERR_PTR(rstc_id);
+
+ guard(mutex)(&rcdev->lock);
+ return __reset_control_get_internal(rcdev, rstc_id, flags);
+}
+
struct reset_control *
__fwnode_reset_control_get(struct fwnode_handle *fwnode, const char *id, int index,
enum reset_control_flags flags)
@@ -1142,10 +1185,7 @@ __fwnode_reset_control_get(struct fwnode_handle *fwnode, const char *id, int ind
bool optional = flags & RESET_CONTROL_FLAGS_BIT_OPTIONAL;
bool gpio_fallback = false;
struct reset_control *rstc = ERR_PTR(-EINVAL);
- struct reset_controller_dev *rcdev;
struct fwnode_reference_args args;
- struct of_phandle_args of_args;
- int rstc_id = -EINVAL;
int ret;
if (!fwnode)
@@ -1185,46 +1225,11 @@ __fwnode_reset_control_get(struct fwnode_handle *fwnode, const char *id, int ind
}
}
- guard(mutex)(&reset_list_mutex);
-
- rcdev = __reset_find_rcdev(&args, gpio_fallback);
- if (!rcdev) {
- rstc = ERR_PTR(-EPROBE_DEFER);
- goto out_put;
- }
-
- if (WARN_ON(args.nargs != rcdev->fwnode_reset_n_cells)) {
- rstc = ERR_PTR(-EINVAL);
- goto out_put;
- }
-
- if (rcdev->of_xlate && is_of_node(fwnode)) {
- ret = of_parse_phandle_with_args(to_of_node(fwnode),
- gpio_fallback ? "reset-gpios" : "resets",
- gpio_fallback ? "#gpio-cells" : "#reset-cells",
- gpio_fallback ? 0 : index,
- &of_args);
- if (ret) {
- rstc = ERR_PTR(ret);
- goto out_put;
- }
-
- rstc_id = rcdev->of_xlate(rcdev, &of_args);
- of_node_put(of_args.np);
- } else if (rcdev->fwnode_xlate) {
- rstc_id = rcdev->fwnode_xlate(rcdev, &args);
- }
- if (rstc_id < 0) {
- rstc = ERR_PTR(rstc_id);
- goto out_put;
- }
-
flags &= ~RESET_CONTROL_FLAGS_BIT_OPTIONAL;
- scoped_guard(mutex, &rcdev->lock)
- rstc = __reset_control_get_internal(rcdev, rstc_id, flags);
+ rstc = __reset_control_get_from_provider(&args, fwnode, index,
+ gpio_fallback, flags);
-out_put:
fwnode_handle_put(args.fwnode);
return rstc;
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 4/8] reset: Add reset_control_get_from_provider_exclusive()
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
` (2 preceding siblings ...)
2026-10-16 15:55 ` [PATCH v4 3/8] reset: Extract __reset_control_get_from_provider() Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 16:02 ` sashiko-bot
2026-10-16 15:55 ` [PATCH v4 5/8] pmdomain: Make genpd_get_from_provider() return -EPROBE_DEFER Geert Uytterhoeven
` (3 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
When stacking two reset controllers, the upper one should go through the
proper channels: request exclusive reset controls and use them via the
reset_control_*() APIs. However, unlike the clock subsystem, which has
of_clk_get_from_provider(), the reset subsystem does not offer a way to
lookup and obtain a reset from a reset provider.
Hence add a public helper reset_control_get_from_provider_exclusive()
to lookup and obtain an exclusive reference to a reset controller, based
on the recently introduced __reset_control_get_from_provider() helper.
This will be used by the R-Car X5H Module Controller driver.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- Check for overflow when converting (u64) fwnode args to (uint32_t)
OF args,
- Document that reset_control_get_from_provider_exclusive() must not
be given a reference to a GPIO provider,
- Let the dummy return ERR_PTR(-ENOTSUPP) instead of NULL,
v3:
- Replace reset_controller_get_provider() by
reset_control_get_from_provider_exclusive(),
- Add kerneldoc,
v2:
- No changes.
---
drivers/reset/core.c | 41 ++++++++++++++++++++++++++++++++++++-----
include/linux/reset.h | 8 ++++++++
2 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 0e11f4c81c10d601..421bcc4c89cc21ac 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -1141,6 +1141,7 @@ __reset_control_get_from_provider(const struct fwnode_reference_args *args,
bool gpio_fallback,
enum reset_control_flags flags)
{
+ struct fwnode_handle *fwnode = consumer ?: args->fwnode;
struct reset_controller_dev *rcdev;
int rstc_id = -EINVAL;
@@ -1153,18 +1154,30 @@ __reset_control_get_from_provider(const struct fwnode_reference_args *args,
if (WARN_ON(args->nargs != rcdev->fwnode_reset_n_cells))
return ERR_PTR(-EINVAL);
- if (rcdev->of_xlate && is_of_node(consumer)) {
- struct device_node *np = to_of_node(consumer);
+ if (rcdev->of_xlate && is_of_node(fwnode)) {
+ struct device_node *np = to_of_node(fwnode);
struct of_phandle_args of_args;
int ret;
- ret = of_parse_phandle_with_args(np,
+ if (consumer) {
+ ret = of_parse_phandle_with_args(np,
gpio_fallback ? "reset-gpios" : "resets",
gpio_fallback ? "#gpio-cells" : "#reset-cells",
gpio_fallback ? 0 : index,
&of_args);
- if (ret)
- return ERR_PTR(ret);
+ if (ret)
+ return ERR_PTR(ret);
+ } else {
+ of_args.np = of_node_get(np);
+ of_args.args_count = args->nargs;
+
+ for (unsigned int i = 0; i < args->nargs; i++) {
+ if (args->args[i] > U32_MAX)
+ return ERR_PTR(-EINVAL);
+
+ of_args.args[i] = args->args[i];
+ }
+ }
rstc_id = rcdev->of_xlate(rcdev, &of_args);
of_node_put(of_args.np);
@@ -1254,6 +1267,24 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id,
}
EXPORT_SYMBOL_GPL(__reset_control_get);
+/**
+ * reset_control_get_from_provider_exclusive - Lookup and obtain an exclusive
+ * reference to a reset controller.
+ * @args: Reference to the reset controller provider with all the args like
+ * reset number. This must not be a reference to a GPIO provider.
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ * If this function is called more than once for the same reset control it will
+ * return -EBUSY.
+ */
+struct reset_control *
+reset_control_get_from_provider_exclusive(const struct fwnode_reference_args *args)
+{
+ return __reset_control_get_from_provider(args, NULL, 0, false,
+ RESET_CONTROL_EXCLUSIVE);
+}
+EXPORT_SYMBOL_GPL(reset_control_get_from_provider_exclusive);
+
int __reset_control_bulk_get(struct device *dev, int num_rstcs,
struct reset_control_bulk_data *rstcs,
enum reset_control_flags flags)
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 9c391cf0c82298a0..384273ec0c7cbef7 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -90,6 +90,8 @@ struct reset_control *__fwnode_reset_control_get(struct fwnode_handle *fwnode,
const char *id, int index, enum reset_control_flags flags);
struct reset_control *__reset_control_get(struct device *dev, const char *id,
int index, enum reset_control_flags flags);
+struct reset_control *reset_control_get_from_provider_exclusive(
+ const struct fwnode_reference_args *args);
void reset_control_put(struct reset_control *rstc);
int __reset_control_bulk_get(struct device *dev, int num_rstcs,
struct reset_control_bulk_data *rstcs,
@@ -173,6 +175,12 @@ static inline struct reset_control *__reset_control_get(
return optional ? NULL : ERR_PTR(-ENOTSUPP);
}
+static inline struct reset_control *reset_control_get_from_provider_exclusive(
+ const struct fwnode_reference_args *args)
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
static inline int
reset_control_bulk_reset(int num_rstcs, struct reset_control_bulk_data *rstcs)
{
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 4/8] reset: Add reset_control_get_from_provider_exclusive()
2026-10-16 15:55 ` [PATCH v4 4/8] reset: Add reset_control_get_from_provider_exclusive() Geert Uytterhoeven
@ 2026-10-16 16:02 ` sashiko-bot
2026-10-16 16:08 ` Geert Uytterhoeven
0 siblings, 1 reply; 16+ messages in thread
From: sashiko-bot @ 2026-10-16 16:02 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] of: memory leak of device node reference on error path
--
commit 6e1c5e27c808997c3ce5fe5fc257fedbfabb6954
Author: Geert Uytterhoeven <geert+renesas@glider.be>
reset: Add reset_control_get_from_provider_exclusive()
This patch introduces reset_control_get_from_provider_exclusive() to allow
looking up and obtaining an exclusive reference to a reset controller from
a provider. It modifies the existing __reset_control_get_from_provider()
helper to support this lookup.
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 0e11f4c81c10d..421bcc4c89cc2 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -1153,18 +1154,30 @@ __reset_control_get_from_provider(const struct fwnode_reference_args *args,
> if (WARN_ON(args->nargs != rcdev->fwnode_reset_n_cells))
> return ERR_PTR(-EINVAL);
>
> - if (rcdev->of_xlate && is_of_node(consumer)) {
> - struct device_node *np = to_of_node(consumer);
> + if (rcdev->of_xlate && is_of_node(fwnode)) {
> + struct device_node *np = to_of_node(fwnode);
> struct of_phandle_args of_args;
> int ret;
>
> - ret = of_parse_phandle_with_args(np,
> + if (consumer) {
> + ret = of_parse_phandle_with_args(np,
> gpio_fallback ? "reset-gpios" : "resets",
> gpio_fallback ? "#gpio-cells" : "#reset-cells",
> gpio_fallback ? 0 : index,
> &of_args);
> - if (ret)
> - return ERR_PTR(ret);
> + if (ret)
> + return ERR_PTR(ret);
> + } else {
> + of_args.np = of_node_get(np);
> + of_args.args_count = args->nargs;
> +
> + for (unsigned int i = 0; i < args->nargs; i++) {
> + if (args->args[i] > U32_MAX)
> + return ERR_PTR(-EINVAL);
[Severity: Medium]
Will this leak the device node reference acquired above?
When consumer is NULL in __reset_control_get_from_provider(), the code
acquires a device node reference via of_args.np = of_node_get(np).
If an argument exceeds U32_MAX, the function immediately returns
ERR_PTR(-EINVAL). This early return skips the of_node_put(of_args.np) call
at the end of the block, permanently leaking the OF node reference count.
> +
> + of_args.args[i] = args->args[i];
> + }
> + }
>
> rstc_id = rcdev->of_xlate(rcdev, &of_args);
> of_node_put(of_args.np);
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792163045.git.geert+renesas@glider.be?part=4
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 4/8] reset: Add reset_control_get_from_provider_exclusive()
2026-10-16 16:02 ` sashiko-bot
@ 2026-10-16 16:08 ` Geert Uytterhoeven
0 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 16:08 UTC (permalink / raw)
To: sashiko-reviews; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
Hi Sashiko,
On Fri, 16 Oct 2026 at 18:02, <sashiko-bot@kernel.org> wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] of: memory leak of device node reference on error path
> --
>
> commit 6e1c5e27c808997c3ce5fe5fc257fedbfabb6954
> Author: Geert Uytterhoeven <geert+renesas@glider.be>
>
> reset: Add reset_control_get_from_provider_exclusive()
>
> This patch introduces reset_control_get_from_provider_exclusive() to allow
> looking up and obtaining an exclusive reference to a reset controller from
> a provider. It modifies the existing __reset_control_get_from_provider()
> helper to support this lookup.
>
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 0e11f4c81c10d..421bcc4c89cc2 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -1153,18 +1154,30 @@ __reset_control_get_from_provider(const struct fwnode_reference_args *args,
> > if (WARN_ON(args->nargs != rcdev->fwnode_reset_n_cells))
> > return ERR_PTR(-EINVAL);
> >
> > - if (rcdev->of_xlate && is_of_node(consumer)) {
> > - struct device_node *np = to_of_node(consumer);
> > + if (rcdev->of_xlate && is_of_node(fwnode)) {
> > + struct device_node *np = to_of_node(fwnode);
> > struct of_phandle_args of_args;
> > int ret;
> >
> > - ret = of_parse_phandle_with_args(np,
> > + if (consumer) {
> > + ret = of_parse_phandle_with_args(np,
> > gpio_fallback ? "reset-gpios" : "resets",
> > gpio_fallback ? "#gpio-cells" : "#reset-cells",
> > gpio_fallback ? 0 : index,
> > &of_args);
> > - if (ret)
> > - return ERR_PTR(ret);
> > + if (ret)
> > + return ERR_PTR(ret);
> > + } else {
> > + of_args.np = of_node_get(np);
> > + of_args.args_count = args->nargs;
> > +
> > + for (unsigned int i = 0; i < args->nargs; i++) {
> > + if (args->args[i] > U32_MAX)
> > + return ERR_PTR(-EINVAL);
>
> [Severity: Medium]
> Will this leak the device node reference acquired above?
>
> When consumer is NULL in __reset_control_get_from_provider(), the code
> acquires a device node reference via of_args.np = of_node_get(np).
>
> If an argument exceeds U32_MAX, the function immediately returns
> ERR_PTR(-EINVAL). This early return skips the of_node_put(of_args.np) call
> at the end of the block, permanently leaking the OF node reference count.
Yes it will!
This can easily be fixed by moving the for-loop to the top of the
branch, i.e. before the of_node_get().
BTW, perhaps the time is ripe for helper functions to convert between
fwnode_reference_args and of_phandle_args?
>
> > +
> > + of_args.args[i] = args->args[i];
> > + }
> > + }
> >
> > rstc_id = rcdev->of_xlate(rcdev, &of_args);
> > of_node_put(of_args.np);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792163045.git.geert+renesas@glider.be?part=4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 5/8] pmdomain: Make genpd_get_from_provider() return -EPROBE_DEFER
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
` (3 preceding siblings ...)
2026-10-16 15:55 ` [PATCH v4 4/8] reset: Add reset_control_get_from_provider_exclusive() Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 16:09 ` sashiko-bot
2026-10-16 15:55 ` [PATCH v4 6/8] pmdomain: Make genpd_get_from_provider() public Geert Uytterhoeven
` (2 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
Unlike provider-based look-up functions in other subsystems[1],
genpd_get_from_provider() returns -ENOENT instead of -EPROBE_DEFER when
the provider is not found, thus not supporting probe deferral.
Currently this is worked around in of_genpd_add_subdomain() by remapping
-ENOENT[2].
Fix this for all current and future callers by returning -EPROBE_DEFER
instead, and drop the no longer needed remapping code.
[1] of_clk_get_hw_from_clkspec(), __fwnode_reset_control_get()),
[2] Commit 18027d6f392ee8d8 ("PM: domains: Make of_genpd_add_subdomain()
return -EPROBE_DEFER").
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- New.
---
drivers/pmdomain/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 964e44c2f78f1d55..88c886814a5b9da3 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3021,7 +3021,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
static struct generic_pm_domain *genpd_get_from_provider(
const struct of_phandle_args *genpdspec)
{
- struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
+ struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
struct of_genpd_provider *provider;
if (!genpdspec)
@@ -3276,7 +3276,7 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
out:
mutex_unlock(&gpd_list_lock);
- return ret == -ENOENT ? -EPROBE_DEFER : ret;
+ return ret;
}
EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 5/8] pmdomain: Make genpd_get_from_provider() return -EPROBE_DEFER
2026-10-16 15:55 ` [PATCH v4 5/8] pmdomain: Make genpd_get_from_provider() return -EPROBE_DEFER Geert Uytterhoeven
@ 2026-10-16 16:09 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-10-16 16:09 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] pmdomain: unnecessary probe deferral on invalid domain index
--
commit c105920d803c0cf9396fb90d8428fdc4d83e8655
Author: Geert Uytterhoeven <geert+renesas@glider.be>
pmdomain: Make genpd_get_from_provider() return -EPROBE_DEFER
Unlike provider-based look-up functions in other subsystems[1],
genpd_get_from_provider() returns -ENOENT instead of -EPROBE_DEFER when
the provider is not found, thus not supporting probe deferral.
Currently this is worked around in of_genpd_add_subdomain() by remapping
-ENOENT[2].
Fix this for all current and future callers by returning -EPROBE_DEFER
instead, and drop the no longer needed remapping code.
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 964e44c2f78f1..88c886814a5b9 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3021,7 +3021,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
> static struct generic_pm_domain *genpd_get_from_provider(
> const struct of_phandle_args *genpdspec)
> {
> - struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
> + struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
> struct of_genpd_provider *provider;
>
> if (!genpdspec)
[ ... ]
> @@ -3276,7 +3276,7 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
> out:
> mutex_unlock(&gpd_list_lock);
>
> - return ret == -ENOENT ? -EPROBE_DEFER : ret;
> + return ret;
> }
[Severity: Low]
Does this change leave an unnecessary probe deferral in __genpd_dev_pm_attach()?
If a device specifies a valid PM domain provider but an invalid domain index,
genpd_get_from_provider() returns the specific error code from the provider's
xlate callback, such as -ENOENT.
However, __genpd_dev_pm_attach() still unconditionally routes all errors
through driver_deferred_probe_check_state():
drivers/pmdomain/core.c:__genpd_dev_pm_attach() {
...
pd = genpd_get_from_provider(&pd_args);
of_node_put(pd_args.np);
if (IS_ERR(pd)) {
mutex_unlock(&gpd_list_lock);
dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
__func__, PTR_ERR(pd));
return driver_deferred_probe_check_state(base_dev);
}
...
}
Could this unnecessarily delay boot and error reporting for devices with
permanently invalid PM domain IDs by forcing a wait until late_initcall?
Should __genpd_dev_pm_attach() be updated to only call
driver_deferred_probe_check_state() when PTR_ERR(pd) is -EPROBE_DEFER?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792163045.git.geert+renesas@glider.be?part=5
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 6/8] pmdomain: Make genpd_get_from_provider() public
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
` (4 preceding siblings ...)
2026-10-16 15:55 ` [PATCH v4 5/8] pmdomain: Make genpd_get_from_provider() return -EPROBE_DEFER Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 16:08 ` sashiko-bot
2026-10-16 15:55 ` [PATCH v4 7/8] pmdomain: renesas: r8a78000: Add SCMI remapping support Geert Uytterhoeven
2026-10-16 15:55 ` [PATCH v4 8/8] arm64: dts: renesas: ironhide: Enable SCMI devpd, sys, clock, and reset Geert Uytterhoeven
7 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
Unlike the corresponding function in the clock subsystem[1],
genpd_get_from_provider() is private, and thus cannot be used by PM
Domain drivers.
Make it public, so the R-Car X5H Module Controller driver can call it.
Add an "of_"-prefix for consistency with public genpd function naming.
[1] of_clk_get_from_provider().
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- Improve patch description,
v3:
- Add "of_" prefix,
v2:
- No changes.
---
drivers/pmdomain/core.c | 22 +++++++++++-----------
include/linux/pm_domain.h | 7 +++++++
2 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 88c886814a5b9da3..4136368456d994f7 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3008,7 +3008,7 @@ void of_genpd_del_provider(struct device_node *np)
EXPORT_SYMBOL_GPL(of_genpd_del_provider);
/**
- * genpd_get_from_provider() - Look-up PM domain
+ * of_genpd_get_from_provider() - Look-up PM domain
* @genpdspec: OF phandle args to use for look-up
*
* Looks for a PM domain provider under the node specified by @genpdspec and if
@@ -3018,8 +3018,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
* Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
* on failure.
*/
-static struct generic_pm_domain *genpd_get_from_provider(
- const struct of_phandle_args *genpdspec)
+struct generic_pm_domain *of_genpd_get_from_provider(const struct of_phandle_args *genpdspec)
{
struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
struct of_genpd_provider *provider;
@@ -3041,6 +3040,7 @@ static struct generic_pm_domain *genpd_get_from_provider(
return genpd;
}
+EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
/**
* of_genpd_add_child_ids() - Parse power-domains-child-ids property
@@ -3113,7 +3113,7 @@ int of_genpd_add_child_ids(struct device_node *np,
goto err_unwind;
/* Get the parent domain */
- parent_genpd = genpd_get_from_provider(&parent_args);
+ parent_genpd = of_genpd_get_from_provider(&parent_args);
of_node_put(parent_args.np);
if (IS_ERR(parent_genpd)) {
pr_err("Failed to get parent domain for %pOF: %ld\n",
@@ -3196,7 +3196,7 @@ int of_genpd_remove_child_ids(struct device_node *np,
if (ret)
continue;
- parent_genpd = genpd_get_from_provider(&parent_args);
+ parent_genpd = of_genpd_get_from_provider(&parent_args);
of_node_put(parent_args.np);
if (IS_ERR(parent_genpd))
continue;
@@ -3227,7 +3227,7 @@ int of_genpd_add_device(const struct of_phandle_args *genpdspec, struct device *
mutex_lock(&gpd_list_lock);
- genpd = genpd_get_from_provider(genpdspec);
+ genpd = of_genpd_get_from_provider(genpdspec);
if (IS_ERR(genpd)) {
ret = PTR_ERR(genpd);
goto out;
@@ -3259,13 +3259,13 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
mutex_lock(&gpd_list_lock);
- parent = genpd_get_from_provider(parent_spec);
+ parent = of_genpd_get_from_provider(parent_spec);
if (IS_ERR(parent)) {
ret = PTR_ERR(parent);
goto out;
}
- subdomain = genpd_get_from_provider(subdomain_spec);
+ subdomain = of_genpd_get_from_provider(subdomain_spec);
if (IS_ERR(subdomain)) {
ret = PTR_ERR(subdomain);
goto out;
@@ -3297,13 +3297,13 @@ int of_genpd_remove_subdomain(const struct of_phandle_args *parent_spec,
mutex_lock(&gpd_list_lock);
- parent = genpd_get_from_provider(parent_spec);
+ parent = of_genpd_get_from_provider(parent_spec);
if (IS_ERR(parent)) {
ret = PTR_ERR(parent);
goto out;
}
- subdomain = genpd_get_from_provider(subdomain_spec);
+ subdomain = of_genpd_get_from_provider(subdomain_spec);
if (IS_ERR(subdomain)) {
ret = PTR_ERR(subdomain);
goto out;
@@ -3491,7 +3491,7 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
return ret;
mutex_lock(&gpd_list_lock);
- pd = genpd_get_from_provider(&pd_args);
+ pd = of_genpd_get_from_provider(&pd_args);
of_node_put(pd_args.np);
if (IS_ERR(pd)) {
mutex_unlock(&gpd_list_lock);
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 14e0e346c610a9a6..d65ee8b3725c1bcb 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -467,6 +467,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
int of_genpd_add_provider_onecell(struct device_node *np,
struct genpd_onecell_data *data);
void of_genpd_del_provider(struct device_node *np);
+struct generic_pm_domain *of_genpd_get_from_provider(const struct of_phandle_args *genpdspec);
int of_genpd_add_device(const struct of_phandle_args *args, struct device *dev);
int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
const struct of_phandle_args *subdomain_spec);
@@ -501,6 +502,12 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
static inline void of_genpd_del_provider(struct device_node *np) {}
+static inline struct generic_pm_domain *of_genpd_get_from_provider(
+ const struct of_phandle_args *genpdspec)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline int of_genpd_add_device(const struct of_phandle_args *args,
struct device *dev)
{
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 6/8] pmdomain: Make genpd_get_from_provider() public
2026-10-16 15:55 ` [PATCH v4 6/8] pmdomain: Make genpd_get_from_provider() public Geert Uytterhoeven
@ 2026-10-16 16:08 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-10-16 16:08 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pmdomain: UAF due to un-refcounted genpd pointer export
--
commit a7a3a37d2be118bab420ebff1f13629dfa676eb6
Author: Geert Uytterhoeven <geert+renesas@glider.be>
pmdomain: Make genpd_get_from_provider() public
This commit makes genpd_get_from_provider() public as
of_genpd_get_from_provider(), allowing PM Domain drivers to look up
generic_pm_domain structures directly.
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 88c886814a5b9..4136368456d99 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3018,8 +3018,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
> * Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
> * on failure.
> */
> -static struct generic_pm_domain *genpd_get_from_provider(
> - const struct of_phandle_args *genpdspec)
> +struct generic_pm_domain *of_genpd_get_from_provider(const struct of_phandle_args *genpdspec)
> {
> struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
> struct of_genpd_provider *provider;
> @@ -3041,6 +3040,7 @@ static struct generic_pm_domain *genpd_get_from_provider(
>
> return genpd;
> }
> +EXPORT_SYMBOL_GPL(of_genpd_get_from_provider);
[Severity: High]
Can exporting this API lead to a use-after-free regression?
of_genpd_get_from_provider() returns an unpinned, un-refcounted pointer to
struct generic_pm_domain whose lifetime is managed by the provider. While the
commit message compares this to the clock subsystem, clock pointers are
refcounted or uniquely allocated.
The follow-up r8a78000-mdlc patch demonstrates caching these genpd pointers in
its power_map without acquiring any locks (since gpd_list_lock is internal).
Because the SCMI provider is looked up dynamically via
of_find_compatible_node(), fw_devlink cannot automatically detect the
dependency to create a device link.
If the SCMI driver is unbound (via sysfs or module unload), pm_genpd_remove()
will free the genpd structures, leaving dangling pointers. Furthermore,
overwriting the attach_dev and detach_dev function pointers of the SCMI genpd
objects will lead to invalid execution if the module is unloaded.
[ ... ]
> @@ -3113,7 +3113,7 @@ int of_genpd_add_child_ids(struct device_node *np,
> goto err_unwind;
>
> /* Get the parent domain */
> - parent_genpd = genpd_get_from_provider(&parent_args);
> + parent_genpd = of_genpd_get_from_provider(&parent_args);
[Severity: High]
This problem wasn't introduced by this patch, but does this internal caller
have a time-of-check to time-of-use (TOCTOU) race?
Fetching parent_genpd locklessly in of_genpd_add_child_ids() allows a
concurrent pm_genpd_remove() to free the domain before gpd_list_lock is
subsequently acquired.
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792163045.git.geert+renesas@glider.be?part=6
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 7/8] pmdomain: renesas: r8a78000: Add SCMI remapping support
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
` (5 preceding siblings ...)
2026-10-16 15:55 ` [PATCH v4 6/8] pmdomain: Make genpd_get_from_provider() public Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
2026-10-16 16:15 ` sashiko-bot
2026-10-16 15:55 ` [PATCH v4 8/8] arm64: dts: renesas: ironhide: Enable SCMI devpd, sys, clock, and reset Geert Uytterhoeven
7 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
Extend the Module Controller driver for R-Car X5H with SCMI support, to
remap hardware power domain, module, and reset IDs to abstractions
(power domains, clocks, and resets) offered by the SCMI firmware when
running on a system with SCMI, and SCMI support is enabled. Take into
account some peculiarities of the current SCMI firmware implementations,
like missing handling of gateable functional clock parents.
Support for power domains, and module clocks and resets used by serial,
I2C, and SPI devices is included.
Note that SCMI clocks representing hardware modules expose access to
Module Standby control: they are fake clocks, with a zero clock rate,
that can just be enabled and disabled. Hence they are controlled from
the clock domain's start/stop callbacks, together with gateable
functional clock parents.
This lets us avoid putting SCMI IDs in DT, and allows us to use the same
hardware description in SoC DTS for systems with and without SCMI
firmware.
For now, it is assumed SCMI power domain, clock, and reset IDs are
stable, and match SCP FW SDKv4.31.0 and later.
Suggested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- Let r8a78000_mdlc_attach_dev() return success when the MDLC instance
is not found, to avoid breaking users referring in DT to an SCMI
power-domain directly,
- Mark scmi_{reset,power}_np __free(device_node),
- Extract scmi_get_clk(),
- Add support for gateable fck parents,
- Assume all SCMI clock IDs are stable, and drop support for handling
different ID spaces,
- Include initial mapping table for SCP FW SDKv4.31.0 and later,
- Propagate all reset failures, as SCP FW SDKv4.28.0 which restricted
HSCIF0 reset is no longer supported,
- Drop renesas,scmi-firmware support, just find an arm,scmi node
instead,
- Fall back to the non-SCMI defaults when any of SCMI power domain,
clock, or reset support is disabled,
v3:
- Rebase to v7.3-rc1,
- Consistently use /* sentinel */ for zero sentinels,
- s/genpd_get_from_provider/of_genpd_get_from_provider/,
- s/firmware/renesas,scmi-firmware/,
- Replace reset_controller_get_provider() by
reset_control_get_from_provider_exclusive(),
- Call reset_control_*() APIs instead of internal reset_control_ops
methods,
- Rename struct mod_map to struct mod_map_in,
- Add struct mod_map, which also contains a reset_control pointer,
- Add fill_mod_map() to obtain all SCMI reset_control structures
up-front, as one can no longer use lazy look-up,
- Prefix DEFINE_MDLC_RESET_WRAPPER() parameters with underscores,
- Reword,
v2:
- Spin off from "pmdomain: renesas: Add R-Car X5H MDLC SCMI remapping
driver".
- Propagate error codes from scmi_get_base_info() now it can return
-EPROBE_DEFER,
- Improve error message on scmi_get_base_info() failure,
- Check if SCMI firmware node is available,
- Replace scmi_power_np and scmi_reset_np members of struct
r8a78000_mdlc_priv by local variables in r8a78000_mdlc_probe(), and
pass the former to fill_power_map(),
- Move check for supported firmware version up,
- Fall back to the default support when SCMI support or firmware is
not available, or when the detected SCMI firmware version is not
supported by the driver,
- Drop mapping tables.
---
drivers/pmdomain/renesas/r8a78000-mdlc.c | 410 +++++++++++++++++++++--
1 file changed, 386 insertions(+), 24 deletions(-)
diff --git a/drivers/pmdomain/renesas/r8a78000-mdlc.c b/drivers/pmdomain/renesas/r8a78000-mdlc.c
index 1fd71bb3da776eb1..af36d8229030f6c0 100644
--- a/drivers/pmdomain/renesas/r8a78000-mdlc.c
+++ b/drivers/pmdomain/renesas/r8a78000-mdlc.c
@@ -5,24 +5,55 @@
* Copyright (C) 2026 Glider bv
*/
+#include <linux/cleanup.h>
+#include <linux/clk.h>
#include <linux/dev_printk.h>
#include <linux/device-id/of.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_clock.h>
#include <linux/pm_domain.h>
#include <linux/reset-controller.h>
+#include <linux/reset.h>
+#include <linux/scmi_protocol.h>
#include <linux/slab.h>
#include <dt-bindings/power/renesas,r8a78000-mdlc.h>
+struct power_map_in {
+ int hw_id; /* Hardware power domain ID or -1 sentinel */
+ u32 fw_id; /* SCMI firmware power domain ID */
+};
+
+struct power_map {
+ int hw_id; /* Hardware power domain ID or -1 sentinel */
+ u32 fw_id; /* SCMI firmware power domain ID */
+ struct generic_pm_domain *genpd;
+};
+
+struct mod_map_in {
+ int hw_id; /* Hardware module ID or -1 sentinel */
+ u32 fw_id; /* SCMI clock and reset IDs are identical */
+ s32 fw_fck_parent; /* SCMI functional clock parent ID or -1 */
+};
+
struct mod_map {
int hw_id; /* Hardware module ID or -1 sentinel */
+ u32 fw_id; /* SCMI clock and reset IDs are identical */
+ s32 fw_fck_parent; /* SCMI functional clock parent ID or -1 */
+ struct reset_control *rstc;
};
struct mdlc_info {
u32 base;
- const struct mod_map *mod_map;
+ const struct power_map_in *power_map;
+ const struct mod_map_in *mod_map;
+};
+
+struct mdlc_data {
+ const struct mdlc_info *default_info;
+ const struct mdlc_info *fw_info;
};
/**
@@ -33,7 +64,9 @@ struct mdlc_info {
* @rcdev: Reset controller entity
* @dev: MDLC device
* @np: Device node in DT representing the MDLC
- * @mod_map: Mapping from hardware module IDs
+ * @scmi_clk_np: Device node in DT for the SCMI firmware clock protocol
+ * @power_map: Mapping from hardware power domain IDs to SCMI power domains
+ * @mod_map: Mapping from hardware module IDs to SCMI clocks and resets
*/
struct r8a78000_mdlc_priv {
struct hlist_node link;
@@ -41,6 +74,8 @@ struct r8a78000_mdlc_priv {
struct reset_controller_dev rcdev;
struct device *dev;
struct device_node *np;
+ struct device_node *scmi_clk_np;
+ const struct power_map *power_map;
const struct mod_map *mod_map;
};
@@ -49,12 +84,28 @@ static struct generic_pm_domain *r8a78000_genpd_always_on;
static HLIST_HEAD(r8a78000_mdlc_list);
static DEFINE_MUTEX(r8a78000_mdlc_lock); /* protects the list above */
+static const struct power_map *power_map_find(const struct power_map *map,
+ u32 id)
+{
+ if (!map)
+ return NULL;
+
+ for (; map->hw_id >= 0; map++) {
+ if (map->hw_id == id)
+ return map;
+ }
+
+ return NULL;
+}
+
static struct generic_pm_domain *r8a78000_genpd_xlate(
const struct of_phandle_args *spec, void *data)
{
struct r8a78000_mdlc_priv *priv = container_of(data,
struct r8a78000_mdlc_priv, genpd_data);
+ struct generic_pm_domain *genpd;
struct device *dev = priv->dev;
+ const struct power_map *map;
u32 id;
if (spec->args_count != 2)
@@ -69,9 +120,18 @@ static struct generic_pm_domain *r8a78000_genpd_xlate(
return r8a78000_genpd_always_on;
}
- /* For now only always-on domains are supported */
- dev_err(dev, "Unknown power domain 0x%x\n", id);
- return ERR_PTR(-ENOENT);
+ map = power_map_find(priv->power_map, id);
+ if (!map) {
+ dev_err(dev, "Unknown power domain 0x%x\n", id);
+ return ERR_PTR(-ENOENT);
+ }
+
+ dev_dbg(dev, "Mapping HW power domain 0x%x to SCMI power domain %u\n",
+ id, map->fw_id);
+
+ genpd = map->genpd;
+
+ return genpd;
}
#define rcdev_to_priv(_rcdev) \
@@ -109,18 +169,22 @@ static int r8a78000_mdlc_reset_xlate(struct reset_controller_dev *rcdev,
return -ENOENT;
}
- dev_dbg(dev, "Ignoring HW reset 0x%x\n", id);
- return id;
+ if (!map->rstc)
+ dev_dbg(dev, "Ignoring HW reset 0x%x\n", id);
+ else
+ dev_dbg(dev, "Mapping HW reset 0x%x to SCMI reset %u\n", id,
+ map->fw_id);
+
+ return map - priv->mod_map;
}
-#define DEFINE_MDLC_RESET_WRAPPER(op) \
- static int r8a78000_mdlc_ ## op(struct reset_controller_dev *rcdev, \
- unsigned long id) \
- { \
- struct r8a78000_mdlc_priv *priv = rcdev_to_priv(rcdev); \
- \
- dev_dbg(priv->dev, "%s: Ignoring\n", __func__); \
- return 0; \
+#define DEFINE_MDLC_RESET_WRAPPER(_op) \
+ static int r8a78000_mdlc_ ## _op(struct reset_controller_dev *rcdev, \
+ unsigned long id) \
+ { \
+ struct r8a78000_mdlc_priv *priv = rcdev_to_priv(rcdev); \
+ \
+ return reset_control_ ## _op(priv->mod_map[id].rstc); \
}
DEFINE_MDLC_RESET_WRAPPER(reset)
@@ -135,11 +199,45 @@ static const struct reset_control_ops r8a78000_mdlc_reset_ops = {
.status = r8a78000_mdlc_status,
};
+static struct device_node *scmi_find_proto(struct device_node *scmi, u32 proto)
+{
+ for_each_available_child_of_node_scoped(scmi, child) {
+ u32 reg;
+
+ if (of_property_read_u32(child, "reg", ®))
+ continue;
+
+ if (reg == proto)
+ return_ptr(child);
+ }
+
+ return NULL;
+}
+
+static struct clk *scmi_get_clk(struct device *dev, struct device_node *np,
+ u32 id)
+{
+ struct of_phandle_args scmi_spec = {
+ .np = np,
+ .args_count = 1,
+ .args[0] = id,
+ };
+ struct clk *clk = of_clk_get_from_provider(&scmi_spec);
+
+ if (IS_ERR(clk))
+ dev_err(dev, "Cannot get SCMI clock %u: %pe\n", id, clk);
+ else
+ dev_dbg(dev, "SCMI clock %u is %pC\n", id, clk);
+
+ return clk;
+}
+
static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
struct device *dev)
{
struct device_node *np = dev->of_node;
struct r8a78000_mdlc_priv *priv;
+ struct clk *clk, *parent = NULL;
struct of_phandle_args pd_spec;
const struct mod_map *map;
unsigned int id;
@@ -158,9 +256,10 @@ static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
}
if (!priv) {
- dev_err(dev, "%s: MDLC %pOF not found\n", __func__, pd_spec.np);
+ dev_warn(dev, "%s: Ignoring non-MDLC %pOF\n", __func__,
+ pd_spec.np);
of_node_put(pd_spec.np);
- return -ENODEV;
+ return 0;
}
id = pd_spec.args[1];
@@ -172,8 +271,158 @@ static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
return -ENOENT;
}
- dev_dbg(dev, "Ignoring HW module 0x%x\n", id);
+ if (!priv->scmi_clk_np) {
+ dev_dbg(dev, "Ignoring HW module 0x%x\n", id);
+ return 0;
+ }
+
+ dev_dbg(dev, "Mapping HW module 0x%x to SCMI clock %u fck parent %d\n",
+ id, map->fw_id, map->fw_fck_parent);
+
+ clk = scmi_get_clk(dev, priv->scmi_clk_np, map->fw_id);
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ if (!clk) {
+ /* Ignore missing SCMI module clocks */
+ return 0;
+ }
+
+ if (map->fw_fck_parent >= 0) {
+ parent = scmi_get_clk(dev, priv->scmi_clk_np,
+ map->fw_fck_parent);
+ if (IS_ERR(parent)) {
+ ret = PTR_ERR(parent);
+ goto fail_put_clk;
+ }
+ }
+
+ ret = pm_clk_create(dev);
+ if (ret)
+ goto fail_put_parent;
+
+ if (parent)
+ ret = pm_clk_add_clk(dev, parent);
+ if (!ret)
+ ret = pm_clk_add_clk(dev, clk);
+ if (ret)
+ goto fail_destroy;
+
return 0;
+
+fail_destroy:
+ pm_clk_destroy(dev);
+fail_put_parent:
+ clk_put(parent);
+fail_put_clk:
+ clk_put(clk);
+ return ret;
+}
+
+static void r8a78000_mdlc_detach_dev(struct generic_pm_domain *domain,
+ struct device *dev)
+{
+ if (!pm_clk_no_clocks(dev))
+ pm_clk_destroy(dev);
+}
+
+static const struct power_map *fill_power_map(struct r8a78000_mdlc_priv *priv,
+ const struct power_map_in *map_in,
+ struct device_node *scmi_power_np)
+{
+ struct of_phandle_args scmi_spec;
+ struct generic_pm_domain *genpd;
+ struct device *dev = priv->dev;
+ struct power_map *map;
+ unsigned int i;
+
+ if (!map_in)
+ return NULL;
+
+ for (i = 0; map_in[i].hw_id >= 0; i++) { }
+
+ map = devm_kcalloc(dev, i + 1, sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; ; i++) {
+ map[i].hw_id = map_in[i].hw_id;
+ if (map[i].hw_id < 0)
+ break;
+
+ map[i].fw_id = map_in[i].fw_id;
+
+ scmi_spec.np = scmi_power_np;
+ scmi_spec.args_count = 1;
+ scmi_spec.args[0] = map[i].fw_id;
+
+ genpd = of_genpd_get_from_provider(&scmi_spec);
+ if (IS_ERR(genpd))
+ return dev_err_cast_probe(dev, genpd,
+ "Failed to get SCMI power domain %u\n",
+ map[i].fw_id);
+
+ dev_dbg(dev, "SCMI power domain %u is %s\n", map[i].fw_id,
+ genpd->name);
+
+ map[i].genpd = genpd;
+
+ /* Hook up clock domain support */
+ genpd->attach_dev = r8a78000_mdlc_attach_dev;
+ genpd->detach_dev = r8a78000_mdlc_detach_dev;
+ /* Setting flags this late has no impact, but does not hurt */
+ genpd->flags |= GENPD_FLAG_PM_CLK;
+ genpd->dev_ops.stop = pm_clk_suspend;
+ genpd->dev_ops.start = pm_clk_resume;
+ }
+
+ return map;
+}
+
+static const struct mod_map *fill_mod_map(struct r8a78000_mdlc_priv *priv,
+ const struct mod_map_in *map_in,
+ struct fwnode_handle *scmi_reset_fwnode)
+{
+ struct fwnode_reference_args scmi_spec;
+ struct device *dev = priv->dev;
+ struct reset_control *rstc;
+ struct mod_map *map;
+ unsigned int i;
+
+ if (!map_in)
+ return NULL;
+
+ for (i = 0; map_in[i].hw_id >= 0; i++) { }
+
+ map = devm_kcalloc(dev, i + 1, sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; ; i++) {
+ map[i].hw_id = map_in[i].hw_id;
+ if (map[i].hw_id < 0)
+ break;
+
+ map[i].fw_id = map_in[i].fw_id;
+ map[i].fw_fck_parent = map_in[i].fw_fck_parent;
+
+ if (!scmi_reset_fwnode)
+ continue;
+
+ scmi_spec.fwnode = scmi_reset_fwnode;
+ scmi_spec.nargs = 1;
+ scmi_spec.args[0] = map[i].fw_id;
+
+ rstc = reset_control_get_from_provider_exclusive(&scmi_spec);
+ if (IS_ERR(rstc))
+ return dev_err_cast_probe(dev, rstc,
+ "Failed to get SCMI reset %u\n",
+ map[i].fw_id);
+
+ map[i].rstc = rstc;
+ }
+
+ return map;
}
static void r8a78000_mdlc_unlink(void *data)
@@ -207,6 +456,8 @@ static int r8a78000_genpd_always_on_singleton(struct device *dev)
genpd->name = "always-on";
genpd->attach_dev = r8a78000_mdlc_attach_dev;
+ genpd->detach_dev = r8a78000_mdlc_detach_dev;
+ genpd->flags |= GENPD_FLAG_PM_CLK;
ret = pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
if (ret) {
@@ -221,8 +472,13 @@ static int r8a78000_genpd_always_on_singleton(struct device *dev)
static int r8a78000_mdlc_probe(struct platform_device *pdev)
{
+ struct device_node *scmi_reset_np __free(device_node) = NULL;
+ struct device_node *scmi_power_np __free(device_node) = NULL;
+ struct device_node *scmi __free(device_node) = NULL;
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
+ const struct mdlc_data *mdlc_data;
+ struct device_node *scmi_clk_np;
struct r8a78000_mdlc_priv *priv;
const struct mdlc_info *info;
struct resource *res;
@@ -232,10 +488,12 @@ static int r8a78000_mdlc_probe(struct platform_device *pdev)
if (ret)
return ret;
- info = of_device_get_match_data(dev);
- if (!info)
+ mdlc_data = of_device_get_match_data(dev);
+ if (!mdlc_data)
return -ENODEV;
+ info = mdlc_data->default_info;
+
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -243,6 +501,47 @@ static int r8a78000_mdlc_probe(struct platform_device *pdev)
priv->dev = dev;
priv->np = np;
+ if (!IS_ENABLED(CONFIG_ARM_SCMI_POWER_DOMAIN) ||
+ !IS_ENABLED(CONFIG_COMMON_CLK_SCMI) ||
+ !IS_ENABLED(CONFIG_RESET_SCMI)) {
+ dev_dbg(dev, "SCMI pmdomain/clock/reset support is disabled\n");
+ goto fallback;
+ }
+
+ scmi = of_find_compatible_node(NULL, NULL, "arm,scmi");
+ if (!scmi) {
+ dev_dbg(dev, "Cannot find SCMI firmware node\n");
+ goto fallback;
+ }
+
+ if (!of_device_is_available(scmi)) {
+ dev_dbg(dev, "SCMI firmware node is not available\n");
+ goto fallback;
+ }
+
+ scmi_power_np = scmi_find_proto(scmi, SCMI_PROTOCOL_POWER);
+ if (!scmi_power_np) {
+ dev_dbg(dev,
+ "Cannot find SCMI power domain management protocol\n");
+ goto fallback;
+ }
+
+ scmi_clk_np = scmi_find_proto(scmi, SCMI_PROTOCOL_CLOCK);
+ if (!scmi_clk_np) {
+ dev_dbg(dev, "Cannot find SCMI clock management protocol\n");
+ goto fallback;
+ }
+
+ scmi_reset_np = scmi_find_proto(scmi, SCMI_PROTOCOL_RESET);
+ if (!scmi_reset_np) {
+ dev_dbg(dev, "Cannot find SCMI reset management protocol\n");
+ goto fallback;
+ }
+
+ priv->scmi_clk_np = scmi_clk_np;
+ info = mdlc_data->fw_info;
+
+fallback:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return -ENODEV;
@@ -257,7 +556,22 @@ static int r8a78000_mdlc_probe(struct platform_device *pdev)
return -ENODEV;
}
- priv->mod_map = info->mod_map;
+ /*
+ * We cannot do lazy look-up in r8a78000_genpd_xlate(), as that
+ * function is called with of_genpd_mutex already held.
+ */
+ priv->power_map = fill_power_map(priv, info->power_map, scmi_power_np);
+ if (IS_ERR(priv->power_map))
+ return PTR_ERR(priv->power_map);
+
+ /*
+ * We cannot do lazy look-up in r8a78000_mdlc_reset_xlate(), as that
+ * function is called with reset_list_mutex already held.
+ */
+ priv->mod_map = fill_mod_map(priv, info->mod_map,
+ of_fwnode_handle(scmi_reset_np));
+ if (IS_ERR(priv->mod_map))
+ return PTR_ERR(priv->mod_map);
scoped_guard(mutex, &r8a78000_mdlc_lock) {
hlist_add_head(&priv->link, &r8a78000_mdlc_list);
@@ -292,8 +606,8 @@ static int r8a78000_mdlc_probe(struct platform_device *pdev)
return 0;
}
-static const struct mod_map r8a78000_mdlc_perw_mod_default[] = {
- { 0x54 }, /* HSCIF0 */
+static const struct mod_map_in r8a78000_mdlc_perw_mod_default[] = {
+ { 0x54, 0, -1 }, /* HSCIF0 */
{ -1 }
};
@@ -305,10 +619,58 @@ static const struct mdlc_info r8a78000_mdlc_default[] = {
{ /* sentinel */ }
};
+static const struct mod_map_in r8a78000_mdlc_perw_mod_fw[] = {
+ { 0x40, 205, -1 }, /* SCIF0 */
+ { 0x41, 206, -1 }, /* SCIF1 */
+ { 0x42, 207, -1 }, /* SCIF3 */
+ { 0x43, 208, -1 }, /* SCIF4 */
+ { 0x44, 209, -1 }, /* I2C1 */
+ { 0x45, 210, -1 }, /* I2C2 */
+ { 0x46, 211, -1 }, /* I2C3 */
+ { 0x47, 212, -1 }, /* I2C4 */
+ { 0x48, 213, -1 }, /* I2C5 */
+ { 0x49, 214, -1 }, /* I2C6 */
+ { 0x4a, 215, -1 }, /* I2C7 */
+ { 0x4b, 216, -1 }, /* I2C8 */
+ { 0x4f, 220, 1667 }, /* MSI4 / MSOCK_PERW_BUS */
+ { 0x50, 221, 1667 }, /* MSI5 / MSOCK_PERW_BUS */
+ { 0x51, 222, 1667 }, /* MSI6 / MSOCK_PERW_BUS */
+ { 0x52, 223, 1667 }, /* MSI7 / MSOCK_PERW_BUS */
+ { 0x54, 224, -1 }, /* HSCIF0 */
+ { 0x55, 225, -1 }, /* HSCIF1 */
+ { 0x56, 226, -1 }, /* HSCIF2 */
+ { 0x57, 227, -1 }, /* HSCIF3 */
+ { -1 }
+};
+
+static const struct mod_map_in r8a78000_mdlc_scp_mod_fw[] = {
+ { 0x50, 776, 1459 }, /* MSI0 / MSO_SCP_MAIN */
+ { 0x51, 777, 1459 }, /* MSI1 / MSO_SCP_MAIN */
+ { 0x52, 778, 1459 }, /* MSI2 / MSO_SCP_MAIN */
+ { 0x53, 779, 1459 }, /* MSI3 / MSO_SCP_MAIN */
+ { -1 }
+};
+
+static const struct mdlc_info r8a78000_mdlc_fw[] = {
+ {
+ .base = 0xc05d0000 /* mdlc_perw */,
+ .mod_map = r8a78000_mdlc_perw_mod_fw,
+ }, {
+ .base = 0xc1330000 /* mdlc_scp */,
+ .mod_map = r8a78000_mdlc_scp_mod_fw,
+ },
+ { /* sentinel */ }
+};
+
+static const struct mdlc_data r8a78000_mdlc_data = {
+ .default_info = r8a78000_mdlc_default,
+ .fw_info = r8a78000_mdlc_fw,
+};
+
static const struct of_device_id r8a78000_mdlc_match[] = {
{
.compatible = "renesas,r8a78000-mdlc",
- .data = &r8a78000_mdlc_default,
+ .data = &r8a78000_mdlc_data,
},
{ /* sentinel */ }
};
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 7/8] pmdomain: renesas: r8a78000: Add SCMI remapping support
2026-10-16 15:55 ` [PATCH v4 7/8] pmdomain: renesas: r8a78000: Add SCMI remapping support Geert Uytterhoeven
@ 2026-10-16 16:15 ` sashiko-bot
0 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-10-16 16:15 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: devicetree, linux-clk, robh, conor+dt, wsa+renesas
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] clk: double free of parent clock reference in error path
- [High] pmdomain: renesas: mutation of SCMI genpd and hardcoded index 0 in attach_dev
- [Medium] of: memory leak of scmi_clk_np reference
- [Medium] reset: memory leak of exclusive reset controls on error and unbind
- [Medium] pmdomain: renesas: out-of-bounds stack read in r8a78000_mdlc_attach_dev()
- [Low] cleanup: mixed goto and scope-based cleanups in attach_dev
--
commit a125fd15fada610e74f2b15cf0cafd68482a5100
Author: Geert Uytterhoeven <geert+renesas@glider.be>
pmdomain: renesas: r8a78000: Add SCMI remapping support
Extend the Module Controller driver for R-Car X5H with SCMI support, to
remap hardware power domain, module, and reset IDs to abstractions
offered by the SCMI firmware. This avoids putting SCMI IDs in DT and
supports fallback to non-SCMI defaults when needed.
> --- a/drivers/pmdomain/renesas/r8a78000-mdlc.c
> +++ b/drivers/pmdomain/renesas/r8a78000-mdlc.c
[ ... ]
> +static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
> + struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct r8a78000_mdlc_priv *priv;
> + struct clk *clk, *parent = NULL;
> + struct of_phandle_args pd_spec;
[Severity: Medium]
Is pd_spec guaranteed to be fully initialized?
If the device tree node has a malformed #power-domain-cells < 2, could we
end up with uninitialized stack memory in pd_spec.args[1] that is later
used as a module ID?
> + const struct mod_map *map;
> + unsigned int id;
> + int ret;
> +
> + ret = of_parse_phandle_with_args(np, "power-domains",
> + "#power-domain-cells", 0, &pd_spec);
[Severity: High]
Is it intended to unconditionally parse index 0 here, even if the domain
being attached is at a higher index?
If a non-MDLC device uses multiple domains and the MDLC domain is not at
index 0, could this look up the wrong provider or silently fail to attach
the correct clocks?
> + if (ret < 0)
> + return ret;
> +
> + scoped_guard(mutex, &r8a78000_mdlc_lock) {
[Severity: Low]
Does this mix of scope-based cleanup and goto violate the cleanup subsystem
guidelines?
This function uses scoped_guard() here, but also manually manages cleanup
using goto-based error handling labels later in the function (e.g.,
goto fail_put_parent). The guidelines suggest converting either all or none
of the resources in a given routine to avoid LIFO ordering bugs.
> + hlist_for_each_entry(priv, &r8a78000_mdlc_list, link) {
> + if (priv->np == pd_spec.np)
> + break;
> + }
> + }
[ ... ]
> + if (parent)
> + ret = pm_clk_add_clk(dev, parent);
> + if (!ret)
> + ret = pm_clk_add_clk(dev, clk);
> + if (ret)
> + goto fail_destroy;
> +
> + return 0;
> +
> +fail_destroy:
> + pm_clk_destroy(dev);
> +fail_put_parent:
> + clk_put(parent);
[Severity: High]
Will this result in a double free of the parent clock?
pm_clk_destroy() destroys all clocks attached to the device, which includes
cleanly removing the successfully added parent clock (and internally calling
clk_put() on it). Falling through to fail_put_parent seems to call
clk_put(parent) a second time.
> +fail_put_clk:
> + clk_put(clk);
> + return ret;
> +}
[ ... ]
> +static const struct power_map *fill_power_map(struct r8a78000_mdlc_priv *priv,
> + const struct power_map_in *map_in,
> + struct device_node *scmi_power_np)
> +{
[ ... ]
> + genpd = of_genpd_get_from_provider(&scmi_spec);
> + if (IS_ERR(genpd))
> + return dev_err_cast_probe(dev, genpd,
> + "Failed to get SCMI power domain %u\n",
> + map[i].fw_id);
> +
> + dev_dbg(dev, "SCMI power domain %u is %s\n", map[i].fw_id,
> + genpd->name);
> +
> + map[i].genpd = genpd;
> +
> + /* Hook up clock domain support */
> + genpd->attach_dev = r8a78000_mdlc_attach_dev;
> + genpd->detach_dev = r8a78000_mdlc_detach_dev;
> + /* Setting flags this late has no impact, but does not hurt */
> + genpd->flags |= GENPD_FLAG_PM_CLK;
> + genpd->dev_ops.stop = pm_clk_suspend;
> + genpd->dev_ops.start = pm_clk_resume;
[Severity: High]
Is it safe to mutate the genpd struct of the SCMI provider directly?
Modifying genpd->attach_dev and genpd->dev_ops here changes the behavior
for all consumers of the shared SCMI domain. Could this cause
cross-subsystem state corruption or spurious warnings when non-MDLC devices
use this SCMI domain?
> + }
> +
> + return map;
> +}
> +
> +static const struct mod_map *fill_mod_map(struct r8a78000_mdlc_priv *priv,
> + const struct mod_map_in *map_in,
> + struct fwnode_handle *scmi_reset_fwnode)
> +{
[ ... ]
> + scmi_spec.args[0] = map[i].fw_id;
> +
> + rstc = reset_control_get_from_provider_exclusive(&scmi_spec);
> + if (IS_ERR(rstc))
> + return dev_err_cast_probe(dev, rstc,
> + "Failed to get SCMI reset %u\n",
> + map[i].fw_id);
> +
> + map[i].rstc = rstc;
[Severity: Medium]
Does this error path leak the previously acquired exclusive reset controls?
If an allocation fails mid-loop, IS_ERR(rstc) evaluates to true and the
function immediately returns an error pointer, leaving no mechanism to free
map entries from earlier iterations.
Furthermore, the driver does not appear to iterate through this map and
release these controls during driver unbind or teardown.
> + }
> +
> + return map;
> }
>
> static void r8a78000_mdlc_unlink(void *data)
> {
> struct r8a78000_mdlc_priv *priv = data;
>
> scoped_guard(mutex, &r8a78000_mdlc_lock) {
> hlist_del(&priv->link);
> }
> }
[Severity: Medium]
Does this teardown sequence leak the priv->scmi_clk_np reference?
The reference was previously acquired using scmi_find_proto() with
SCMI_PROTOCOL_CLOCK, which returns an incremented reference count, but
of_node_put() does not appear to be called during unbind.
[ ... ]
> static int r8a78000_mdlc_probe(struct platform_device *pdev)
> {
> + struct device_node *scmi_reset_np __free(device_node) = NULL;
> + struct device_node *scmi_power_np __free(device_node) = NULL;
> + struct device_node *scmi __free(device_node) = NULL;
[ ... ]
> + scmi_clk_np = scmi_find_proto(scmi, SCMI_PROTOCOL_CLOCK);
> + if (!scmi_clk_np) {
> + dev_dbg(dev, "Cannot find SCMI clock management protocol\n");
> + goto fallback;
> + }
> +
> + scmi_reset_np = scmi_find_proto(scmi, SCMI_PROTOCOL_RESET);
> + if (!scmi_reset_np) {
> + dev_dbg(dev, "Cannot find SCMI reset management protocol\n");
> + goto fallback;
> + }
> +
> + priv->scmi_clk_np = scmi_clk_np;
> + info = mdlc_data->fw_info;
> +
> +fallback:
[Severity: Medium]
If the jump to the fallback label occurs after successfully assigning
scmi_clk_np (e.g., when scmi_reset_np is missing), does this leak the
scmi_clk_np reference?
Since scmi_clk_np is not annotated with __free(device_node) like
scmi_power_np and scmi_reset_np, it requires explicit cleanup on error
paths.
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792163045.git.geert+renesas@glider.be?part=7
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 8/8] arm64: dts: renesas: ironhide: Enable SCMI devpd, sys, clock, and reset
2026-10-16 15:55 [PATCH v4 0/8] R-Car X5H Ironhide CPG/MDLC SCMI remapping support Geert Uytterhoeven
` (6 preceding siblings ...)
2026-10-16 15:55 ` [PATCH v4 7/8] pmdomain: renesas: r8a78000: Add SCMI remapping support Geert Uytterhoeven
@ 2026-10-16 15:55 ` Geert Uytterhoeven
7 siblings, 0 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2026-10-16 15:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Stephen Boyd,
Brian Masney, Jerome Brunet, Ulf Hansson, Philipp Zabel,
Rafael J . Wysocki, Marek Vasut, Bartosz Golaszewski,
Konrad Dybcio, Kevin Hilman, Vinod Koul, Wolfram Sang,
Kuninori Morimoto, Laurent Pinchart, Clément Le Goffic,
Sudeep Holla, Cristian Marussi, arm-scmi, linux-arm-kernel
Cc: devicetree, linux-clk, linux-pm, linux-renesas-soc,
Geert Uytterhoeven
SCP FW SDKv4.28.0 and later for Renesas R-Car X5H Ironhide not only
implement the SCMI base protocol, but also the SCMI power domain, system
power, clock, and reset domain management protocols.
Enable support for the latter by adding the corresponding SCMI protocol
subnodes.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4:
- No changes,
v3:
- No changes,
v2:
- Fix misindentations,
- Add scmi_clk node, as it is no longer added in earlier commits.
---
.../boot/dts/renesas/r8a78000-ironhide.dts | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a78000-ironhide.dts b/arch/arm64/boot/dts/renesas/r8a78000-ironhide.dts
index b2d6b06c63881896..ade8bcd3a0e5ee8b 100644
--- a/arch/arm64/boot/dts/renesas/r8a78000-ironhide.dts
+++ b/arch/arm64/boot/dts/renesas/r8a78000-ironhide.dts
@@ -31,6 +31,25 @@ scmi: scmi {
arm,no-completion-irq;
#address-cells = <1>;
#size-cells = <0>;
+
+ scmi_devpd: protocol@11 {
+ reg = <0x11>;
+ #power-domain-cells = <1>;
+ };
+
+ scmi_sys: protocol@12 {
+ reg = <0x12>;
+ };
+
+ scmi_clk: protocol@14 {
+ reg = <0x14>;
+ #clock-cells = <1>;
+ };
+
+ scmi_reset: protocol@16 {
+ reg = <0x16>;
+ #reset-cells = <1>;
+ };
};
};
--
2.43.0
^ permalink raw reply related [flat|nested] 16+ messages in thread