Re: [PATCH v2 1/2] i3c: master: svc: Add amlogic a9 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i3c: master: svc: NULL dereference on unchecked devm_kcalloc() in aml_ao_i3c_tail_clocks()
- [High] i3c: master: svc: unclocked MMIO access to AML_AO_I3C_FCLK_DIV_CTRL in aml_i3c_init()
- [Low] i3c: master: svc: missing COMMON_CLK dependency for aml_ao_i3c_tail_clocks()
--

commit 5d539aa0589681cba0b31f37c581a2a552c6d504
Author: Junyi Zhao <junyi.zhao@amlogic.com>

i3c: master: svc: Add amlogic a9 support

This commit adds support for the Silvaco I3C master controller on Amlogic
a9 SoCs (both EE-domain and AO-domain variants). It introduces a
vendor_init callback and vendor_quirks to register the AO-domain internal
clock divider chain and configure the fast_clk rate during probe.

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index e700392f0801f..98aa6eb0f49ca 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>

[ ... ]

> @@ -2019,6 +2043,12 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  	if (master->num_clks < 0)
>  		return dev_err_probe(dev, -EINVAL, "can't get I3C clocks\n");
>  
> +	if (master->drvdata->vendor_init) {
> +		ret = master->drvdata->vendor_init(master);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for (i = 0; i < master->num_clks; i++) {
>  		if (!strcmp(master->clks[i].id, "fast_clk"))
>  			break;
> @@ -2036,7 +2066,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  	if (master->irq < 0)
>  		return master->irq;
>  
> -	master->dev = dev;
>  	ret = clk_bulk_prepare_enable(master->num_clks, master->clks);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "can't enable I3C clocks\n");
> @@ -2163,9 +2192,184 @@ static const struct svc_i3c_drvdata npcm845_drvdata = {
>  
>  static const struct svc_i3c_drvdata svc_default_drvdata = {};
>  
> +/*
> + * Container for the tail clocks of the AO domain variant. The clock HW
> + * structures are registered with the clock framework and must stay alive
> + * for the whole driver lifetime, so they are kept along with the shared
> + * lock used by their gate/divider operations.
> + */
> +struct aml_i3c_ao_clk {
> +	struct clk_gate div_en;
> +	struct clk_divider div;
> +	struct clk_gate gate;
> +	spinlock_t lock;
> +};
> +
> +/*
> + * aml_ao_i3c_tail_clocks - register the internal divider chain that
> + * produces the AO variant's fast_clk, and append it to the clk bulk
> + * array as "fast_clk".  @parent must be "pclk", the only clock the AO
> + * variant exposes (see silvaco,i3c-master.yaml).
> + */
> +static int aml_ao_i3c_tail_clocks(struct svc_i3c_master *master, struct clk *parent)
> +{
> +	struct device *dev = master->dev;
> +	struct aml_i3c_ao_clk *ao;
> +	struct clk_init_data init = {};
> +	char name[64];
> +	int ret;
> +	struct clk_bulk_data *new_clks;
> +	struct clk *fclk;
> +
> +	ao = devm_kzalloc(dev, sizeof(*ao), GFP_KERNEL);
> +	if (!ao)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&ao->lock);
> +
> +	snprintf(name, sizeof(name), "%s#div_en", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_gate_ops;
> +	init.flags = CLK_IGNORE_UNUSED;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = __clk_get_hw(parent),
> +	};
> +	init.num_parents = 1;
> +
> +	ao->div_en.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->div_en.bit_idx = AML_AO_I3C_FCLK_DIV_EN_SHIFT;
> +	ao->div_en.hw.init = &init;
> +	ao->div_en.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->div_en.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	snprintf(name, sizeof(name), "%s#div", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_divider_ops;
> +	init.flags = 0;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = &ao->div_en.hw,
> +	};
> +	init.num_parents = 1;
> +
> +	ao->div.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->div.shift = AML_AO_I3C_FCLK_DIV_SHIFT;
> +	ao->div.width = AML_AO_I3C_FCLK_DIV_WIDTH;
> +	ao->div.hw.init = &init;
> +	ao->div.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->div.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	snprintf(name, sizeof(name), "%s#gate", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_gate_ops;
> +	init.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED;
> +	init.parent_data = &(struct clk_parent_data) {
> +		.index = -1,
> +		.hw = &ao->div.hw,
> +	};
> +	init.num_parents = 1;
> +
> +	ao->gate.reg = master->regs + AML_AO_I3C_FCLK_DIV_CTRL;
> +	ao->gate.bit_idx = AML_AO_I3C_FCLK_EN_SHIFT;
> +	ao->gate.hw.init = &init;
> +	ao->gate.lock = &ao->lock;
> +
> +	ret = devm_clk_hw_register(dev, &ao->gate.hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register %s\n", name);
> +
> +	fclk = devm_clk_hw_get_clk(dev, &ao->gate.hw, NULL);
> +	if (IS_ERR(fclk))
> +		return dev_err_probe(dev, PTR_ERR(fclk),
> +				     "tail i3c fast clock failed\n");

[Severity: Low]
Will this fail to link when the common clock framework is disabled?

aml_ao_i3c_tail_clocks() unconditionally references clk_gate_ops,
clk_divider_ops, devm_clk_hw_register(), and devm_clk_hw_get_clk(), which do
not have stub definitions in include/linux/clk-provider.h when common clock
support is not enabled.

In drivers/i3c/master/Kconfig, the driver currently has:

    config SVC_I3C_MASTER
        tristate "Silvaco I3C Dual-Role Master driver"
        depends on HAS_IOMEM
        depends on !(ALPHA || PARISC)

Should the Kconfig entry also depend on common clock support?

> +
> +	new_clks = devm_kcalloc(dev,
> +				master->num_clks + 1,
> +				sizeof(*new_clks),
> +				GFP_KERNEL);
> +	memcpy(new_clks, master->clks,
> +			master->num_clks * sizeof(*master->clks));
> +	new_clks[master->num_clks].id = "fast_clk";
> +	new_clks[master->num_clks].clk = fclk;

[Severity: High]
What happens if devm_kcalloc() returns null in aml_ao_i3c_tail_clocks()?

new_clks is passed directly to memcpy() and dereferenced at
new_clks[master->num_clks] without checking whether the allocation succeeded.
Should this return -ENOMEM if !new_clks?

> +	master->clks = new_clks;
> +	master->num_clks++;
> +
> +	return 0;
> +}
> +
> +static int aml_i3c_init(struct svc_i3c_master *master)
> +{
> +	struct device *dev = master->dev;
> +	long fin_freq;
> +	int ret, i;
> +	struct clk *fclk;
> +
> +	if (vendor_has_quirk(master, AML_I3C_QUIRK_AO)) {
> +		/*
> +		 * clk_bulk_get_all() preserves the DT order, and the
> +		 * binding restricts the AO variant to exactly one clock,
> +		 * which is always "pclk".  So clks[0] is the input of the
> +		 * internal divider chain.
> +		 */
> +		ret = aml_ao_i3c_tail_clocks(master, master->clks[0].clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < master->num_clks; i++) {
> +		if (!strcmp(master->clks[i].id, "fast_clk"))
> +			break;
> +	}
> +
> +	if (i == master->num_clks)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "can't get I3C fast clock\n");
> +
> +	fclk = master->clks[i].clk;
> +	if (IS_ERR(fclk))
> +		return PTR_ERR(fclk);
> +
> +	fin_freq = clk_round_rate(fclk, 25000000);
> +	if (fin_freq <= 0) {
> +		dev_err(dev, "invalid fast_clk\n");
> +		return fin_freq ? fin_freq : -EINVAL;
> +	}
> +
> +	ret = clk_set_rate(fclk, fin_freq);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "setting fast_clk rate failed\n");

[Severity: High]
On the AO variant, is it safe to access master->regs + AML_AO_I3C_FCLK_DIV_CTRL
before pclk is prepared and enabled?

In svc_i3c_master_probe(), master->drvdata->vendor_init(master) is called right
after devm_clk_bulk_get_all(), before clk_bulk_prepare_enable() enables pclk
and before pm_runtime_enable() is called:

    svc_i3c_master_probe()
        devm_clk_bulk_get_all()
        aml_i3c_init()
            aml_ao_i3c_tail_clocks()
                devm_clk_hw_register(&ao->div.hw)
                    __clk_core_init()
                        clk_divider_recalc_rate()
            clk_set_rate(fclk, fin_freq)
                clk_divider_set_rate()
        ...
        clk_bulk_prepare_enable()
        ...
        pm_runtime_enable()

Both devm_clk_hw_register(&ao->div.hw) and clk_set_rate(fclk, fin_freq) read
and write master->regs + AML_AO_I3C_FCLK_DIV_CTRL while pclk is still
disabled, and none of the registered AO clocks set CLK_OPS_PARENT_ENABLE.

In addition, because devm_clk_hw_register() runs before pm_runtime_enable(),
runtime PM is not bound to the registered clocks, so reading debugfs
clk_summary while the controller is runtime-suspended (and pclk is gated by
svc_i3c_runtime_suspend()) will also read master->regs +
AML_AO_I3C_FCLK_DIV_CTRL with pclk disabled.

Should pclk be enabled before registering and programming the internal divider
clocks (or should the AO clocks use CLK_OPS_PARENT_ENABLE / runtime PM)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-a9_i3c-v2-0-0c474bfccffa@amlogic.com?part=1