[PATCH] clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()

Geert Uytterhoeven posted 1 patch 2 weeks, 6 days ago
drivers/clk/clk-renesas-pcie.c | 39 ++++++++++++++++------------------
1 file changed, 18 insertions(+), 21 deletions(-)
[PATCH] clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()
Posted by Geert Uytterhoeven 2 weeks, 6 days ago
rs9_of_clk_get() does not validate the clock index in the passed
DT clock specifier.  If DT specifies an incorrect and out-of-range
index, this may access memory beyond the end of the fixed-size clk_dif[]
array.

Instead of fixing this by adding a range check to rs9_of_clk_get(),
convert the driver to use the of_clk_hw_onecell_get() helper, which
already contains such a check.  Embedding a clk_hw_onecell_data
structure in the rs9_driver_data structure has the added benefit that
the clock array always has the correct size, and thus can no longer
become out of sync when adding support for new rs9 variants.

Fixes: 892e0ddea1aa6f70 ("clk: rs9: Add Renesas 9-series PCIe clock generator driver")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
While this patch applies on top of "[PATCH v2] clk: rs9: Reserve 8
struct clk_hw slots for for 9FGV0841", it can be applied or backported
without, by ignoring the change from "clk_dif[4]" to "clk_dif[8]".

[1] https://patch.msgid.link/20260119150242.29444-1-marek.vasut+renesas@mailbox.org
---
 drivers/clk/clk-renesas-pcie.c | 39 ++++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index aa108df12e44fb9f..2b8b6b82250360d5 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -64,10 +64,11 @@ struct rs9_driver_data {
 	struct i2c_client	*client;
 	struct regmap		*regmap;
 	const struct rs9_chip_info *chip_info;
-	struct clk_hw		*clk_dif[8];
 	u8			pll_amplitude;
 	u8			pll_ssc;
 	u8			clk_dif_sr;
+	/* must be last */
+	struct clk_hw_onecell_data onecell;
 };
 
 /*
@@ -271,32 +272,28 @@ static void rs9_update_config(struct rs9_driver_data *rs9)
 	}
 }
 
-static struct clk_hw *
-rs9_of_clk_get(struct of_phandle_args *clkspec, void *data)
-{
-	struct rs9_driver_data *rs9 = data;
-	unsigned int idx = clkspec->args[0];
-
-	return rs9->clk_dif[idx];
-}
-
 static int rs9_probe(struct i2c_client *client)
 {
+	const struct rs9_chip_info *chip_info;
 	unsigned char name[5] = "DIF0";
 	struct rs9_driver_data *rs9;
 	unsigned int vid, did;
 	struct clk_hw *hw;
 	int i, ret;
 
-	rs9 = devm_kzalloc(&client->dev, sizeof(*rs9), GFP_KERNEL);
+	chip_info = i2c_get_match_data(client);
+	if (!chip_info)
+		return -EINVAL;
+
+	rs9 = devm_kzalloc(&client->dev, struct_size(rs9, onecell.hws,
+			   chip_info->num_clks), GFP_KERNEL);
 	if (!rs9)
 		return -ENOMEM;
 
 	i2c_set_clientdata(client, rs9);
 	rs9->client = client;
-	rs9->chip_info = i2c_get_match_data(client);
-	if (!rs9->chip_info)
-		return -EINVAL;
+	rs9->chip_info = chip_info;
+	rs9->onecell.num = chip_info->num_clks;
 
 	/* Fetch common configuration from DT (if specified) */
 	ret = rs9_get_common_config(rs9);
@@ -304,7 +301,7 @@ static int rs9_probe(struct i2c_client *client)
 		return ret;
 
 	/* Fetch DIFx output configuration from DT (if specified) */
