Re: [RESEND v1] clk: imx95-blk-ctl: Add func_out_en clock for i.MX9x PCIe

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

New issues:
- [Medium] The newly added clock is named `func_out_en`, which is too generic for the global Common Clock Framework (CCF) namespace.

Pre-existing issues:
- [High] The driver lacks a `.remove` callback but does not suppress sysfs unbinding, causing use-after-free and I/O page faults on unbind.
- [High] The `netcmix_dev_data` specifies an incorrect `.clk_reg_offset`, corrupting Ethernet clock state on suspend/resume.
--

--- Patch [1]: [RESEND v1] clk: imx95-blk-ctl: Add func_out_en clock for i.MX9x PCIe ---
commit 5827f093c090c0f0800defab20d47c7314cbe3c9
Author: Richard Zhu <[email protected]>

clk: imx95-blk-ctl: Add func_out_en clock for i.MX9x PCIe

The patch adds a func_out_en clock gate that serves as the parent of the 
existing CREF_EN gate clock for i.MX9x PCIe, reducing unnecessary power 
consumption.

> diff --git a/drivers/clk/imx/clk-imx95-blk-ctl.c b/drivers/clk/imx/clk-imx95-blk-ctl.c
> index 56bed44719954..1f9259f456079 100644
> --- a/drivers/clk/imx/clk-imx95-blk-ctl.c
> +++ b/drivers/clk/imx/clk-imx95-blk-ctl.c

[ ... ]

> @@ -286,18 +286,28 @@ static const struct imx95_blk_ctl_dev_data netcmix_dev_data = {

[Severity: High]
This is a pre-existing issue, but does netcmix_dev_data specify an
incorrect .clk_reg_offset that could corrupt Ethernet clock state on
suspend and resume?

The clocks for netcmix_dev_data are all at offset 4:

drivers/clk/imx/clk-imx95-blk-ctl.c:netxmix_clk_dev_data[] = {
	[0] = {
		...
		.reg = 4,

However, .clk_reg_offset is initialized to 0:

drivers/clk/imx/clk-imx95-blk-ctl.c:netcmix_dev_data = {
	.num_clks = ARRAY_SIZE(netxmix_clk_dev_data),
	.clk_dev_data = netxmix_clk_dev_data,
	.clk_reg_offset = 0,
};

This offset is used by the suspend and resume routines to save and restore
the clock register state:

drivers/clk/imx/clk-imx95-blk-ctl.c:imx95_bc_runtime_suspend() {
	...
	bc->clk_reg_restore = readl(bc->base + bc->pdata->clk_reg_offset);
	...
}

Will this cause the driver to save and restore register 0 instead of
register 4, causing Ethernet RMII clock muxes to lose their configured state?

>  static const struct imx95_blk_ctl_clk_dev_data hsio_blk_ctl_clk_dev_data[] = {
>  	[0] = {
>  		.name = "hsio_blk_ctl_clk",
> -		.parent_names = (const char *[]){ "hsio_pll", },
> +		.parent_names = (const char *[]){ "func_out_en", },
>  		.num_parents = 1,
>  		.reg = 0,
>  		.bit_idx = 6,
>  		.bit_width = 1,
>  		.type = CLK_GATE,
>  		.flags = CLK_SET_RATE_PARENT,
> +	},
> +	[1] = {
> +		.name = "func_out_en",

[Severity: Medium]
Will this name conflict with other clocks in the global Common Clock
Framework (CCF) namespace?

Because "func_out_en" lacks a subsystem or block-specific prefix
(like "hsio_func_out_en"), it might cause namespace collisions if another
hardware block registers a clock with the same generic name.

> +		.parent_names = (const char *[]){ "hsio_pll", },
> +		.num_parents = 1,
> +		.reg = 0,
> +		.bit_idx = 2,
> +		.bit_width = 1,
> +		.type = CLK_GATE,
> +		.flags = CLK_SET_RATE_PARENT,
>  	}
>  };
>  
>  static const struct imx95_blk_ctl_dev_data hsio_blk_ctl_dev_data = {
> -	.num_clks = 1,
> +	.num_clks = ARRAY_SIZE(hsio_blk_ctl_clk_dev_data),
>  	.clk_dev_data = hsio_blk_ctl_clk_dev_data,
>  	.clk_reg_offset = 0,
>  };

[Severity: High]
This isn't a bug introduced by this patch, but could this driver cause a
use-after-free and I/O page faults on unbind?

The driver registers non-devm clock providers and clocks in imx95_bc_probe():

drivers/clk/imx/clk-imx95-blk-ctl.c:imx95_bc_probe() {
	...
		} else {
			hws[i] = clk_hw_register_gate(dev, data->name, data->parent_names[0],
						      data->flags, reg, data->bit_idx,
						      data->flags2, &bc->lock);
		}
	...
	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, clk_hw_data);
	...
}

However, it does not provide a .remove callback or suppress sysfs unbinding:

drivers/clk/imx/clk-imx95-blk-ctl.c:imx95_bc_driver = {
	.probe = imx95_bc_probe,
	.driver = {
		.name = "imx95-blk-ctl",
		.of_match_table = imx95_bc_of_match,
		.pm = &imx95_bc_pm_ops,
	},
};

If the driver is unbound, devres will automatically free the driver state and
unmap I/O memory, but the clocks remain registered with CCF.  Will subsequent
CCF operations access freed memory and unmapped registers?

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