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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional AXI clock fetch breaks probe in static-config mode.

Pre-existing issues:
- [High] Driver completely skips parsing and registering output clocks when static-config is enabled, acting as a silent stub.
- [High] The driver shares a single `struct notifier_block` instance to register rate-change notifiers for two different clocks, corrupting the kernel's SRCU notifier chains.
- [High] Ineffective value cap allows writing out-of-range values to hardware registers.
--

--- Patch [2]: [PATCH v3 2/8] clk: clocking-wizard: Do not map the memory for static-config ---
commit 5be3eccdee47c9c51ffe9fd16deab92141ef8346
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
Subject: 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

[ ... ]

> static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
> 				     unsigned long parent_rate)
> {
[ ... ]
> 	value = DIV_ROUND_CLOSEST(parent_rate, rate);
> 
> 	/* Cap the value to max */
> 	min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
> 
> 	/* Set divisor and clear phase offset */
> 	writel(value, div_addr);

[Severity: High]
This is a pre-existing issue, but is the value cap effective here?

The min_t() macro evaluates as a standalone statement and doesn't modify its
arguments in place. Because the returned minimum value is discarded, the
'value' variable remains entirely uncapped when passed to writel(). If the
calculated divisor exceeds the expected width, it could corrupt adjacent bit
fields within the hardware register.

[ ... ]

> @@ -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]
Does this unconditionally fail the probe when static-config is enabled?

The commit message notes that only the dynamic path needs the AXI block, which
implies "s_axi_aclk" might be omitted from the device tree for static
configurations. If the clock is missing, devm_clk_get_enabled() will return
-ENOENT and immediately abort the probe before reaching the static-config
check.

> @@ -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 does this branch entirely skip clock
registration for static configurations?

When "xlnx,static-config" is present, the driver skips this block and simply
returns 0 at the end of the probe function. No fixed-factor clocks are
registered, meaning any downstream consumers requesting a clock from this
provider will indefinitely defer their probe (-EPROBE_DEFER).

[ ... ]

> 			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 this corrupt the SRCU notifier chain?

The exact same notifier_block instance (&clk_wzrd->nb) is registered to two
independent clock notifier chains (clk_in1 and axi_clk). The notifier API links
the structure into a singly-linked list via the 'next' pointer embedded inside
the notifier_block.

Registering the same block twice overwrites the 'next' pointer for the first
chain, which can permanently break the lists and lead to kernel oopses or
infinite loops during traversal.

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