diff mbox series

[v4,06/11] rtc: sun6i: add Allwinner B288 compatible

Message ID 20261012100816.331033-7-pmazzini@gmail.com (mailing list archive)
State New
Headers
Series ARM: sunxi: add Allwinner B288 and the PocketBook Verse |

Commit Message

Pablo Mazzini Oct. 12, 2026, 10:08 a.m. UTC 
The 32 kHz clocks are the same as the A23's. Only one alarm interrupt
is wired, which is all the driver uses.

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/rtc/rtc-sun6i.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

sashiko-bot@kernel.org Oct. 12, 2026, 10:19 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: sun6i: missing ERR_PTR check causes crash in probe on clk_register failure
- [Medium] rtc: sun6i: resource leak of losc clock on initialization error path
- [Low] rtc: sun6i: memory leak of rtc structure in sun6i_rtc_clk_init error path
--

commit 903d00f6b5cb6afede9ad6ad5fca01853d776ce4
Author: Pablo Mazzini <pmazzini@gmail.com>
Subject: rtc: sun6i: add Allwinner B288 compatible

This commit adds support for the Allwinner B288 RTC by reusing the A23
clock configuration. It maps the new compatible string
allwinner,sun8i-b288-rtc to the existing initialization functions.

> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 7fc60880e4094..05004e80ac089 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -350,6 +350,10 @@ static void __init sun8i_a23_rtc_clk_init(struct device_node *node)
>  CLK_OF_DECLARE_DRIVER(sun8i_a23_rtc_clk, "allwinner,sun8i-a23-rtc",
>  		      sun8i_a23_rtc_clk_init);
>  
> +/* The B288 RTC clocks are the same as the A23's */
> +CLK_OF_DECLARE_DRIVER(sun8i_b288_rtc_clk, "allwinner,sun8i-b288-rtc",
> +		      sun8i_a23_rtc_clk_init);
> +

[Severity: High]
Does adding this compatible string expose an issue in the error handling
of sun6i_rtc_clk_init on registration failure?

If clk_register fails for rtc->losc, the function returns an error
pointer and jumps to the err_register label. Since the global sun6i_rtc
pointer is already assigned to the allocated rtc struct earlier:

sun6i_rtc_clk_init()
    /* Yes, I know, this is ugly. */
    sun6i_rtc = rtc;
    ...
    rtc->losc = clk_register(NULL, &rtc->hw);
    if (IS_ERR(rtc->losc)) {
        pr_crit("Couldn't register the LOSC clock\n");
        goto err_register;
    }

The probe function sun6i_rtc_probe later fetches this dangling pointer:

sun6i_rtc_probe()
    struct sun6i_rtc_dev *chip = sun6i_rtc;
    ...
    clk_prepare_enable(chip->losc);

Can this lead to a kernel panic when clk_prepare_enable attempts to
dereference the error pointer stored in chip->losc?

[Severity: Medium]
Additionally, if clk_register succeeds for rtc->losc but a later clock
registration fails, triggering the jump to err_register, does this
cleanup path leak the rtc->losc clock?

sun6i_rtc_clk_init()
err_register:
    clk_hw_unregister_fixed_rate(rtc->int_osc);
err:
    kfree(clk_data);

The err_register path unregisters rtc->int_osc but appears to omit
calling clk_unregister(rtc->losc).

[Severity: Low]
Does the err cleanup path also leak the dynamically allocated rtc
structure?

sun6i_rtc_clk_init()
    rtc = kzalloc_obj(*rtc);
    ...
err:
    kfree(clk_data);

The error path frees clk_data but seems to omit freeing the allocated
rtc structure.

[ ... ]
Pablo Mazzini Oct. 14, 2026, 10:03 a.m. UTC | #2 
These are in sun6i_rtc_clk_init(), which this patch reuses unchanged for
the B288, so they apply to all the SoCs using it. I can look into it
separately.

