Re: [PATCH RFC] clk: versaclock7: propagate all xin lookup failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Reading and writing multi-byte device registers into native integer variables via regmap_bulk_read/write causes endianness-dependent behavior and data corruption on big-endian architectures.
- [High] vc7_get_apll_rate() returns negative error codes cast to unsigned long, resulting in bogus clock rates being registered on failure.
- [High] Unconditional dereference of client->dev.of_node->name may cause a kernel panic if of_node is NULL.
- [High] Missing error check for regmap_read causes uninitialized stack variable usage.
--

commit 3e1c7db53668b06c3bbde7c57f495f683d910e66
Author: Slavin Liu <bolin.liu@seu.edu.cn>

clk: versaclock7: propagate all xin lookup failures

This commit updates the versaclock7 driver to handle all lookup errors for
the xin clock, propagating failures properly rather than just probe
deferrals.

> diff --git a/drivers/clk/clk-versaclock7.c b/drivers/clk/clk-versaclock7.c
> index 1340d2f512be5..5f32d3fb807a0 100644
> --- a/drivers/clk/clk-versaclock7.c
> +++ b/drivers/clk/clk-versaclock7.c
> @@ -515,10 +515,10 @@ static int vc7_read_apll(struct vc7_driver_data *vc7)
>  {
>  	int err;
>  	u32 val32;
>  	u16 val16;
>  
>  	err = regmap_bulk_read(vc7->regmap,
>  			       VC7_REG_XO_CNFG,
>  			       (u32 *)&val32,
>  			       VC7_REG_XO_CNFG_COUNT);

[Severity: High]
This is a pre-existing issue, but does using regmap_bulk_read() to read raw
bytes sequentially into native integer variables like u32 and u64 cause
endianness-dependent behavior?

On big-endian architectures, reading raw bytes sequentially into a native
integer will alias incorrectly. This appears to also occur in
vc7_read_fod(), vc7_write_fod(), vc7_read_iod(), and vc7_write_iod().

[ ... ]

> @@ -725,10 +725,10 @@ static unsigned long vc7_get_apll_rate(struct vc7_driver_data *vc7)
>  {
>  	int err;
>  	unsigned long xtal_rate;
>  	u64 refin_div, apll_rate;
>  
>  	xtal_rate = clk_get_rate(vc7->pin_xin);
>  	err = vc7_read_apll(vc7);
>  	if (err) {
>  		dev_err(&vc7->client->dev, "unable to read apll\n");
>  		return err;
>  	}

[Severity: High]
This is a pre-existing issue, but does returning a negative error code
as an unsigned long produce a bogus clock rate?

When vc7_read_apll() fails (e.g., due to an I2C transaction timeout), it
returns a negative error code which gets cast to a large unsigned value.
This value is then silently accepted by the caller in vc7_probe() and
passed to clk_register_fixed_rate() as the apll_rate.

[ ... ]

> @@ -1117,17 +1117,17 @@ static int vc7_probe(struct i2c_client *client)
>  	vc7->chip_info = i2c_get_match_data(client);
>  
>  	vc7->pin_xin = devm_clk_get(&client->dev, "xin");
> -	if (PTR_ERR(vc7->pin_xin) == -EPROBE_DEFER) {
> -		return dev_err_probe(&client->dev, -EPROBE_DEFER,
> +	if (IS_ERR(vc7->pin_xin)) {
> +		return dev_err_probe(&client->dev, PTR_ERR(vc7->pin_xin),
>  				     "xin not specified\n");
>  	}
>  
>  	vc7->regmap = devm_regmap_init_i2c(client, &vc7_regmap_config);
>  	if (IS_ERR(vc7->regmap)) {
>  		return dev_err_probe(&client->dev, PTR_ERR(vc7->regmap),
>  				     "failed to allocate register map\n");
>  	}
>  
>  	if (of_property_read_string(client->dev.of_node, "clock-output-names",
>  				    &node_name))
>  		node_name = client->dev.of_node->name;

[Severity: High]
This is a pre-existing issue, but could this unconditionally dereference a
NULL pointer if client->dev.of_node is NULL?

If the device is instantiated outside of device tree (e.g., via I2C board
info or ACPI without an explicit OF node), of_property_read_string() handles
the NULL of_node safely by returning an error.  However, the fallback path
then unconditionally dereferences client->dev.of_node->name, which would
cause a kernel panic.

[ ... ]

> @@ -1186,8 +1186,8 @@ static int vc7_probe(struct i2c_client *client)
>  		/*
>  		 * This driver does not support remapping FOD/IOD to banks.
>  		 * The device state is read and the driver is setup to match
>  		 * the device's existing mapping.
>  		 */
>  		bank_idx = output_bank_mapping[out_num];
>  
>  		regmap_read(vc7->regmap, VC7_REG_OUT_BANK_CNFG(bank_idx), &val);
>  		val &= VC7_REG_OUTPUT_BANK_SRC_MASK;

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of an error check
on regmap_read() cause the use of an uninitialized stack variable?

If regmap_read() fails, it returns an error code without modifying val.
The uninitialized stack garbage in val is then used in a bitwise AND and
subsequently passed to vc7_get_bank_clk(), which could cause unpredictable
device configuration or out-of-bounds array mapping.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911060918.94343-1-bolin.liu@seu.edu.cn?part=1