[PATCH v2 0/8] clk: clocking-wizard: Add static-config clock provider support

[PATCH v2 0/8] clk: clocking-wizard: Add static-config clock provider support

[Shubhrajyoti Datta]

 The Xilinx clocking-wizard IP can be used in a static-config mode
 where the hardware is pre-programmed at boot and no dynamic register
 access is required. In that case the driver should skip ioremap,
 read the fixed multiplier/divisor pairs from the device tree, and
 register fixed-factor clocks derived from clk_in1.

 This series:
   - adds the xlnx,clk-mul-div binding to carry mul/div pairs
   - makes the reg property optional for static-config nodes
   - skips ioremap when xlnx,static-config is present
   - moves clk_in1 acquisition before the static-config check so it
     is available in both code paths
   - registers fixed-factor output clocks in static-config mode


Changes in v2:
Merged the devicetree  patches
Patch added for notifier block and division by 0 

Shubhrajyoti Datta (8):
  dt-bindings: clock: clocking-wizard: Add static-config mode support
  clk: clocking-wizard: Do not map the memory for static-config
  clk: clocking-wizard: Move clk_in1 acquisition before static-config
    check
  clk: clocking-wizard: Add static-config clock provider support
  clk: clocking-wizard: Skip s_axi_aclk for static-config
  clk: clocking-wizard: Use dev_err_probe() when mapping registers
  clk: clocking-wizard: Fix division by zero and unbounded register
    write
  clk: clocking-wizard: Use separate notifier_block for each clock

 .../bindings/clock/xlnx,clocking-wizard.yaml  |  53 ++++-
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c    | 195 +++++++++++++++---
 2 files changed, 220 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH v2 1/8] dt-bindings: clock: clocking-wizard: Add static-config mode support

[Shubhrajyoti Datta]

Static-config MMCM/PLL ratios are fixed at IP build time, so the kernel
cannot read per-output multiply/divide from registers and needs DT pairs
to register fixed-factor clocks. Also add the tuples by pair in example.

In static-config mode the IP exposes only fixed-factor clock outputs and
has no runtime-programmable registers, so reg is not required.
Make reg optional for static-config

In static-config mode the AXI bus interface is unused, so s_axi_aclk
is not required. Allow clocks/clock-names to have only one entry
(clk_in1) when xlnx,static-config is present and enforce two entries
otherwise. Update the static-config example accordingly.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

Changes in v2:
Merged the similar patches
Allow check for current dt even if it is unused.

 .../bindings/clock/xlnx,clocking-wizard.yaml  | 53 +++++++++++++++++--
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
index b497c28e8094..865927b9f1be 100644
--- a/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
+++ b/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
@@ -29,11 +29,13 @@ properties:
     const: 1
 
   clocks:
+    minItems: 1
     items:
       - description: clock input
       - description: axi clock
 
   clock-names:
+    minItems: 1
     items:
       - const: clk_in1
       - const: s_axi_aclk
@@ -57,27 +59,68 @@ properties:
     description:
       Number of outputs.
 
+  xlnx,clk-mul-div:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    description:
+      Fixed MMCM/PLL multiply/divide ratios, one (multiplier, divisor)
+      pair per clock output relative to clk_in1. The number of entries
+      must equal xlnx,nr-outputs.
+    minItems: 1
+    maxItems: 8
+    items:
+      items:
+        - description: multiplier
+          minimum: 1
+        - description: divisor
+          minimum: 1
+
 required:
   - compatible
-  - reg
   - "#clock-cells"
   - clocks
   - clock-names
   - xlnx,speed-grade
   - xlnx,nr-outputs
 
+allOf:
+  - if:
+      required:
+        - xlnx,static-config
+    then:
+      required:
+        - xlnx,clk-mul-div
+    else:
+      required:
+        - reg
+      properties:
+        clocks:
+          minItems: 2
+        clock-names:
+          minItems: 2
+
 additionalProperties: false
 
 examples:
   - |
-    clock-controller@b0000000  {
+    clock-controller@b0000000 {
         compatible = "xlnx,clocking-wizard";
         reg = <0xb0000000 0x10000>;
         #clock-cells = <1>;
-        xlnx,static-config;
+        clocks = <&clkc 15>, <&clkc 18>;
+        clock-names = "clk_in1", "s_axi_aclk";
+        xlnx,nr-outputs = <6>;
         xlnx,speed-grade = <1>;
+    };
+
+  - |
+    clock-controller {
+        compatible = "xlnx,clocking-wizard";
+        #clock-cells = <1>;
+        clocks = <&clkc 15>;
+        clock-names = "clk_in1";
         xlnx,nr-outputs = <6>;
-        clock-names = "clk_in1", "s_axi_aclk";
-        clocks = <&clkc 15>, <&clkc 15>;
+        xlnx,speed-grade = <1>;
+        xlnx,static-config;
+        xlnx,clk-mul-div = <12 1>, <10 2>, <8 1>, <6 1>, <4 2>, <2 1>;
     };
 ...
-- 
2.34.1

[PATCH v2 2/8] clk: clocking-wizard: Do not map the memory for static-config

[Shubhrajyoti Datta]

With xlnx,static-config the MMCM/PLL topology is fixed at synthesis time
and no register programming is performed; only the dynamic path needs
the AXI register block. Move devm_platform_ioremap_resource() under the
non-static-config branch.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

(no changes since v1)

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 4a0136349f71..e082051221be 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1168,10 +1168,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, clk_wzrd);
 
