Re: [PATCH v3 21/23] mfd: Unify style of of_device_id arrays

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Complex platform data and child device structures passed via match data
- [High] Dynamic modification of global mfd_cell arrays introduces data races
- [High] Data race on global da9063_regmap_config struct during probe
- [High] IRQ handler leak and subsequent Use-After-Free in twl6030_exit_irq
--

--- Patch [21]: [PATCH v3 21/23] mfd: Unify style of of_device_id arrays ---
commit 99f4911da67f8c43f41d211ea0ad4e7a3a4972cd
Author: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>

mfd: Unify style of of_device_id arrays

This commit unifies the coding style of of_device_id arrays across multiple
MFD drivers. It removes trailing commas after list terminators, adds spaces
before closing braces, and ensures consistent formatting.

> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
[ ... ]
> @@ -361,11 +361,12 @@ static struct regmap_config da9063_regmap_config = {
>  };
>  
>  static const struct of_device_id da9063_dt_ids[] = {
> -	{ .compatible = "dlg,da9063", },
> -	{ .compatible = "dlg,da9063l", },
> +	{ .compatible = "dlg,da9063" },
> +	{ .compatible = "dlg,da9063l" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, da9063_dt_ids);
> +
>  static int da9063_i2c_probe(struct i2c_client *i2c)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);

[Severity: High]
This is a pre-existing issue, but does da9063_i2c_probe() mutate the shared
global da9063_regmap_config struct during probe?

Depending on the device variant, the probe function dynamically assigns
different tables:

    da9063_regmap_config.rd_table = &da9063_ad_readable_table;

Could this lead to a data race causing incorrect configuration if multiple
instances of the device (or different variants) are probed at the same time?

> diff --git a/drivers/mfd/rohm-bd9576.c b/drivers/mfd/rohm-bd9576.c
> --- a/drivers/mfd/rohm-bd9576.c
> +++ b/drivers/mfd/rohm-bd9576.c
[ ... ]
> @@ -167,9 +167,9 @@ static int bd957x_i2c_probe(struct i2c_client *i2c)
>  }
>  
>  static const struct of_device_id bd957x_of_match[] = {
> -	{ .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576, },
> -	{ .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573, },
> -	{ },
> +	{ .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576 },
> +	{ .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573 },
> +	{ }
>  };

[Severity: High]
This isn't a bug introduced by this patch, but does the probe function for
this device dynamically modify a shared global mfd_cell array?

bd957x_i2c_probe() contains:

    regulators = &bd9576_mfd_cells[BD957X_REGULATOR_CELL];
    regulators->resources = bd9576_regulator_irqs;

If multiple instances of this device are probed concurrently, could they race
to modify the same global array? A similar issue also seems to exist in
88pm860x-core.c and as3711.c.

> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> --- a/drivers/mfd/simple-mfd-i2c.c
> +++ b/drivers/mfd/simple-mfd-i2c.c
[ ... ]
> @@ -126,7 +126,7 @@ static const struct of_device_id simple_mfd_i2c_of_match[] = {
>  	{ .compatible = "maxim,max77705-battery", .data = &maxim_mon_max77705 },
>  	{ .compatible = "silergy,sy7636a", .data = &silergy_sy7636a },
>  	{ .compatible = "spacemit,p1", .data = &spacemit_p1 },
> -	{}
> +	{ }
>  };

[Severity: High]
This is a pre-existing issue, but does passing complex struct pointers
through the match data violate the MFD subsystem guidelines? The
documentation recommends using an integer or enum ID instead to avoid memory
safety hazards and initialization ordering races.

This pattern appears here as well as in mc13xxx-spi.c, rk8xx-i2c.c,
sec-acpm.c, sec-i2c.c, and sun6i-prcm.c.

> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
[ ... ]
> @@ -285,7 +285,7 @@ static const struct irq_domain_ops twl6030_irq_domain_ops = {
>  static const struct of_device_id twl6030_of_match[] __maybe_unused = {
>  	{ .compatible = "ti,twl6030", .data = &twl6030_interrupt_mapping },
>  	{ .compatible = "ti,twl6032", .data = &twl6032_interrupt_mapping },
> -	{ },
> +	{ }
>  };
>  
>  int twl6030_init_irq(struct device *dev, int irq_num)

[Severity: High]
This is a pre-existing issue, but is there an IRQ handler leak and potential
use-after-free during device removal here?

In twl6030_init_irq(), the interrupt is requested with twl6030_irq as the
dev_id:

    status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
                                  IRQF_ONESHOT, "TWL6030-PIH", twl6030_irq);

However, in twl6030_exit_irq(), it appears NULL is passed to free_irq()
instead of twl6030_irq:

    free_irq(twl6030_irq->twl_irq, NULL);

Will free_irq() silently fail to remove the handler due to the mismatched
dev_id, leaving the handler registered after the twl6030_irq struct is freed?

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