On Mon, Oct 12, 2026 at 11:19 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] rtc: sun6i: missing ERR_PTR check causes crash in probe on clk_register failure
> - [Medium] rtc: sun6i: resource leak of losc clock on initialization error path
> - [Low] rtc: sun6i: memory leak of rtc structure in sun6i_rtc_clk_init error path
> --
>
> commit 903d00f6b5cb6afede9ad6ad5fca01853d776ce4
> Author: Pablo Mazzini <pmazzini@gmail.com>
> Subject: rtc: sun6i: add Allwinner B288 compatible
>
> This commit adds support for the Allwinner B288 RTC by reusing the A23
> clock configuration. It maps the new compatible string
> allwinner,sun8i-b288-rtc to the existing initialization functions.
>
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index 7fc60880e4094..05004e80ac089 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -350,6 +350,10 @@ static void __init sun8i_a23_rtc_clk_init(struct device_node *node)
> >  CLK_OF_DECLARE_DRIVER(sun8i_a23_rtc_clk, "allwinner,sun8i-a23-rtc",
> >                     sun8i_a23_rtc_clk_init);
> >
> > +/* The B288 RTC clocks are the same as the A23's */
> > +CLK_OF_DECLARE_DRIVER(sun8i_b288_rtc_clk, "allwinner,sun8i-b288-rtc",
> > +                   sun8i_a23_rtc_clk_init);
> > +
>
> [Severity: High]
> Does adding this compatible string expose an issue in the error handling
> of sun6i_rtc_clk_init on registration failure?
>
> If clk_register fails for rtc->losc, the function returns an error
> pointer and jumps to the err_register label. Since the global sun6i_rtc
> pointer is already assigned to the allocated rtc struct earlier:
>
> sun6i_rtc_clk_init()
>     /* Yes, I know, this is ugly. */
>     sun6i_rtc = rtc;
>     ...
>     rtc->losc = clk_register(NULL, &rtc->hw);
>     if (IS_ERR(rtc->losc)) {
>         pr_crit("Couldn't register the LOSC clock\n");
>         goto err_register;
>     }
>
> The probe function sun6i_rtc_probe later fetches this dangling pointer:
>
> sun6i_rtc_probe()
>     struct sun6i_rtc_dev *chip = sun6i_rtc;
>     ...
>     clk_prepare_enable(chip->losc);
>
> Can this lead to a kernel panic when clk_prepare_enable attempts to
> dereference the error pointer stored in chip->losc?
>
> [Severity: Medium]
> Additionally, if clk_register succeeds for rtc->losc but a later clock
> registration fails, triggering the jump to err_register, does this
> cleanup path leak the rtc->losc clock?
>
> sun6i_rtc_clk_init()
> err_register:
>     clk_hw_unregister_fixed_rate(rtc->int_osc);
> err:
>     kfree(clk_data);
>
> The err_register path unregisters rtc->int_osc but appears to omit
> calling clk_unregister(rtc->losc).
>
> [Severity: Low]
> Does the err cleanup path also leak the dynamically allocated rtc
> structure?
>
> sun6i_rtc_clk_init()
>     rtc = kzalloc_obj(*rtc);
>     ...
> err:
>     kfree(clk_data);
>
> The error path frees clk_data but seems to omit freeing the allocated
> rtc structure.
>
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012100816.331033-1-pmazzini@gmail.com?part=6
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 7fc60880e409..05004e80ac08 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -350,6 +350,10 @@  static void __init sun8i_a23_rtc_clk_init(struct device_node *node)
 CLK_OF_DECLARE_DRIVER(sun8i_a23_rtc_clk, "allwinner,sun8i-a23-rtc",
 		      sun8i_a23_rtc_clk_init);
 
+/* The B288 RTC clocks are the same as the A23's */
+CLK_OF_DECLARE_DRIVER(sun8i_b288_rtc_clk, "allwinner,sun8i-b288-rtc",
+		      sun8i_a23_rtc_clk_init);
+
 static const struct sun6i_rtc_clk_data sun8i_h3_rtc_data = {
 	.rc_osc_rate = 16000000,
 	.fixed_prescaler = 32,
@@ -859,6 +863,7 @@  static int sun6i_rtc_probe(struct platform_device *pdev)
 static const struct of_device_id sun6i_rtc_dt_ids[] = {
 	{ .compatible = "allwinner,sun6i-a31-rtc" },
 	{ .compatible = "allwinner,sun8i-a23-rtc" },
+	{ .compatible = "allwinner,sun8i-b288-rtc" },
 	{ .compatible = "allwinner,sun8i-h3-rtc" },
 	{ .compatible = "allwinner,sun8i-r40-rtc" },
 	{ .compatible = "allwinner,sun8i-v3-rtc" },