Re: [PATCH v2 2/4] mfd: tps65217: Check return value when masking interrupt sources

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

Pre-existing issues:
- [High] The global `mfd_cell` array `tps65217s` is not defined as `static const` and is dynamically mutated during probe, breaking subsequent device instances.
- [High] The driver hardcodes `-1` (`PLATFORM_DEVID_NONE`) for cell indexing in `devm_mfd_add_devices()`, causing sysfs device naming collisions.
- [High] Unchecked return value of `irq_find_mapping()` is passed directly to `handle_nested_irq()`, potentially causing a NULL pointer dereference or invalid IRQ execution.
- [High] Read-modify-write operations on the clear-on-read `TPS65217_REG_INT` register silently drop pending hardware interrupts.
- [Medium] The IRQ wake count is unbalanced due to a missing `disable_irq_wake()` call on probe error paths.
--

--- Patch [2]: [PATCH v2 2/4] mfd: tps65217: Check return value when masking interrupt sources ---
commit 07181fdc30db713af1912813b5a67b5c8d55e141
Author: Жамбакиев Радий Рикардинович <[email protected]>

mfd: tps65217: Check return value when masking interrupt sources

This commit modifies tps65217_irq_init() to check the return value of
tps65217_set_bits() when masking interrupt sources. It propagates the error
if the register write fails, preventing the software mask from going out of
sync with the hardware.

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 2d04d9e0ae29c..9a1528456ffc2 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -155,8 +155,13 @@ static int tps65217_irq_init(struct tps65217 *tps, int irq)
>  
>  	/* Mask all interrupt sources */
>  	tps->irq_mask = TPS65217_INT_MASK;
> -	tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> -			  TPS65217_INT_MASK, TPS65217_PROTECT_NONE);
> +	ret = tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> +				TPS65217_INT_MASK, TPS65217_PROTECT_NONE);
> +	if (ret) {
> +		dev_err(tps->dev, "Failed to mask interrupt sources: %d\n",
> +			ret);
> +		return ret;
> +	}
>  
>  	tps->irq_domain = irq_domain_create_linear(dev_fwnode(tps->dev), TPS65217_NUM_IRQ,
>  						   &tps65217_irq_domain_ops, tps);

[ ... ]

> static void tps65217_irq_sync_unlock(struct irq_data *data)
> {
> 	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
> 	int ret;
> 
> 	ret = tps65217_set_bits(tps, TPS65217_REG_INT, TPS65217_INT_MASK,
> 				tps->irq_mask, TPS65217_PROTECT_NONE);

[Severity: High]
This is a pre-existing issue, but does performing a read-modify-write via
tps65217_set_bits() on the clear-on-read TPS65217_REG_INT register silently
drop pending hardware interrupts? 

[ ... ]

> static struct mfd_cell tps65217s[] = {

[Severity: High]
This isn't a bug introduced by this patch, but does the tps65217s array lack a
const qualifier?

> 	{
> 		.name = "tps65217-pmic",
> 		.of_compatible = "ti,tps65217-pmic",
> 	},

[ ... ]

> static irqreturn_t tps65217_irq_thread(int irq, void *data)
> {

[ ... ]

> 	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
> 		if (status & BIT(i)) {
> 			handle_nested_irq(irq_find_mapping(tps->irq_domain, i));

[Severity: High]
This is a pre-existing issue, but does this code unconditionally pass the
result of irq_find_mapping() to handle_nested_irq() without verifying it is
non-zero?

Since irq_find_mapping() returns 0 if an interrupt is unmapped, could passing
0 to handle_nested_irq() lead to a NULL pointer dereference in irq_to_desc(0)
or falsely trigger the system timer interrupt handler if an unmapped bit is
set?

> 			handled = true;
> 		}
> 	}

[ ... ]

> static int tps65217_probe(struct i2c_client *client)
> {

[ ... ]

> 	if (client->irq) {
> 		ret = tps65217_irq_init(tps, client->irq);
> 		if (ret)
> 			return ret;
> 	} else {
> 		int i;
> 
> 		/* Don't tell children about IRQ resources which won't fire */
> 		for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
> 			tps65217s[i].num_resources = 0;

[Severity: High]
This isn't a bug introduced by this patch, but does modifying
tps65217s[i].num_resources permanently corrupt this shared global template?

If a TPS65217 device is probed without an IRQ, wouldn't subsequent instances
or deferred probes observe the corrupted zeroed resources?

> 	}
> 
> 	ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
> 				   ARRAY_SIZE(tps65217s), NULL, 0,
> 				   tps->irq_domain);

[Severity: High]
This is a pre-existing issue, but doesn't hardcoding -1 (PLATFORM_DEVID_NONE)
for cell indexing prevent the MFD core from appending unique instance IDs to
child devices?

Should PLATFORM_DEVID_AUTO be used instead to avoid duplicate sysfs device
naming collisions if multiple chips are present?

> 	if (ret < 0) {
> 		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> 		return ret;

[Severity: Medium]
This isn't a bug introduced by this patch, but if tps65217_irq_init() succeeds
and enables IRQ wake, does this error path leave the IRQ wake count unbalanced
since it misses a disable_irq_wake() call before returning?

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