-	clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(clk_wzrd->base))
-		return PTR_ERR(clk_wzrd->base);
-
 	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
 	if (IS_ERR(clk_wzrd->axi_clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
@@ -1183,6 +1179,10 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	}
 
 	if (!of_property_present(np, "xlnx,static-config")) {
+		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(clk_wzrd->base))
+			return PTR_ERR(clk_wzrd->base);
+
 		ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
 		if (!ret) {
 			if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
-- 
2.34.1

Re: [PATCH v2 2/8] clk: clocking-wizard: Do not map the memory for static-config

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional fetch of `s_axi_aclk` causes probe failure in static-config mode.
- [High] The driver completely skips clock and provider registration in static-config mode.
- [High] Notifier chain linked list corruption due to registering the same `struct notifier_block` twice.
--

--- Patch [2]: [PATCH v2 2/8] clk: clocking-wizard: Do not map the memory for static-config ---
commit 0264def41bcae176c3fa735b3a9b2da6412f451c
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Do not map the memory for static-config

With xlnx,static-config the MMCM/PLL topology is fixed at synthesis time
and no register programming is performed; only the dynamic path needs
the AXI register block. Move devm_platform_ioremap_resource() under the
non-static-config branch.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 4a0136349f71a..e082051221bef 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -1168,10 +1168,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, clk_wzrd);
>  
> -	clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(clk_wzrd->base))
> -		return PTR_ERR(clk_wzrd->base);
> -
>  	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
>  	if (IS_ERR(clk_wzrd->axi_clk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),

[Severity: High]
This isn't a bug introduced by this patch, but since the static-config device
tree bindings explicitly make the s_axi_aclk clock optional (because the bus
interface is unused), doesn't this unconditional call to devm_clk_get_enabled()
cause the probe to fail with -ENOENT if the clock is legitimately missing?

> @@ -1183,6 +1179,10 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  	}
>  
>  	if (!of_property_present(np, "xlnx,static-config")) {
> +		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
> +		if (IS_ERR(clk_wzrd->base))
> +			return PTR_ERR(clk_wzrd->base);
> +

[Severity: High]
This is a pre-existing issue, but by putting all the provider registration logic
inside this non-static-config block, doesn't the driver completely bypass
registering any output clocks when in static-config mode?

If clk_wzrd_register_output_clocks() and devm_of_clk_add_hw_provider() are
skipped entirely, won't consumer devices depending on this provider hang
indefinitely with -EPROBE_DEFER?

[ ... ]

>  		if (clk_wzrd->speed_grade) {
>  			clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
>  
>  			ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
>  							 &clk_wzrd->nb);
>  			if (ret)
>  				dev_warn(&pdev->dev,
>  					 "unable to register clock notifier\n");
>  
>  			ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
>  							 &clk_wzrd->nb);

[Severity: High]
This is a pre-existing issue, but does calling devm_clk_notifier_register() for
both clk_in1 and axi_clk using the exact same struct notifier_block instance
(&clk_wzrd->nb) cause notifier chain corruption?

Since the notifier_block contains a next pointer that gets overwritten when
linked into a second chain, won't this splice the clk_in1 and axi_clk notifier
chains together and cause list corruption or use-after-free during clock rate
changes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=2

[PATCH v2 3/8] clk: clocking-wizard: Move clk_in1 acquisition before static-config check

[Shubhrajyoti Datta]

The clk_in1 is the input clock for both the dynamic reconfig and the
static-config paths. Acquire clk_in1 for static-config as well. Output
clocks are registered as fixed-factor children of clk_in1.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

(no changes since v1)

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index e082051221be..ffc78c90bee6 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1178,6 +1178,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
+	if (IS_ERR(clk_wzrd->clk_in1))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
+				     "failed to get clk_in1\n");
+
 	if (!of_property_present(np, "xlnx,static-config")) {
 		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
 		if (IS_ERR(clk_wzrd->base))
@@ -1192,11 +1197,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 			}
 		}
 
-		clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
-		if (IS_ERR(clk_wzrd->clk_in1))
-			return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
-					     "clk_in1 not found\n");
-
 		ret = clk_wzrd_register_output_clocks(&pdev->dev, nr_outputs);
 		if (ret)
 			return ret;
-- 
2.34.1

Re: [PATCH v2 3/8] clk: clocking-wizard: Move clk_in1 acquisition before static-config check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unmet dependency on `s_axi_aclk` in static-config mode causes probe failure.
--

