Re: [PATCH] mfd: tps6594: copy regmap IRQ chip descriptors per probe

[Lee Jones]

On Thu, 11 Jun 2026, Runyu Xiao wrote:

> tps6594_device_init() selects one of several shared static
> struct regmap_irq_chip templates and then writes the current probe's
> irq_drv_data and generated name into that shared descriptor before
> passing it to devm_regmap_add_irq_chip().
> 
> On a running system this is reachable whenever another TPS6594,
> TPS65224, or TPS652G1 instance probes through the same descriptor
> family. regmap-irq keeps the raw chip pointer, so the later probe
> overwrites the earlier instance's callback context. A later IRQ can
> then run tps6594_handle_post_irq() with the wrong struct tps6594,
> name, chip_id, regmap, and CRC handling path.
> 
> The issue was found on Linux v6.18.21 during manual auditing of drivers
> that reuse shared regmap_irq_chip descriptors while filling probe-local
> irq_drv_data and name fields before devm_regmap_add_irq_chip(), and was
> confirmed with a focused QEMU no-device validation harness. That test
> showed a later probe could overwrite the earlier registration's saved
> callback context through the shared chip descriptor, while per-probe
> descriptor copies preserved callback ownership for both registrations.
> 
> Copy the selected descriptor with devm_kmemdup(), mutate only the
> copy, and pass that copy to devm_regmap_add_irq_chip(). Also mark the
> static descriptors const so probe-local state cannot be written back
> into shared templates again.
> 
> Fixes: 325bec7157b3 ("mfd: tps6594: Add driver for TI TPS6594 PMIC")
> Fixes: 9d855b8144e6 ("mfd: tps6594-core: Add TI TPS65224 PMIC core")
> Fixes: 626bb0a45584 ("mfd: tps6594: Add TI TPS652G1 support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
> ---
>  drivers/mfd/tps6594-core.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git "a/drivers/mfd/tps6594-core.c" "b/drivers/mfd/tps6594-core.c"
> index 8b26c4127472..36904979b6b0 100644
> --- "a/drivers/mfd/tps6594-core.c"
> +++ "b/drivers/mfd/tps6594-core.c"
> @@ -531,7 +531,7 @@ static int tps6594_handle_post_irq(void *irq_drv_data)
>  	return ret;
>  };
>  
> -static struct regmap_irq_chip tps6594_irq_chip = {
> +static const struct regmap_irq_chip tps6594_irq_chip = {
>  	.ack_base = TPS6594_REG_INT_BUCK1_2,
>  	.ack_invert = 1,
>  	.clear_ack = 1,
> @@ -543,7 +543,7 @@ static struct regmap_irq_chip tps6594_irq_chip = {
>  	.handle_post_irq = tps6594_handle_post_irq,
>  };
>  
> -static struct regmap_irq_chip tps65224_irq_chip = {
> +static const struct regmap_irq_chip tps65224_irq_chip = {
>  	.ack_base = TPS6594_REG_INT_BUCK,
>  	.ack_invert = 1,
>  	.clear_ack = 1,
> @@ -555,7 +555,7 @@ static struct regmap_irq_chip tps65224_irq_chip = {
>  	.handle_post_irq = tps6594_handle_post_irq,
>  };
>  
> -static struct regmap_irq_chip tps652g1_irq_chip = {
> +static const struct regmap_irq_chip tps652g1_irq_chip = {
>  	.ack_base = TPS6594_REG_INT_BUCK,
>  	.ack_invert = 1,
>  	.clear_ack = 1,
> @@ -707,7 +707,10 @@ int tps6594_device_init(struct tps6594 *tps, bool enable_crc)
>  {
>  	struct device *dev = tps->dev;
>  	int ret;
> -	struct regmap_irq_chip *irq_chip;
> +	const struct regmap_irq_chip *irq_chip;
> +	struct regmap_irq_chip irq_chip_copy;
> +	const char *irq_chip_name;
> +	void *irq_chip_desc;

Putting irq_chip_copy on the stack and using void* here is pretty rough.

How about declaring a typed 'struct regmap_irq_chip *chip' pointer
instead would keep things cleaner.

>  	unsigned int pwr_on, gpio3_cfg;
>  	const struct mfd_cell *cells;
>  	int n_cells;
> @@ -738,15 +741,22 @@ int tps6594_device_init(struct tps6594 *tps, bool enable_crc)
>  		cells = tps6594_common_cells;
>  	}
>  
> -	irq_chip->irq_drv_data = tps;
> -	irq_chip->name = devm_kasprintf(dev, GFP_KERNEL, "%s-%ld-0x%02x",
> -					dev->driver->name, tps->chip_id, tps->reg);
> +	irq_chip_name = devm_kasprintf(dev, GFP_KERNEL, "%s-%ld-0x%02x",
> +				       dev->driver->name, tps->chip_id, tps->reg);
> +	if (!irq_chip_name)
> +		return -ENOMEM;
> +
> +	irq_chip_copy = *irq_chip;
> +	irq_chip_copy.irq_drv_data = tps;
> +	irq_chip_copy.name = irq_chip_name;
>  
> -	if (!irq_chip->name)
> +	irq_chip_desc = devm_kmemdup(dev, &irq_chip_copy, sizeof(irq_chip_copy),
> +				     GFP_KERNEL);

Then we can perform the 'devm_kmemdup()' call first using the template
pointer and modify the heap-allocated structure directly. 

How about:

chip = devm_kmemdup(dev, irq_chip, sizeof(*chip), GFP_KERNEL);
if (!chip)
	return -ENOMEM;

chip->irq_drv_data = tps;
chip->name = irq_chip_name;

> +	if (!irq_chip_desc)
>  		return -ENOMEM;
>  
>  	ret = devm_regmap_add_irq_chip(dev, tps->regmap, tps->irq, IRQF_SHARED | IRQF_ONESHOT,
> -				       0, irq_chip, &tps->irq_data);
> +				       0, irq_chip_desc, &tps->irq_data);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to add regmap IRQ\n");
>  
> -- 
> 2.34.1

-- 
Lee Jones