Re: [PATCH v2 3/3] Input: synaptics_i2c - return 0 explicitly on success

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

Pre-existing issues:
- [High] The delayed work `touch->dwork` is not cancelled on driver unbind or probe failure, leading to a use-after-free.
--

--- Patch [3]: [PATCH v2 3/3] Input: synaptics_i2c - return 0 explicitly on success ---
commit 2cc2d0ff5c54be0c06b9f9251e169b1467e86f9b
Author: Sang-Heon Jeon <[email protected]>

Input: synaptics_i2c - return 0 explicitly on success

error is always zero at the last return in synaptics_i2c_reg_set().
Explicitly return 0 on the success path instead of returning error,
which is the preferred way when there are multiple failure points.

> diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
> index d4cf982f12632..66e833974c6da 100644
> --- a/drivers/input/mouse/synaptics_i2c.c
> +++ b/drivers/input/mouse/synaptics_i2c.c
> @@ -261,7 +261,7 @@ static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8 val)
>  	if (error)
>  		return error;
>  
> -	return error;
> +	return 0;
>  }
>  
>  static s32 synaptics_i2c_word_get(struct i2c_client *client, u16 reg)

[Severity: High]
This is a pre-existing issue, but I noticed that the delayed work touch->dwork
doesn't appear to be cancelled on driver unbind or probe failure.

In synaptics_i2c_probe(), interrupts are enabled and devm_request_irq() is
called before input_register_device(). The IRQ handler can immediately schedule
touch->dwork.

If input_register_device() fails:

drivers/input/mouse/synaptics_i2c.c:synaptics_i2c_probe() {
	...
	error = input_register_device(touch->input);
	if (error) {
		dev_err(dev, "Input device register failed: %d\n", error);
		return error;
	}
	...
}

The probe function returns the error directly. Since devres will automatically
free the IRQ and the touch structure, but the pending work on system_dfl_wq is
never cancelled, could the delayed work run later and access the freed touch
memory?

Furthermore, the driver lacks a remove callback entirely:

drivers/input/mouse/synaptics_i2c.c:
static struct i2c_driver synaptics_i2c_driver = {
	.driver = {
		.name	= DRIVER_NAME,
		.of_match_table = of_match_ptr(synaptics_i2c_of_match),
		.pm	= pm_sleep_ptr(&synaptics_i2c_pm),
	},

	.probe		= synaptics_i2c_probe,
	.id_table	= synaptics_i2c_id_table,
};

If the device is unbound via sysfs, devres will automatically tear down
resources. However, the delayed work is only explicitly cancelled if the input
device is currently open (via synaptics_i2c_close()).

If the device was never opened before it is unbound, does this mean the work
is never cancelled before the memory is freed?

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