--- Patch [3]: [PATCH v2 3/8] clk: clocking-wizard: Move clk_in1 acquisition before static-config check ---
commit 2d23efc5eb24a84c2dc3c21068e9e68c699593d8
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Move clk_in1 acquisition before static-config check

The clk_in1 is the input clock for both the dynamic reconfig and the
static-config paths. Acquire clk_in1 for static-config as well. Output
clocks are registered as fixed-factor children of clk_in1.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index e082051221bef..ffc78c90bee6f 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -1178,6 +1178,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
> +	if (IS_ERR(clk_wzrd->clk_in1))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
> +				     "failed to get clk_in1\n");
> +

[Severity: High]
Does this probe sequence correctly handle devices using xlnx,static-config
without an s_axi_aclk?

Earlier in clk_wzrd_probe(), the driver unconditionally acquires s_axi_aclk:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_probe() {
    ...
    clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
    if (IS_ERR(clk_wzrd->axi_clk))
        return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
                             "s_axi_aclk not found\n");
    ...
}

If a device tree specifies xlnx,static-config and omits the s_axi_aclk clock,
wouldn't this unconditional acquisition return -ENOENT and cause the probe to
fail before it even reaches this new clk_in1 acquisition?

>  	if (!of_property_present(np, "xlnx,static-config")) {
>  		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
>  		if (IS_ERR(clk_wzrd->base))

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=3

[PATCH v2 4/8] clk: clocking-wizard: Add static-config clock provider support

[Shubhrajyoti Datta]

When xlnx,static-config is present the divider/multiplier path is
synthesized inside the Wizard without exposing runtime MMIO
reconfiguration, so omit the AXI register mapping and advertise each
routed output clock as a clk_fixed_factor child of clk_in1 using the
synthesized ratios exported through xlnx,clk-mul-div.

However the parent clock of clk_in1 can still be gated, disabled, or
(re-)enabled after FPGA programming (typical FPGA Manager flows,
CCF-managed parents, PLL fabric power sequencing).

Add a 10 us delay in each output clock's .enable() hook so consumers
wait for reference settling before first access.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

(no changes since v1)

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 137 ++++++++++++++++++++-
 1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index ffc78c90bee6..5470a717fccc 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -107,6 +108,7 @@
 #define VER_WZRD_O_MAX			511
 #define WZRD_MIN_ERR			20000
 #define WZRD_FRAC_POINTS		1000
+#define WZRD_ENABLE_DELAY_US		10
 
 /* Get the mask from width */
 #define div_mask(width)			((1 << (width)) - 1)
@@ -697,6 +699,13 @@ static int clk_wzrd_ver_determine_rate_all(struct clk_hw *hw,
 	return 0;
 }
 
+static int clk_wzrd_enable(struct clk_hw *hw)
+{
+	/* Allow the output clock to settle after enable */
+	udelay(WZRD_ENABLE_DELAY_US);
+	return 0;
+}
+
 static const struct clk_ops clk_wzrd_ver_divider_ops = {
 	.determine_rate = clk_wzrd_determine_rate,
 	.set_rate = clk_wzrd_ver_dynamic_reconfig,
@@ -790,6 +799,85 @@ static const struct clk_ops clk_wzrd_clk_divider_ops_f = {
 	.recalc_rate = clk_wzrd_recalc_ratef,
 };
 
+static unsigned long
+clk_wzrd_static_factor_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+	unsigned long long rate;
+
+	rate = (unsigned long long)parent_rate * fix->mult;
+	do_div(rate, fix->div);
+	return (unsigned long)rate;
+}
+
+static int clk_wzrd_static_factor_determine_rate(struct clk_hw *hw,
+						 struct clk_rate_request *req)
+{
+	struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+
+	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+		unsigned long best_parent;
+
+		best_parent = (req->rate / fix->mult) * fix->div;
+		req->best_parent_rate =
+			clk_hw_round_rate(clk_hw_get_parent(hw), best_parent);
+	}
+
+	req->rate = (req->best_parent_rate / fix->div) * fix->mult;
+
+	return 0;
+}
+
+static int clk_wzrd_static_factor_set_rate(struct clk_hw *hw,
+					   unsigned long rate,
+					   unsigned long parent_rate)
+{
+	return 0;
+}
+
+static const struct clk_ops clk_wzrd_static_fixed_factor_ops = {
+	.enable = clk_wzrd_enable,
+	.determine_rate = clk_wzrd_static_factor_determine_rate,
+	.set_rate = clk_wzrd_static_factor_set_rate,
+	.recalc_rate = clk_wzrd_static_factor_recalc_rate,
+};
+
+static struct clk_hw *
+clk_wzrd_devm_register_static_fixed_factor(struct device *dev,
+					   const char *name,
+					   const struct clk_parent_data *parent_data,
+					   unsigned long flags,
+					   unsigned int mult,
+					   unsigned int div)
+{
+	struct clk_init_data init = {};
+	struct clk_fixed_factor *fix;
+	struct clk_hw *hw;
+	int ret;
+
+	fix = devm_kzalloc(dev, sizeof(*fix), GFP_KERNEL);
+	if (!fix)
+		return ERR_PTR(-ENOMEM);
+
+	fix->mult = mult;
+	fix->div = div;
+
+	init.name = name;
+	init.ops = &clk_wzrd_static_fixed_factor_ops;
+	init.flags = flags;
+	init.parent_data = parent_data;
+	init.num_parents = 1;
+
+	fix->hw.init = &init;
+
+	hw = &fix->hw;
+	ret = devm_clk_hw_register(dev, hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return hw;
+}
+
 static struct clk_hw *clk_wzrd_register_divf(struct device *dev,
 					  const char *name,
 					  const char *parent_name,
@@ -1154,9 +1242,11 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct clk_wzrd *clk_wzrd;
+	const char *clk_name;
 	unsigned long rate;
+	struct clk_hw *hw;
 	int nr_outputs;
-	int ret;
+	int ret, i;
 
 	ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs);
 	if (ret || nr_outputs > WZRD_NUM_OUTPUTS)
@@ -1224,6 +1314,51 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 				dev_warn(&pdev->dev,
 					 "unable to register clock notifier\n");
 		}
