* [PATCH] clk: x86: fch: validate registrations and manage their lifetime
@ 2026-09-13 12:52 Slavin Liu
2026-09-24 20:54 ` Brian Masney
0 siblings, 1 reply; 2+ messages in thread
From: Slavin Liu @ 2026-09-13 12:52 UTC (permalink / raw)
To: sboyd, bmasney+clk, jbrunet+clk; +Cc: linux-clk, linux-kernel, bolin.liu
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",
+ 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;
+
+ 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;
-
- 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);
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH] clk: x86: fch: validate registrations and manage their lifetime
2026-09-13 12:52 [PATCH] clk: x86: fch: validate registrations and manage their lifetime Slavin Liu
@ 2026-09-24 20:54 ` Brian Masney
0 siblings, 0 replies; 2+ messages in thread
From: Brian Masney @ 2026-09-24 20:54 UTC (permalink / raw)
To: Slavin Liu; +Cc: sboyd, bmasney+clk, jbrunet+clk, linux-clk, linux-kernel
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);
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-09-24 20:54 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-13 12:52 [PATCH] clk: x86: fch: validate registrations and manage their lifetime Slavin Liu
2026-09-24 20:54 ` Brian Masney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®