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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver registers the same `struct notifier_block` instance (`clk_wzrd->nb`) to two different clock notification chains (`clk_in1` and `axi_clk`), causing notifier list corruption.
--

commit 2cde73f9cf8529ad3e29f28db978947c4f80fd0e
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

clk: clocking-wizard: Skip s_axi_aclk for static-config

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.

> 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");
> +		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;
> +		}
> +

[Severity: High]
This is a pre-existing issue, but does using the same struct notifier_block
instance for multiple clock notifiers corrupt the notification chain?

Looking further down in clk_wzrd_probe(), the same &clk_wzrd->nb instance
is registered to two different clocks:

    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 a struct notifier_block has a single next pointer, registering the
same instance to a second chain overwrites its next pointer. Doesn't this
merge the two clock notification chains and corrupt the first notifier list?

>  		clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0);
>  		if (IS_ERR(clk_wzrd->base))
>  			return PTR_ERR(clk_wzrd->base);

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