Re: [PATCH v3 5/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 provided concern incorrectly states that the callback resolves its context using an invalid offset due to `nb_axi`. In the actual code, `nb_axi` does not exist, and `clk_wzrd->nb` is used for both registrations, so the offset calculation is correct. However, a critical bug exists at the same location: the exact same `notifier_block` instance (`clk_wzrd->nb`) is registered to two different clock notifier chains (`clk_in1` and `axi_clk`). A `notifier_block` has only one `next` pointer and cannot be added to multiple SRCU notifier chains simultaneously (or twice to the same chain). Doing so corrupts the notifier linked list.
--

--- Patch [5]: [PATCH v3 5/8] clk: clocking-wizard: Skip s_axi_aclk for static-config ---
commit 8f005ad6b615119302b96a70915b5c138c7ff09f
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 1332abc8a9501..c79733f58082d 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
[ ... ]
> @@ -1259,22 +1258,24 @@ static int clk_wzrd_probe(struct platform_device *pdev)
[ ... ]
>  	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 isn't a bug introduced by this patch, but while reviewing the axi_clk
setup here, I noticed a pre-existing issue further down in clk_wzrd_probe()
where the exact same notifier_block is registered for both clk_in1 and
axi_clk:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_probe() {
	...
	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);
	...
}

Does this corrupt the SRCU notifier linked list? A notifier_block has only
one next pointer and cannot be added to multiple SRCU notifier chains
simultaneously (or twice to the same chain).

If the clocks are different, nb->next would be overwritten, merging the
chains and corrupting traversal. If the clocks are the same, registering
the same block twice creates a circular linked list (n->next = n), which
can cause an infinite loop and hard lockup during clock rate changes.

Could we add a second notifier_block to struct clk_wzrd to handle the
axi_clk registration separately?

> +		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/20260731072049.957624-1-shubhrajyoti.datta@amd.com?part=5