Re: [PATCH v3] mfd: da903x: cancel IRQ work during teardown

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

New issues:
- [Low] The commit message description string is not capitalized.

Pre-existing issues:
- [High] Missing NULL check for `pdata` in probe path leads to kernel Oops.
- [High] Data race on `chip->events_mask` leading to lost interrupt masks.
--

--- Patch [1]: [PATCH v3] mfd: da903x: cancel IRQ work during teardown ---
commit d86788370ab7658c6e3b5684327f0a0b0169e280
Author: Hongyan Xu <[email protected]>

mfd: da903x: cancel IRQ work during teardown

The IRQ handler disables the IRQ and schedules irq_work. Releasing the
IRQ does not drain that work, which can continue to use the devm-allocated
chip and notifier state. Manage both IRQ-work cancellation and subdevice
removal with devres.

> mfd: da903x: cancel IRQ work during teardown

[Severity: Low]
This isn't a bug, but there's a minor formatting issue in the commit message.
The MFD subsystem guidelines require capitalizing the description after the
subsystem prefix.

Could the commit message be updated to use 'Cancel' instead of 'cancel' to
match these conventions?

> diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
> index e86b39de3303a..e4ad48e1f139b 100644
> --- a/drivers/mfd/da903x.c
> +++ b/drivers/mfd/da903x.c
[ ... ]
> @@ -520,6 +534,11 @@ static int da903x_probe(struct i2c_client *client)
>  	chip->ops->mask_events(chip, chip->events_mask);
>  	chip->ops->read_events(chip, &tmp);

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race involving
updates to chip->events_mask.

The exported functions da903x_register_notifier() and
da903x_unregister_notifier() call da9030_unmask_events() and
da9030_mask_events().

These functions perform non-atomic read-modify-write operations without
holding chip->lock:

drivers/mfd/da903x.c:da9030_unmask_events() {
    ...
    chip->events_mask &= ~events;
    ...
}

If multiple child drivers (like backlight or regulator) are probed or removed
concurrently, could these bitwise updates race and overwrite each other's
changes?

Would this lead to lost updates to the interrupt mask, causing some child
devices to permanently miss interrupts?

> +	ret = devm_add_action_or_reset(&client->dev,
> +				       da903x_remove_subdevs_action, chip);
> +	if (ret)
> +		return ret;
[ ... ]
> @@ -529,14 +548,12 @@ static int da903x_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	return da903x_add_subdevs(chip, pdata);
> -}
> -
> -static void da903x_remove(struct i2c_client *client)
> -{
> -	struct da903x_chip *chip = i2c_get_clientdata(client);
> +	ret = devm_add_action_or_reset(&client->dev, da903x_cancel_irq_work,
> +				       chip);
> +	if (ret)
> +		return ret;
>  
> -	da903x_remove_subdevs(chip);
> +	return da903x_add_subdevs(chip, pdata);
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential null pointer
dereference when passing pdata here.

If the I2C device is dynamically instantiated via the sysfs new_device
interface or if platform code fails to attach platform data,
dev_get_platdata(&client->dev) returns NULL.

This null pointer is then passed unconditionally to da903x_add_subdevs(),
which dereferences it immediately:

drivers/mfd/da903x.c:da903x_add_subdevs() {
    ...
    for (i = 0; i < pdata->num_subdevs; i++) {
        subdev = &pdata->subdevs[i];
    ...
}

Could this lead to a kernel oops triggered by privileged userspace interacting
with sysfs?

Should we add a null check for pdata before creating subdevices?

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