+	} else {
+		u32 mul_div[WZRD_NUM_OUTPUTS * 2];
+		const struct clk_parent_data parent_data = { .fw_name = "clk_in1" };
+		int num_elems = nr_outputs * 2;
+
+		/*
+		 * xlnx,clk-mul-div is a uint32-matrix of <mul div> pairs;
+		 * FDT encodes it as a flat u32 array so we can read it directly.
+		 */
+		ret = of_property_read_u32_array(np, "xlnx,clk-mul-div",
+						 mul_div, num_elems);
+		if (ret) {
+			dev_err(&pdev->dev, "xlnx,clk-mul-div missing or invalid\n");
+			return ret;
+		}
+
+		for (i = 0; i < nr_outputs; i++) {
+			u32 mul = mul_div[2 * i];
+			u32 div = mul_div[2 * i + 1];
+
+			if (!mul || !div)
+				return dev_err_probe(&pdev->dev, -EINVAL,
+						     "invalid mul/div for clkout%d\n", i);
+
+			clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+						  "%s_out%d", dev_name(&pdev->dev), i);
+			if (!clk_name)
+				return -ENOMEM;
+
+			hw = clk_wzrd_devm_register_static_fixed_factor(&pdev->dev, clk_name,
+									&parent_data,
+									CLK_SET_RATE_PARENT,
+									mul, div);
+			if (IS_ERR(hw))
+				return PTR_ERR(hw);
+			clk_wzrd->clk_data.hws[i] = hw;
+		}
+
+		clk_wzrd->clk_data.num = nr_outputs;
+
+		ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
+						  &clk_wzrd->clk_data);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "unable to register clock provider\n");
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v2 4/8] clk: clocking-wizard: Add static-config clock provider support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Flexible array access violates `__counted_by` bounds check due to uninitialized `num` variable in `clk_wzrd_probe`.
--

--- Patch [4]: [PATCH v2 4/8] clk: clocking-wizard: Add static-config clock provider support ---
commit 0905a91a49c4de73e34946b056e201766ae078dd
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Add static-config clock provider support

This commit adds support for static-config mode in the clocking wizard.
It bypasses AXI register mapping and advertises output clocks as
clk_fixed_factor children of clk_in1 using synthesized ratios. It also
adds a 10 us delay in the enable hook to allow the clock to settle.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index ffc78c90bee6f..5470a717fccc8 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
[ ... ]
> @@ -1224,6 +1314,51 @@ static int clk_wzrd_probe(struct platform_device *pdev)
[ ... ]
> +	} else {
> +		u32 mul_div[WZRD_NUM_OUTPUTS * 2];
> +		const struct clk_parent_data parent_data = { .fw_name = "clk_in1" };
> +		int num_elems = nr_outputs * 2;
> +
> +		/*
> +		 * xlnx,clk-mul-div is a uint32-matrix of <mul div> pairs;
> +		 * FDT encodes it as a flat u32 array so we can read it directly.
> +		 */
> +		ret = of_property_read_u32_array(np, "xlnx,clk-mul-div",
> +						 mul_div, num_elems);
> +		if (ret) {
> +			dev_err(&pdev->dev, "xlnx,clk-mul-div missing or invalid\n");
> +			return ret;
> +		}
> +
> +		for (i = 0; i < nr_outputs; i++) {
> +			u32 mul = mul_div[2 * i];
> +			u32 div = mul_div[2 * i + 1];
> +
> +			if (!mul || !div)
> +				return dev_err_probe(&pdev->dev, -EINVAL,
> +						     "invalid mul/div for clkout%d\n", i);
> +
> +			clk_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +						  "%s_out%d", dev_name(&pdev->dev), i);
> +			if (!clk_name)
> +				return -ENOMEM;
> +
> +			hw = clk_wzrd_devm_register_static_fixed_factor(&pdev->dev, clk_name,
> +									&parent_data,
> +									CLK_SET_RATE_PARENT,
> +									mul, div);
> +			if (IS_ERR(hw))
> +				return PTR_ERR(hw);
> +			clk_wzrd->clk_data.hws[i] = hw;

[Severity: High]
Will this trigger a UBSAN bounds-check panic when assigning to hws[i]?

The struct clk_hw_onecell_data uses the __counted_by(num) attribute for
its hws flexible array. Since clk_wzrd->clk_data.num is zero here (from
the initial kzalloc) and is only updated after this loop, does writing
to hws[i] violate the __counted_by bounds check?

Should clk_wzrd->clk_data.num be set to nr_outputs before entering the
loop to prevent a panic during device probe when CONFIG_UBSAN_BOUNDS
is enabled?

It looks like the pre-existing dynamic config path in
clk_wzrd_register_output_clocks() suffers from this same initialization
ordering issue, even though this specific loop is new to this patch.

> +		}
> +
> +		clk_wzrd->clk_data.num = nr_outputs;
> +
> +		ret = devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
> +						  &clk_wzrd->clk_data);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "unable to register clock provider\n");
>  	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=4

