Re: [PATCH v2] clk: si521xx: correct the SI52147 OF output enable map

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds array access in si521xx_of_clk_get() when parsing device tree clock indices.
- [High] `regcache_sync()` during resume overwrites the direct I2C initialization of the Byte Count register, breaking subsequent I2C reads.
--

--- Patch [1]: [PATCH v2] clk: si521xx: correct the SI52147 OF output enable map ---
commit 415da2f3277cfd67c9fa71b04185cd6d8978b923
Author: Pengpeng Hou <[email protected]>
Subject: clk: si521xx: correct the SI52147 OF output enable map

This patch corrects the SI52147 OF match data to use 0x17 for output-enable
bits in control register 1. This aligns the OF map with the data sheet and
I2C ID table, exposing the documented outputs.

> diff --git a/drivers/clk/clk-si521xx.c b/drivers/clk/clk-si521xx.c
> index ceadc07bcb6df..d5be08e126858 100644
> --- a/drivers/clk/clk-si521xx.c
> +++ b/drivers/clk/clk-si521xx.c
> @@ -374,7 +374,7 @@ MODULE_DEVICE_TABLE(i2c, si521xx_id);
>  static const struct of_device_id clk_si521xx_of_match[] = {
>  	{ .compatible = "skyworks,si52144", .data = (void *)SI521XX_OE_MAP(0x5, 0xc0) },
>  	{ .compatible = "skyworks,si52146", .data = (void *)SI521XX_OE_MAP(0x15, 0xe0) },
> -	{ .compatible = "skyworks,si52147", .data = (void *)SI521XX_OE_MAP(0x15, 0xf8) },
> +	{ .compatible = "skyworks,si52147", .data = (void *)SI521XX_OE_MAP(0x17, 0xf8) },

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to an
out-of-bounds array access in si521xx_of_clk_get()?

When parsing device tree clock indices, the user-provided index from the
device tree is directly used as an array index without bounds checking:

drivers/clk/clk-si521xx.c:si521xx_of_clk_get() {
	...
	unsigned int idx = clkspec->args[0];

	return &si->clk_dif[idx].hw;
}

If a device tree specifies a clock index beyond the initialized clocks for
the specific chip, this returns an arbitrary pointer or an uninitialized
struct, causing memory corruption or a NULL pointer dereference in the
clock core.

Should the index be validated against a maximum value (like the number of
outputs for the chip) before accessing si->clk_dif[]?

[Severity: High]
This is a pre-existing issue, but does the resume path overwrite the Byte
Count register and break subsequent I2C reads?

In si521xx_probe(), the Byte Count register (SI521XX_REG_BC) is directly
written to 1 via i2c_master_send(), bypassing the regmap:

drivers/clk/clk-si521xx.c:si521xx_probe() {
	...
	const u8 data[3] = { SI521XX_REG_BC, 1, 1 };
	...
	ret = i2c_master_send(client, data, ARRAY_SIZE(data));
	...
}

Because this register is never updated via the regmap and no reg_defaults
are provided, its cached value remains uninitialized (0).

During si521xx_resume(), regcache_sync() writes all cached values for
writable registers back to the hardware:

drivers/clk/clk-si521xx.c:si521xx_resume() {
	...
	ret = regcache_sync(si->regmap);
	...
}

This overwrites SI521XX_REG_BC with 0. The custom read function
si521xx_regmap_i2c_read() relies on the hardware BC being 1 to correctly
receive 1 byte of data.

Does this cause I2C reads to fail or return corrupt data after a suspend
and resume cycle?

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, clk_si521xx_of_match);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1