Re: [PATCH v2 10/13] clk: berlin: add Synaptics SL2610 clocks and PLLs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] clk: berlin: dereference of dev before NULL check in
  sl2610_pll_register()
- [Medium] clk: berlin: irq lock inversion on sl_sysmgr_gateclk_lock via
  sl2610_sysmgr_clk_setup()
- [Low] clk: berlin: fix apbuart3clk and apbi2c1clk typos in
  sl2610_gates
- [Low] clk: berlin: missing OF and HAS_IOMEM dependencies on SL2610_PLL
- [Low] clk: berlin: unused SL2610_APLL macro and struct forward
  declarations
--

commit 9456f61cc7fffcd42790e27e9671ad341c8cf8cd
Author: Jisheng Zhang <jszhang@kernel.org>

clk: berlin: add Synaptics SL2610 clocks and PLLs

This commit adds clock and PLL drivers for the Synaptics SL2610 SoC,
registering the SoC, CPU, AVIO, and MCU clocks.

> diff --git a/drivers/clk/berlin/Kconfig b/drivers/clk/berlin/Kconfig
> index 8f71d4c7a478b..be7e47e0ff523 100644
> --- a/drivers/clk/berlin/Kconfig
> +++ b/drivers/clk/berlin/Kconfig
> @@ -5,3 +5,30 @@ config SL_COMMON_CLK
>  
>  config SL_SYSMGR_CLK
>  	bool
> +
> +config SL2610_PLL
> +	tristate "Synaptics SL2610 PLL Driver"
> +	depends on ARCH_BERLIN || COMPILE_TEST
> +	default n
> +	help
> +	  Support the integer and fractional PLL outputs used by the SoC,
> +	  CPU and audio clock controllers on Synaptics SL2610 SoCs.
> +
> +	  To compile this driver as a module, choose M here. The module will
> +	  be called sl2610-pll.
> +
> +config SL2610_CLK
> +	tristate "Synaptics SL2610 Clock Driver"
> +	depends on ARCH_BERLIN || COMPILE_TEST
> +	depends on OF && HAS_IOMEM

[Severity: Low]
Should SL2610_PLL also depend on OF && HAS_IOMEM like SL2610_CLK?
sl2610-pll.c calls readl_relaxed() and of_clk_get().

Also, both symbols allow COMPILE_TEST when ARCH_BERLIN=n, but
drivers/clk/Makefile only descends into berlin/ for CONFIG_ARCH_BERLIN, so
neither driver is built under COMPILE_TEST=y with ARCH_BERLIN=n.

> diff --git a/drivers/clk/berlin/clk-sl2610.c b/drivers/clk/berlin/clk-sl2610.c
> new file mode 100644
> index 0000000000000..b51d3523028de
> --- /dev/null
> +++ b/drivers/clk/berlin/clk-sl2610.c

[ ... ]