[PATCH v2 5/8] clk: clocking-wizard: Skip s_axi_aclk for static-config

[Shubhrajyoti Datta]

For static-config mode the AXI bus interface is not used, so there is
no need to get and enable s_axi_aclk. Move the axi_clk setup inside
the non-static-config branch.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

(no changes since v1)

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 23 +++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 5470a717fccc..fe73ee02b54e 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1243,7 +1243,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct clk_wzrd *clk_wzrd;
 	const char *clk_name;
-	unsigned long rate;
 	struct clk_hw *hw;
 	int nr_outputs;
 	int ret, i;
@@ -1258,22 +1257,24 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, clk_wzrd);
 
-	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
-	if (IS_ERR(clk_wzrd->axi_clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
-				     "s_axi_aclk not found\n");
-	rate = clk_get_rate(clk_wzrd->axi_clk);
-	if (rate > WZRD_ACLK_MAX_FREQ) {
-		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
-		return -EINVAL;
-	}
-
 	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
 	if (IS_ERR(clk_wzrd->clk_in1))
 		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
 				     "failed to get clk_in1\n");
 
 	if (!of_property_present(np, "xlnx,static-config")) {
+		unsigned long rate;
+
+		clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
+		if (IS_ERR(clk_wzrd->axi_clk))
+			return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
+					     "s_axi_aclk not found\n");
+		rate = clk_get_rate(clk_wzrd->axi_clk);
+		if (rate > WZRD_ACLK_MAX_FREQ) {
+			dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
+			return -EINVAL;
+		}
+
 		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
 		if (IS_ERR(clk_wzrd->base))
 			return PTR_ERR(clk_wzrd->base);
-- 
2.34.1

Re: [PATCH v2 5/8] clk: clocking-wizard: Skip s_axi_aclk for static-config

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Double registration of the same `struct notifier_block` instance into two different SRCU notifier chains.
--

--- Patch [5]: [PATCH v2 5/8] clk: clocking-wizard: Skip s_axi_aclk for static-config ---
commit 02fdca3c9a0733ff035a7b08c1d76f778176b4c1
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
Subject: clk: clocking-wizard: Skip s_axi_aclk for static-config

This patch moves the axi_clk setup into the non-static-config branch,
skipping the s_axi_aclk initialization when the AXI bus interface is not
used in static-config mode.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 5470a717fccc8..fe73ee02b54ee 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
[ ... ]
> @@ -1258,22 +1257,24 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, clk_wzrd);
>  
> -	clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
> -	if (IS_ERR(clk_wzrd->axi_clk))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
> -				     "s_axi_aclk not found\n");
> -	rate = clk_get_rate(clk_wzrd->axi_clk);
> -	if (rate > WZRD_ACLK_MAX_FREQ) {
> -		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", rate);
> -		return -EINVAL;
> -	}
> -
>  	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
>  	if (IS_ERR(clk_wzrd->clk_in1))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1),
>  				     "failed to get clk_in1\n");
>  
>  	if (!of_property_present(np, "xlnx,static-config")) {
> +		unsigned long rate;
> +
> +		clk_wzrd->axi_clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");

[Severity: High]
This is a pre-existing issue, but since we are modifying the setup of
axi_clk here, does the subsequent notifier registration corrupt the core
clock notifier lists?

Looking further down in drivers/clk/xilinx/clk-xlnx-clock-wizard.c in
clk_wzrd_probe(), the exact same notifier block instance is registered to
two different clocks unconditionally when xlnx,speed-grade is set:

    if (clk_wzrd->speed_grade) {
        clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;

        ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
                                         &clk_wzrd->nb);
        ...
        ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
                                         &clk_wzrd->nb);
    }

Since srcu_notifier_chain_register() modifies the next pointer of the
passed notifier_block, doesn't the second registration overwrite the next
pointer established by the first?