-	for (i = 0; i < rs9->chip_info->num_clks; i++) {
+	for (i = 0; i < rs9->onecell.num; i++) {
 		ret = rs9_get_output_config(rs9, i);
 		if (ret)
 			return ret;
@@ -330,24 +327,24 @@ static int rs9_probe(struct i2c_client *client)
 		return ret;
 
 	vid &= RS9_REG_VID_MASK;
-	if (vid != RS9_REG_VID_IDT || did != rs9->chip_info->did)
+	if (vid != RS9_REG_VID_IDT || did != chip_info->did)
 		return dev_err_probe(&client->dev, -ENODEV,
 				     "Incorrect VID/DID: %#02x, %#02x. Expected %#02x, %#02x\n",
-				     vid, did, RS9_REG_VID_IDT,
-				     rs9->chip_info->did);
+				     vid, did, RS9_REG_VID_IDT, chip_info->did);
 
 	/* Register clock */
-	for (i = 0; i < rs9->chip_info->num_clks; i++) {
+	for (i = 0; i < rs9->onecell.num; i++) {
 		snprintf(name, 5, "DIF%d", i);
 		hw = devm_clk_hw_register_fixed_factor_index(&client->dev, name,
 						    0, 0, 4, 1);
 		if (IS_ERR(hw))
 			return PTR_ERR(hw);
 
-		rs9->clk_dif[i] = hw;
+		rs9->onecell.hws[i] = hw;
 	}
 
-	ret = devm_of_clk_add_hw_provider(&client->dev, rs9_of_clk_get, rs9);
+	ret = devm_of_clk_add_hw_provider(&client->dev, of_clk_hw_onecell_get,
+					  &rs9->onecell);
 	if (!ret)
 		rs9_update_config(rs9);
 
-- 
2.43.0
Re: [PATCH] clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()
Posted by Marek Vasut 2 weeks, 6 days ago
On 1/19/26 4:23 PM, Geert Uytterhoeven wrote:
> rs9_of_clk_get() does not validate the clock index in the passed
> DT clock specifier.  If DT specifies an incorrect and out-of-range
> index, this may access memory beyond the end of the fixed-size clk_dif[]
> array.
> 
> Instead of fixing this by adding a range check to rs9_of_clk_get(),
> convert the driver to use the of_clk_hw_onecell_get() helper, which
> already contains such a check.  Embedding a clk_hw_onecell_data
> structure in the rs9_driver_data structure has the added benefit that
> the clock array always has the correct size, and thus can no longer
> become out of sync when adding support for new rs9 variants.
> 
> Fixes: 892e0ddea1aa6f70 ("clk: rs9: Add Renesas 9-series PCIe clock generator driver")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> While this patch applies on top of "[PATCH v2] clk: rs9: Reserve 8
> struct clk_hw slots for for 9FGV0841", it can be applied or backported
> without, by ignoring the change from "clk_dif[4]" to "clk_dif[8]".

Since the 9FGV0841 is the biggest part in the 9FGV series, the one-liner 
fix I posted is better suited as a stable backport. This rework 
shouldn't have the Fixes tag, since it is only that, a rework.

With that fixed:

Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Re: [PATCH] clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()
Posted by Geert Uytterhoeven 2 weeks, 5 days ago
Hi Marek,

On Mon, 19 Jan 2026 at 20:36, Marek Vasut <marek.vasut@mailbox.org> wrote:
> On 1/19/26 4:23 PM, Geert Uytterhoeven wrote:
> > rs9_of_clk_get() does not validate the clock index in the passed
> > DT clock specifier.  If DT specifies an incorrect and out-of-range
> > index, this may access memory beyond the end of the fixed-size clk_dif[]
> > array.
> >
> > Instead of fixing this by adding a range check to rs9_of_clk_get(),
> > convert the driver to use the of_clk_hw_onecell_get() helper, which
> > already contains such a check.  Embedding a clk_hw_onecell_data
> > structure in the rs9_driver_data structure has the added benefit that
> > the clock array always has the correct size, and thus can no longer
> > become out of sync when adding support for new rs9 variants.
> >
> > Fixes: 892e0ddea1aa6f70 ("clk: rs9: Add Renesas 9-series PCIe clock generator driver")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > While this patch applies on top of "[PATCH v2] clk: rs9: Reserve 8
> > struct clk_hw slots for for 9FGV0841", it can be applied or backported
> > without, by ignoring the change from "clk_dif[4]" to "clk_dif[8]".
>
> Since the 9FGV0841 is the biggest part in the 9FGV series, the one-liner
> fix I posted is better suited as a stable backport. This rework
> shouldn't have the Fixes tag, since it is only that, a rework.

This is not just a rework, as it also fixes a second issue.
But since you prefer simpler fixes, I have submitted a v2 that just
adds the missing range check[2].  We can revisit the conversion to
of_clk_hw_onecell_get() later, after all fixes have landed.

> With that fixed:
>
> Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Thanks!

[2] "[PATCH] clk: rs9: Add clock index range check to rs9_of_clk_get()"
    https://lore.kernel.org/all/4cb63bd8b1e49407831431fbc88b218f720a74fd.1768899891.git.geert+renesas@glider.be

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
Re: [PATCH] clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()
Posted by Marek Vasut 2 weeks, 5 days ago
On 1/20/26 10:12 AM, Geert Uytterhoeven wrote:

Hello Geert,

> On Mon, 19 Jan 2026 at 20:36, Marek Vasut <marek.vasut@mailbox.org> wrote:
>> On 1/19/26 4:23 PM, Geert Uytterhoeven wrote:
>>> rs9_of_clk_get() does not validate the clock index in the passed
>>> DT clock specifier.  If DT specifies an incorrect and out-of-range
>>> index, this may access memory beyond the end of the fixed-size clk_dif[]
>>> array.
>>>
>>> Instead of fixing this by adding a range check to rs9_of_clk_get(),
>>> convert the driver to use the of_clk_hw_onecell_get() helper, which
>>> already contains such a check.  Embedding a clk_hw_onecell_data
>>> structure in the rs9_driver_data structure has the added benefit that
>>> the clock array always has the correct size, and thus can no longer
>>> become out of sync when adding support for new rs9 variants.
>>>
>>> Fixes: 892e0ddea1aa6f70 ("clk: rs9: Add Renesas 9-series PCIe clock generator driver")
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> While this patch applies on top of "[PATCH v2] clk: rs9: Reserve 8
>>> struct clk_hw slots for for 9FGV0841", it can be applied or backported
>>> without, by ignoring the change from "clk_dif[4]" to "clk_dif[8]".
>>
>> Since the 9FGV0841 is the biggest part in the 9FGV series, the one-liner
>> fix I posted is better suited as a stable backport. This rework
>> shouldn't have the Fixes tag, since it is only that, a rework.
> 
> This is not just a rework, as it also fixes a second issue.
> But since you prefer simpler fixes, I have submitted a v2 that just
> adds the missing range check[2].  We can revisit the conversion to
> of_clk_hw_onecell_get() later, after all fixes have landed.
This is nice, thank you !