> +static const struct sl_gateclk_desc sl2610_gates[] = {
> +	{ "usb0coreclk",	"perifsysclk",	CLK_USB0CORE,		0 },
> +	{ "sdiosysclk",		"perifsysclk",	CLK_SDIOSYS,		1 },
> +	{ "emmcsysclk",		"perifsysclk",	CLK_EMMCSYS,		2 },
> +	{ "gpuaxiclk",		"hpcclk",	CLK_GPUAXI,		3 },
> +	{ "gethrgmiisysclk",	"perifsysclk",	CLK_GETHRGMIISYS,	4 },
> +	{ "sdio1sysclk",	"perifsysclk",	CLK_SDIO1SYS,		5 },
> +	{ "usb1coreclk",	"perifsysclk",	CLK_USB1CORE,		6 },
> +	{ "gethrgmii1sysclk",	"perifsysclk",	CLK_GETHRGMII1SYS,	7 },
> +	{ "usb0phyrefclk",	"perifsysclk",	CLK_USB0PHYREF,		8 },
> +	{ "usb1phyrefclk",	"perifsysclk",	CLK_USB1PHYREF,		9 },
> +	{ "apbuart0clk",	"apbcoreclk",	CLK_APBUART0,		10 },
> +	{ "apbuart1clk",	"apbcoreclk",	CLK_APBUART1,		11 },
> +	{ "apbuart2clk",	"apbcoreclk",	CLK_APBUART2,		12 },
> +	{ "apbuart3lck",	"apbcoreclk",	CLK_APBUART3,		13 },
> +	{ "apbi2c0clk",		"apbcoreclk",	CLK_APBI2C0,		14 },
> +	{ "apbi2c1lck",		"apbcoreclk",	CLK_APBI2C1,		15 },

[Severity: Low]
Are "apbuart3lck" and "apbi2c1lck" typos for "apbuart3clk" and
"apbi2c1clk"?  Every other entry in sl2610_gates[] ends with "clk", so
these two clocks are registered under misspelled names in clk_summary and
any lookup by name.

[ ... ]

> +static int sl2610_sysmgr_clk_setup(struct platform_device *pdev)
> +{
> +	return sl_sysmgr_clk_setup(pdev, sl2610_sysmgr_fixeds, ARRAY_SIZE(sl2610_sysmgr_fixeds),
> +				   sl2610_sysmgr_gates, ARRAY_SIZE(sl2610_sysmgr_gates),
> +				   CLK_GATE_CTRL0,
> +				   sl2610_sysmgr_clks, ARRAY_SIZE(sl2610_sysmgr_clks));
> +}

[Severity: Medium]
Can registering the MCU gate clocks via sl_sysmgr_clk_setup() cause an IRQ
lock inversion or deadlock on sl_sysmgr_gateclk_lock?

In commit "clk: berlin: add Synaptics clock controller helpers",
sl_sysmgr_gateclk_enable() and sl_sysmgr_gateclk_is_enabled() acquire
sl_sysmgr_gateclk_lock with guard(spinlock) instead of
guard(spinlock_irqsave):

drivers/clk/berlin/sl-sysmgr-clk.c:sl_sysmgr_gateclk_is_enabled() {
	struct sl_sysmgr_gateclk *gclk = to_sl_sysmgr_gateclk(hw);
	u32 reg;

	guard(spinlock)(&sl_sysmgr_gateclk_lock);
	reg = readl_relaxed(gclk->reg);
    ...
}

clk_enable() holds the CCF enable_lock with interrupts disabled (and may
run in hardirq context) before calling .enable, whereas
clk_summary_show_one() and clk_hw_is_enabled() call .is_enabled from
process context without enable_lock and with interrupts enabled:

CPU0                              CPU1
----                              ----
clk_enable()
  clk_enable_lock()
    spin_lock_irqsave(&enable_lock)
                                  clk_summary_show_one()
                                    clk_core_is_enabled()
                                      sl_sysmgr_gateclk_is_enabled()
                                        spin_lock(&sl_sysmgr_gateclk_lock)
  sl_sysmgr_gateclk_enable()
    spin_lock(&sl_sysmgr_gateclk_lock)
                                        <hardirq>
                                          clk_enable()
                                            clk_enable_lock()
                                              spin_lock_irqsave(&enable_lock)

 *** DEADLOCK ***

A single-CPU deadlock also occurs if the interrupt on the same CPU calls
clk_enable() on an MCU gate clock while sl_sysmgr_gateclk_is_enabled()
holds sl_sysmgr_gateclk_lock.  Using guard(spinlock_irqsave) (or reading
gclk->reg locklessly in sl_sysmgr_gateclk_is_enabled() as sl_clk_is_enabled()
does) avoids this.

> diff --git a/drivers/clk/berlin/sl2610-pll.c b/drivers/clk/berlin/sl2610-pll.c
> new file mode 100644
> index 0000000000000..62c5d2d19c362
> --- /dev/null
> +++ b/drivers/clk/berlin/sl2610-pll.c

[ ... ]

> +#define CTRLG			0x18
> +#define DIVQF_MASK		GENMASK(2, 0)
> +
> +#define SL2610_APLL
> +struct sl2610_pll {
> +	void __iomem *ctrl;
> +	struct clk_hw pllout;
> +	struct clk_hw plloutf;
> +};

[ ... ]

> +int sl2610_pll_register(struct device *dev, void __iomem *ctrl,
> +			const char *out, const char *outf)
> +{
> +	struct device_node *np = dev->parent->of_node;
> +	struct clk_parent_data parent = {};
> +	struct clk_init_data init = {};
> +	struct sl2610_pll *pll;
> +	struct clk *input;
> +	int ret;
> +
> +	if (!dev || !ctrl || !out || !outf)
> +		return -EINVAL;

[Severity: Medium]
Does sl2610_pll_register() dereference dev and dev->parent before the !dev
check?  If dev or dev->parent is NULL, initializing np at the top of the
function causes a NULL pointer dereference instead of returning -EINVAL.
sl2610_soc_clk_setup(), sl2610_cpu_clk_setup(), and sl2610_aio_clk_setup()
in drivers/clk/berlin/clk-sl2610.c also dereference dev->parent->of_node
without checking dev->parent.

> diff --git a/drivers/clk/berlin/sl2610-pll.h b/drivers/clk/berlin/sl2610-pll.h
> new file mode 100644
> index 0000000000000..b61a93b28610d
> --- /dev/null
> +++ b/drivers/clk/berlin/sl2610-pll.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SL2610_PLL_H
> +#define __SL2610_PLL_H
> +
> +#include <linux/io.h>
> +
> +struct clk_hw_onecell_data;
> +struct device;
> +struct device_node;

[Severity: Low]
This isn't a bug, but struct clk_hw_onecell_data and struct device_node in
sl2610-pll.h and the empty SL2610_APLL macro in sl2610-pll.c are unused and
can be dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115102401.3520-1-jszhang@kernel.org?part=10