Could this break the clk_in1 notifier list, causing its traversal to follow
into the axi_clk notifier list, potentially leading to an infinite loop,
skipped notifiers, or a kernel panic upon subsequent rate changes?

> +		if (IS_ERR(clk_wzrd->axi_clk))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk),
> +					     "s_axi_aclk not found\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=5

[PATCH v2 6/8] clk: clocking-wizard: Use dev_err_probe() when mapping registers

[Shubhrajyoti Datta]

Align the devm_platform_ioremap_resource() error path with clk_in1 and
s_axi_aclk handling for consistent logging and deferred-probe behavior.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

(no changes since v1)

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index fe73ee02b54e..381e396aef0e 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -1277,7 +1277,8 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 
 		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
 		if (IS_ERR(clk_wzrd->base))
-			return PTR_ERR(clk_wzrd->base);
+			return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->base),
+					     "failed to map registers\n");
 
 		ret = of_property_read_u32(np, "xlnx,speed-grade", &clk_wzrd->speed_grade);
 		if (!ret) {
-- 
2.34.1

Re: [PATCH v2 6/8] clk: clocking-wizard: Use dev_err_probe() when mapping registers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Wrapping `devm_platform_ioremap_resource()` in `dev_err_probe()` causes redundant double error logging, and the commit message's justification regarding deferred-probe behavior is factually incorrect.

Pre-existing issues:
- [High] Registering the exact same `struct notifier_block` instance (`clk_wzrd->nb`) to two different clock notifier chains corrupts the SRCU linked lists.
--

--- Patch [6]: [PATCH v2 6/8] clk: clocking-wizard: Use dev_err_probe() when mapping registers ---
commit 62924401e7cb937dd4102855b2b6f8bf68175467
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Use dev_err_probe() when mapping registers

This commit updates the error handling for mapping registers in the clock
wizard driver. It replaces a direct return of the error code with a call
to dev_err_probe().

> Align the devm_platform_ioremap_resource() error path with clk_in1 and
> s_axi_aclk handling for consistent logging and deferred-probe behavior.

[Severity: Medium]
Is this description accurate regarding deferred-probe behavior?

Since devm_platform_ioremap_resource() never returns -EPROBE_DEFER, could
the commit message be more precise about what deferred-probe behavior is
expected here?

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index fe73ee02b54ee..381e396aef0e7 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -1277,7 +1277,8 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  
>  		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
>  		if (IS_ERR(clk_wzrd->base))
> -			return PTR_ERR(clk_wzrd->base);
> +			return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->base),
> +					     "failed to map registers\n");

[Severity: Medium]
Does wrapping devm_platform_ioremap_resource() with dev_err_probe() cause
redundant error logging?

Because devm_platform_ioremap_resource() calls __devm_ioremap_resource()
internally, which natively utilizes dev_err_probe() to print detailed error
messages for all its failure modes, would this exact same failure end up
being logged twice?

[Severity: High]
This is a pre-existing issue, but does registering the same notifier block
instance to two different clock notifier chains corrupt the SRCU linked
lists?

Looking at clk_wzrd_probe(), the driver attempts to register the exact same
notifier block instance to both clk_in1 and axi_clk:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_probe() {
    ...
    clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;

    ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->clk_in1,
                                     &clk_wzrd->nb);
    if (ret)
        dev_warn(&pdev->dev,
                 "unable to register clock notifier\n");

    ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
                                     &clk_wzrd->nb);
    ...
}

Because a notifier_block structure contains a next pointer used to link it
into a singly-linked list, does registering it to the second clock's chain
overwrite the next pointer and corrupt the first clock's notifier list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=6

[PATCH v2 7/8] clk: clocking-wizard: Fix division by zero and unbounded register write

[Shubhrajyoti Datta]

1. clk_wzrd_determine_rate(): DIV_ROUND_CLOSEST() can return 0 when
   the requested rate greatly exceeds the parent rate, causing a
   division by zero on the subsequent parent_rate / div. Clamp div
   to a minimum of 1.

2. clk_wzrd_dynamic_reconfig(): min_t() result was not assigned back
   to value, so the cap to WZRD_DR_MAX_INT_DIV_VALUE was never applied
   before the register write.

3. clk_wzrd_get_divisors() and clk_wzrd_get_divisors_ver(): If rate is
   0, DIV_ROUND_CLOSEST_ULL divides by zero. Return -EINVAL early.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

Changes in v2:
Patch added

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 381e396aef0e..289c32ab5cd8 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -296,7 +296,7 @@ static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
 	value = DIV_ROUND_CLOSEST(parent_rate, rate);
 
 	/* Cap the value to max */
-	min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
+	value = min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
 
 	/* Set divisor and clear phase offset */
 	writel(value, div_addr);
