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

On 7/9/2026 4:58 PM, [email protected] wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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 <[email protected]>
>
> 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?
This is fixed in later in the series

[PATCH v2 5/8] clk: clocking-wizard: Skip s_axi_aclk for static-config - 
Shubhrajyoti Datta <https://lore.kernel.org/all/[email protected]/>


>
>> @@ -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?
[PATCH v2 4/8] clk: clocking-wizard: Add static-config clock provider 
support - Shubhrajyoti Datta 
<https://lore.kernel.org/all/[email protected]/>
Will address it later in the series
>
> [ ... ]
>
>>                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?
Tried to fix it
[PATCH v2 8/8] clk: clocking-wizard: Use separate notifier_block for 
each clock - Shubhrajyoti Datta 
<https://lore.kernel.org/all/[email protected]/>

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2