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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing implementation for static-config mode clock registration
- [High] Unconditional s_axi_aclk acquisition breaks static-config mode probe

Pre-existing issues:
- [High] Double registration of the same notifier_block corrupts SRCU notifier chains
--

--- Patch [3]: [PATCH v3 3/8] clk: clocking-wizard: Move clk_in1 acquisition before static-config check ---
commit 7ab44b637bfcd95c4cb1c02dbfa9fac870a83ce3
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;
>  	}
>  

[Severity: High]
Does unconditionally fetching s_axi_aclk break the probe for static-config
mode?

Earlier in 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 the device tree bindings explicitly omit s_axi_aclk when using
xlnx,static-config, wouldn't this unconditional fetch return -ENOENT and
abort the probe?

> +	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")) {

[Severity: High]
Does this code skip registering output clocks for the static-config mode?

The commit message states that "Output clocks are registered as fixed-factor
children of clk_in1".

However, since all the clock registration logic is inside the
!of_property_present(np, "xlnx,static-config") block, it seems this mode
just returns 0 without parsing xlnx,clk-mul-div or calling any clock
registration functions.

>  		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;

[Severity: High]
This isn't a bug introduced by this patch, but does registering the same
notifier block twice corrupt the SRCU notifier chains?

Later in clk_wzrd_probe():

	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);

The struct notifier_block has a single next pointer used by the kernel's
list implementation. Registering the single clk_wzrd->nb instance to two
distinct lists overwrites its next pointer, which can physically merge the
two independent clock notifier lists and drop any notifiers that followed in
the first list.

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