@@ -334,6 +334,8 @@ static int clk_wzrd_determine_rate(struct clk_hw *hw,
 	 * achievable
 	 */
 	div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
+	if (!div)
+		div = 1;
 
 	req->rate = req->best_parent_rate / div;
 
@@ -348,6 +350,9 @@ static int clk_wzrd_get_divisors_ver(struct clk_hw *hw, unsigned long rate,
 	u32 m, d, o;
 	u32 mmin, mmax, dmin, dmax, omin, omax;
 
+	if (!rate)
+		return -EINVAL;
+
 	mmin = VER_WZRD_M_MIN;
 	mmax = VER_WZRD_M_MAX;
 	dmin = VER_WZRD_D_MIN;
@@ -390,6 +395,9 @@ static int clk_wzrd_get_divisors(struct clk_hw *hw, unsigned long rate,
 	u64 m, d, o;
 	u64 mmin, mmax, dmin, dmax, omin, omax, mdmin, mdmax;
 
+	if (!rate)
+		return -EINVAL;
+
 	mmin = WZRD_M_MIN << 3;
 	mmax = WZRD_M_MAX << 3;
 	dmin = WZRD_D_MIN;
-- 
2.34.1

Re: [PATCH v2 7/8] clk: clocking-wizard: Fix division by zero and unbounded register write

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Integer truncation and logic error causing silent massive overclocking.

Pre-existing issues:
- [High] Unhandled division by zero in rate calculation and configuration callbacks.
- [High] Out-of-bounds register write leading to bitfield corruption.
- [High] Missing lock leading to data races during dynamic hardware reconfiguration.
- [High] Integer overflow in fractional divider calculation on 32-bit systems.
--

--- Patch [7]: [PATCH v2 7/8] clk: clocking-wizard: Fix division by zero and unbounded register write ---
commit 4ea4bda6fb88975f9f2f07bd41a24f7bd7621124
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Fix division by zero and unbounded register write

This commit attempts to fix division by zero and unbounded register writes by
clamping the divisor to a minimum of 1 and ensuring min_t results are
correctly assigned. It also adds early returns when the rate is 0 in the
divisor calculation functions.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 381e396aef0e7..289c32ab5cd80 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -296,7 +296,7 @@ static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
>  	value = DIV_ROUND_CLOSEST(parent_rate, rate);

[Severity: High]
This is a pre-existing issue, but if a consumer driver requests a clock rate
of 0, does the DIV_ROUND_CLOSEST() macro cause a division by zero panic here?

Since this occurs while holding the divider->lock, would a panic also leak
the global hardware lock?

>  
>  	/* Cap the value to max */
> -	min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
> +	value = min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
>  
>  	/* Set divisor and clear phase offset */
>  	writel(value, div_addr);

[ ... ]

> @@ -334,6 +334,8 @@ static int clk_wzrd_determine_rate(struct clk_hw *hw,
>  	 * achievable
>  	 */
>  	div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);

[Severity: High]
This is a pre-existing issue, but if req->rate is 0, wouldn't the
DIV_ROUND_CLOSEST() macro itself trigger a division by zero panic before it
even reaches the newly added zero check below?

> +	if (!div)
> +		div = 1;

[Severity: High]
Because the div variable in clk_wzrd_determine_rate() is declared as a u8:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_determine_rate() {
    u8 div;
    ...
    div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
}

Does assigning a valid large divisor (like 256) result in a modulo-256
truncation to 0?

If a consumer requests a valid low frequency that requires a multiple of 256,
this truncation could silently force the divisor to 0. Then, this new logic
would force the divisor to 1, potentially bypassing the hardware limit and
causing unexpected clock frequencies.

>  
>  	req->rate = req->best_parent_rate / div;
>  

[ ... ]

Additionally, there are a few other pre-existing issues in this file worth
investigating:

[Severity: High]
This is a pre-existing issue, but does the Versal variant lack the same bounds
checking that was just fixed above?

Looking at clk_wzrd_ver_dynamic_reconfig():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_ver_dynamic_reconfig() {
    ...
    value = DIV_ROUND_CLOSEST(parent_rate, rate);
    regh = (value / 4);
    ...
    regval = regh | regh << WZRD_CLKFBOUT_H_SHIFT;
    writel(regval, div_addr + 4);
    ...
}

If the rate is low and demands a large divisor, an unbounded value propagates
to regh. Because it is OR-ed without a bitmask, could this overwrite adjacent
configuration bits in the register?

Also, like clk_wzrd_dynamic_reconfig(), does this DIV_ROUND_CLOSEST() call
panic if the rate is 0?


[Severity: High]
This is a pre-existing issue, but is the fractional dynamic reconfiguration
path missing spinlock protection?

While other paths synchronize access to shared wizard initialization
registers, clk_wzrd_dynamic_reconfig_f() seems to omit the lock entirely:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_dynamic_reconfig_f() {
    int err;
    u32 value, pre;
    ...
    writel(WZRD_DR_BEGIN_DYNA_RECONF_5_2,
           divider->base + WZRD_DR_INIT_REG_OFFSET);
    ...
}

Could concurrent rate changes on multiple fractional outputs corrupt the
sequence of shared register writes?


[Severity: High]
This is a pre-existing issue, but does the fractional divider calculation
overflow on 32-bit systems?

Looking at clk_wzrd_dynamic_reconfig_f():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_dynamic_reconfig_f() {
    ...
    rate_div = DIV_ROUND_DOWN_ULL(parent_rate * 1000, rate);
    clockout0_div = rate_div / 1000;
    pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
    ...
}

On 32-bit architectures, parent_rate * 1000 is computed as a 32-bit integer.
If parent_rate exceeds 4.29 MHz, wouldn't this overflow before the implicit
cast to u64 by DIV_ROUND_DOWN_ULL?

Furthermore, do DIV_ROUND_DOWN_ULL() and DIV_ROUND_CLOSEST() trigger a
division by zero if the rate is 0?


[Severity: High]
This is a pre-existing issue, but do the recalculation paths also suffer from
division by zero if the hardware is uninitialized?

Looking at clk_wzrd_recalc_rate_all():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_recalc_rate_all() {
    ...
    return DIV_ROUND_CLOSEST_ULL(parent_rate * mul, d * (o * 1000 + f));
}

And clk_wzrd_recalc_ratef():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_recalc_ratef() {
    ...
    return mult_frac(parent_rate, 1000, (div * 1000) + frac);
}

If the hardware register values (d, o, f, div, frac) are 0 at boot-time,
would dividing by these unchecked values cause a division by zero panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=7

[PATCH v2 8/8] clk: clocking-wizard: Use separate notifier_block for each clock

[Shubhrajyoti Datta]

A struct notifier_block has a single 'next' pointer, so registering the
same instance to two different notification chains corrupts the linked
lists. Use a dedicated nb_axi for the s_axi_aclk notifier chain.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
patch added for notifiew_block

---

(no changes since v1)

 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index 289c32ab5cd8..bcbfb4341dba 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -126,7 +126,8 @@ enum clk_wzrd_int_clks {
 /**
  * struct clk_wzrd - Clock wizard private data structure
  *
- * @nb:			Notifier block
+ * @nb:			Notifier block for clk_in1
+ * @nb_axi:		Notifier block for s_axi_aclk
  * @base:		Memory base
  * @clk_in1:		Handle to input clock 'clk_in1'
  * @axi_clk:		Handle to input clock 's_axi_aclk'
@@ -137,6 +138,7 @@ enum clk_wzrd_int_clks {
  */
 struct clk_wzrd {
 	struct notifier_block nb;
+	struct notifier_block nb_axi;
 	void __iomem *base;
 	struct clk *clk_in1;
 	struct clk *axi_clk;
@@ -1318,8 +1320,10 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 				dev_warn(&pdev->dev,
 					 "unable to register clock notifier\n");
 
+			clk_wzrd->nb_axi.notifier_call = clk_wzrd_clk_notifier;
+
 			ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
-							 &clk_wzrd->nb);
+							 &clk_wzrd->nb_axi);
 			if (ret)
 				dev_warn(&pdev->dev,
 					 "unable to register clock notifier\n");
-- 
2.34.1

Re: [PATCH v2 8/8] clk: clocking-wizard: Use separate notifier_block for each clock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect `container_of` cast when reusing the same notifier callback for a new `notifier_block`, resulting in a misaligned `struct clk_wzrd` pointer and out-of-bounds memory accesses.
--

--- Patch [8]: [PATCH v2 8/8] clk: clocking-wizard: Use separate notifier_block for each clock ---
commit f1481850744c067369ec043b5f8c6bf3ee19d10b
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Use separate notifier_block for each clock

This commit introduces a dedicated notifier_block, nb_axi, for the
s_axi_aclk chain. This prevents linked list corruption that occurs
when registering a single notifier block to multiple chains.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 289c32ab5cd80..bcbfb4341dbab 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -137,6 +138,7 @@ enum clk_wzrd_int_clks {
>   */
>  struct clk_wzrd {
>  	struct notifier_block nb;
> +	struct notifier_block nb_axi;
>  	void __iomem *base;

[ ... ]

> @@ -1318,8 +1320,10 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  				dev_warn(&pdev->dev,
>  					 "unable to register clock notifier\n");
>  
> +			clk_wzrd->nb_axi.notifier_call = clk_wzrd_clk_notifier;
> +
>  			ret = devm_clk_notifier_register(&pdev->dev, clk_wzrd->axi_clk,
> -							 &clk_wzrd->nb);
> +							 &clk_wzrd->nb_axi);

[Severity: High]
Does sharing clk_wzrd_clk_notifier for nb_axi lead to an incorrect
container_of() cast?

Looking at clk_wzrd_clk_notifier(), it uses to_clk_wzrd(nb), which is
hardcoded to expect the original nb field:

#define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)

Since nb_axi is at a non-zero offset in struct clk_wzrd (unlike nb), won't 
using the same callback for nb_axi result in a misaligned struct clk_wzrd 
pointer? 

It seems this would lead to out-of-bounds memory accesses and incorrectly 
skipped frequency bounds checks when the s_axi_aclk chain triggers a rate 
change.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709111403.1579159-1-shubhrajyoti.datta@amd.com?part=8