Re: [PATCH v3 13/23] mfd: Use named initializers for arrays of i2c_device_id

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `.remove` callback and cleanup for `mfd_add_devices()` leads to Use-After-Free on unbind.
- [High] Dynamic amendment of a global non-const `mfd_cell` array.
- [High] Passing complex pointers and platform data via the `.data` field of the `of_device_id` match table.
--

--- Patch [13]: [PATCH v3 13/23] mfd: Use named initializers for arrays of i2c_device_id ---
commit 17d93c846df3c3e646099533f0b8856a806eee55
Author: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>

mfd: Use named initializers for arrays of i2c_device_id

This commit updates various MFD drivers to use named initializers for
i2c_device_id arrays. This improves code readability and ensures robustness
against future changes to the i2c_device_id struct definition.

> diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
> --- a/drivers/mfd/ti-lmu.c
> +++ b/drivers/mfd/ti-lmu.c
> @@ -206,11 +206,11 @@ static const struct of_device_id ti_lmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, ti_lmu_of_match);
>  
>  static const struct i2c_device_id ti_lmu_ids[] = {

[Severity: High]
This is a pre-existing issue, but looking at ti_lmu_of_match just above
this hunk in drivers/mfd/ti-lmu.c, it appears to pass pointers to platform
data structs (like &lm3631_data) via the data field of the of_device_id
match table.

The MFD subsystem guidelines prohibit passing complex pointers through match
tables due to memory safety hazards and initialization ordering races. Could
this be refactored to use integer IDs or enums instead?

> -	{ "lm3631", LM3631 },
> -	{ "lm3632", LM3632 },
> -	{ "lm3633", LM3633 },
> -	{ "lm3695", LM3695 },
> -	{ "lm36274", LM36274 },
> +	{ .name = "lm3631", .driver_data = LM3631 },
> +	{ .name = "lm3632", .driver_data = LM3632 },
> +	{ .name = "lm3633", .driver_data = LM3633 },
> +	{ .name = "lm3695", .driver_data = LM3695 },
> +	{ .name = "lm36274", .driver_data = LM36274 },
>  	{ }
>  };

[ ... ]

> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
> --- a/drivers/mfd/tps65090.c
> +++ b/drivers/mfd/tps65090.c
> @@ -225,7 +225,7 @@ static int tps65090_i2c_probe(struct i2c_client *client)
>  
>  
>  static const struct i2c_device_id tps65090_id_table[] = {

[Severity: High]
This isn't a bug introduced by this patch, but looking at
tps65090_i2c_probe() in drivers/mfd/tps65090.c, it modifies the global
tps65090s array dynamically before getting to this table:

tps65090s[CHARGER].num_resources = 0;

Since this modifies a shared global mfd_cell array, can this cause a data
race or initialization corruption if multiple instances of the driver probe
concurrently or sequentially with different IRQ configurations?

> -	{ "tps65090" },
> +	{ .name = "tps65090" },
>  	{ }
>  };

[ ... ]

> diff --git a/drivers/mfd/tps65218.c b/drivers/mfd/tps65218.c
> --- a/drivers/mfd/tps65218.c
> +++ b/drivers/mfd/tps65218.c
> @@ -336,8 +336,8 @@ static int tps65218_probe(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id tps65218_id_table[] = {

[Severity: High]
This is a pre-existing issue, but earlier in tps65218_probe() in
drivers/mfd/tps65218.c, mfd_add_devices() is called manually without using
the devm_mfd_add_devices variant.

Since there is no remove callback for this driver to call
mfd_remove_devices(), does this leak the child devices and cause a
use-after-free on the devm-allocated parent data when the driver is unbound?

> -	{ "tps65218", TPS65218 },
> -	{ },
> +	{ .name = "tps65218", .driver_data = TPS65218 },
> +	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, tps65218_id_table);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783615311.git.u.kleine-koenig@baylibre.com?part=13