Re: [PATCH v2 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 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