Re: [PATCH] clk: x86: fch: validate registrations and manage their lifetime

[Brian Masney <bmasney@redhat.com>]

Hi Slavin,

On Sun, Sep 13, 2026 at 08:52:39PM +0800, Slavin Liu wrote:
> Fixed-rate and mux registration can fail before clk_set_parent()
> evaluates their clk members. Check each registration and the later
> parent/clkdev operations. Use managed clock registration so every new
> error exit unwinds only successfully registered clocks in reverse order.
> Keep the hardware-clock array local and drop the manual remove callback
> to avoid unregistering managed clocks twice.
> 
> Detected by static analysis and reviewed with AI-assisted source auditing.
> 
> Fixes: 421bf6a1f061 ("clk: x86: Add ST oscout platform clock")
> Assisted-by: LLM
> Signed-off-by: Slavin Liu <bolin.liu@seu.edu.cn>
> ---
>  drivers/clk/x86/clk-fch.c | 109 ++++++++++++++++++++++----------------
>  1 file changed, 62 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/clk/x86/clk-fch.c b/drivers/clk/x86/clk-fch.c
> index cf5cd3ad4647..b66ede14fc22 100644
> --- a/drivers/clk/x86/clk-fch.c
> +++ b/drivers/clk/x86/clk-fch.c
> @@ -35,7 +35,6 @@
>  #define AMD_CPU_ID_ST                  0x1576
>  
>  static const char * const clk_oscout1_parents[] = { "clk48MHz", "clk25MHz" };
> -static struct clk_hw *hws[ST_MAX_CLKS];
>  
>  static const struct pci_device_id fch_pci_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_ST) },
> @@ -44,8 +43,10 @@ static const struct pci_device_id fch_pci_ids[] = {
>  
>  static int fch_clk_probe(struct platform_device *pdev)
>  {
> +	struct clk_hw *hws[ST_MAX_CLKS];
>  	struct fch_clk_data *fch_data;
>  	struct pci_dev *rdev;
> +	int ret;
>  
>  	fch_data = dev_get_platdata(&pdev->dev);
>  	if (!fch_data || !fch_data->base)
> @@ -58,55 +59,70 @@ static int fch_clk_probe(struct platform_device *pdev)
>  	}
>  
>  	if (pci_match_id(fch_pci_ids, rdev)) {
> -		hws[ST_CLK_48M] = clk_hw_register_fixed_rate(NULL, "clk48MHz",
> -			NULL, 0, 48000000);
> -		hws[ST_CLK_25M] = clk_hw_register_fixed_rate(NULL, "clk25MHz",
> -			NULL, 0, 25000000);
> -
> -		hws[ST_CLK_MUX] = clk_hw_register_mux(NULL, "oscout1_mux",
> -			clk_oscout1_parents, ARRAY_SIZE(clk_oscout1_parents),
> -			0, fch_data->base + CLKDRVSTR2, OSCOUT1CLK25MHZ, 3, 0,
> -			NULL);
> -
> -		clk_set_parent(hws[ST_CLK_MUX]->clk, hws[ST_CLK_48M]->clk);
> -
> -		hws[ST_CLK_GATE] = clk_hw_register_gate(NULL, "oscout1",
> -			"oscout1_mux", 0, fch_data->base + MISCCLKCNTL1,
> -			OSCCLKENB, CLK_GATE_SET_TO_DISABLE, NULL);
> -
> -		devm_clk_hw_register_clkdev(&pdev->dev, hws[ST_CLK_GATE],
> -					    fch_data->name, NULL);
> +		hws[ST_CLK_48M] = devm_clk_hw_register_fixed_rate(&pdev->dev, "clk48MHz",
> +								  NULL, 0, 48000000);
> +		if (IS_ERR(hws[ST_CLK_48M])) {
> +			ret = PTR_ERR(hws[ST_CLK_48M]);
> +			goto out_put;
> +		}
> +		hws[ST_CLK_25M] = devm_clk_hw_register_fixed_rate(&pdev->dev, "clk25MHz",

Should the new IS_ERR() checks be it's own separate commit with the
Fixes tag? Then put the devm conversion in a separate commit without the
Fixes tag? I'm just thinking about ways to make these patches smaller so
that there's less to backport into the stable kernels.

> +								  NULL, 0, 25000000);
> +		if (IS_ERR(hws[ST_CLK_25M])) {
> +			ret = PTR_ERR(hws[ST_CLK_25M]);
> +			goto out_put;
> +		}
> +
> +		hws[ST_CLK_MUX] =
> +			devm_clk_hw_register_mux(&pdev->dev, "oscout1_mux",
> +						 clk_oscout1_parents,
> +						 ARRAY_SIZE(clk_oscout1_parents),
> +						 0, fch_data->base + CLKDRVSTR2, OSCOUT1CLK25MHZ, 3,
> +						 0,
> +						 NULL);
> +		if (IS_ERR(hws[ST_CLK_MUX])) {
> +			ret = PTR_ERR(hws[ST_CLK_MUX]);
> +			goto out_put;
> +		}
> +
> +		ret = clk_set_parent(hws[ST_CLK_MUX]->clk, hws[ST_CLK_48M]->clk);
> +		if (ret)
> +			goto out_put;

It's worth noting in the commit log that if clk_set_parent fails, then
probing will fail. This is fine but it's a behavior change that's worth
calling out.

> +
> +		hws[ST_CLK_GATE] =
> +			devm_clk_hw_register_gate(&pdev->dev, "oscout1",
> +						  "oscout1_mux", 0, fch_data->base + MISCCLKCNTL1,
> +						  OSCCLKENB, CLK_GATE_SET_TO_DISABLE, NULL);
> +		if (IS_ERR(hws[ST_CLK_GATE])) {
> +			ret = PTR_ERR(hws[ST_CLK_GATE]);
> +			goto out_put;
> +		}
> +
> +		ret = devm_clk_hw_register_clkdev(&pdev->dev, hws[ST_CLK_GATE],
> +						  fch_data->name, NULL);
>  	} else {
> -		hws[CLK_48M_FIXED] = clk_hw_register_fixed_rate(NULL, "clk48MHz",
> -			NULL, 0, 48000000);
> -
> -		hws[CLK_GATE_FIXED] = clk_hw_register_gate(NULL, "oscout1",
> -			"clk48MHz", 0, fch_data->base + MISCCLKCNTL1,
> -			OSCCLKENB, 0, NULL);
> -
> -		devm_clk_hw_register_clkdev(&pdev->dev, hws[CLK_GATE_FIXED],
> -					    fch_data->name, NULL);
> +		hws[CLK_48M_FIXED] = devm_clk_hw_register_fixed_rate(&pdev->dev, "clk48MHz",
> +								     NULL, 0, 48000000);
> +		if (IS_ERR(hws[CLK_48M_FIXED])) {
> +			ret = PTR_ERR(hws[CLK_48M_FIXED]);
> +			goto out_put;
> +		}
> +
> +		hws[CLK_GATE_FIXED] =
> +			devm_clk_hw_register_gate(&pdev->dev, "oscout1",
> +						  "clk48MHz", 0, fch_data->base + MISCCLKCNTL1,
> +						  OSCCLKENB, 0, NULL);
> +		if (IS_ERR(hws[CLK_GATE_FIXED])) {
> +			ret = PTR_ERR(hws[CLK_GATE_FIXED]);
> +			goto out_put;
> +		}
> +
> +		ret = devm_clk_hw_register_clkdev(&pdev->dev, hws[CLK_GATE_FIXED],
> +						  fch_data->name, NULL);
>  	}
>  
> +out_put:
>  	pci_dev_put(rdev);
> -	return 0;
> -}
> -
> -static void fch_clk_remove(struct platform_device *pdev)
> -{
> -	int i, clks;
> -	struct pci_dev *rdev;
> -
> -	rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> -	if (!rdev)
> -		return;
> -
> -	clks = pci_match_id(fch_pci_ids, rdev) ? CLK_MAX_FIXED : ST_MAX_CLKS;

CLK_MAX_FIXED should be dropped now that it's unused.

Brian


> -
> -	for (i = 0; i < clks; i++)
> -		clk_hw_unregister(hws[i]);
> -
> -	pci_dev_put(rdev);
> +	return ret;
>  }
>  
>  static struct platform_driver fch_clk_driver = {
> @@ -115,6 +131,5 @@ static struct platform_driver fch_clk_driver = {
>  		.suppress_bind_attrs = true,
>  	},
>  	.probe = fch_clk_probe,
> -	.remove = fch_clk_remove,
>  };
>  builtin_platform_driver(fch_clk_driver);