Re: [PATCH v2 1/3] misc: lis3lv02d: set default click detection thresholds

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch hardcodes a default click threshold instead of parsing the documented legacy DT property `st,click-thresh-*`, incorrectly overriding explicitly configured board values.

Pre-existing issues:
- [High] The dynamically allocated `struct lis3lv02d_platform_data` is leaked on probe error paths and device unbind.
- [High] The return value of `lis3lv02d_add_fs` is ignored, leading to an invalid pointer address generation and a kernel crash if device creation fails.
- [High] Sysfs hardware access occurs with a runtime PM usage count of zero, risking a race condition with device suspend.
--

commit e23e0f8c271ea91f741e5d3559287f33c9596b0d
Author: Md Shofiqul Islam <shofiqtest@gmail.com>

misc: lis3lv02d: set default click detection thresholds

This commit works around an issue where several upstream board DTS files
use an undocumented property name for click thresholds. It hardcodes a
default value of 10 during the driver's device tree initialization to
compensate for the driver reading a misspelled property name.

> diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
> index 6957091ab6dee..245bb6249033b 100644
> --- a/drivers/misc/lis3lv02d/lis3lv02d.c
> +++ b/drivers/misc/lis3lv02d/lis3lv02d.c
> @@ -958,6 +958,14 @@ int lis3lv02d_init_dt(struct lis3lv02d *lis3)
>  	if (!pdata)
>  		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this code leak the allocated platform
data?

When pdata is dynamically allocated via kzalloc() earlier in
lis3lv02d_init_dt() and assigned to lis3_dev.pdata, there appears to be no
corresponding kfree() in the error teardown paths. For example, if the bus
probe fails in lis3lv02d_i2c_probe() due to -EPROBE_DEFER, the allocated
memory is never freed:

drivers/misc/lis3lv02d/lis3lv02d_i2c.c:lis3lv02d_i2c_probe() {
    ...
fail:
    if (pdata && pdata->release_resources)
        pdata->release_resources();
    return ret;
}

Since lis3_dev is a static global variable, repeatedly probing and deferring
or unbinding and rebinding the driver could continuously leak memory.

>  
> +	/* Set sensible click threshold defaults. Several upstream board DTS
> +	 * files use the misspelled property "st,click-thresh-*" which the
> +	 * driver never reads, leaving these at zero. Default to 10 so those
> +	 * boards get working click detection without a DT change. */
> +	pdata->click_thresh_x = 10;
> +	pdata->click_thresh_y = 10;
> +	pdata->click_thresh_z = 10;

[Severity: High]
Can hardcoding these defaults override explicitly configured board values?

The device tree binding documents st,click-thresh-x/y/z, but the driver
erroneously parses st,click-threshold-x/y/z. Instead of updating the driver
to parse both properties to respect the DT ABI, this applies an arbitrary
default of 10.

If a board correctly configures a different threshold, such as
st,click-thresh-x = <8>, will this explicit configuration be ignored and
silently forced to 10?

[ ... ]

[Severity: High]
This is a pre-existing issue, but will the lack of a return value check cause
a kernel crash if device creation fails?

In lis3lv02d_init_device(), lis3lv02d_add_fs() is called without checking
its return value:

drivers/misc/lis3lv02d/lis3lv02d.c:lis3lv02d_init_device() {
    ...
    lis3lv02d_add_fs(lis3);
    err = lis3lv02d_poweron(lis3);
    ...
}

If faux_device_create_with_groups() fails and returns NULL, lis3->fdev
remains NULL. When the code later calls lis3lv02d_joystick_enable(), it
computes &lis3->fdev->dev and assigns it to input_dev->dev.parent. The
offset calculation from a NULL pointer yields an invalid pointer, leading
to a NULL pointer dereference when the input subsystem accesses the parent
device.

[Severity: High]
This is a pre-existing issue, but is sysfs hardware accessed with a runtime
PM usage count of zero?

In sysfs attribute handlers like lis3lv02d_position_show(),
lis3lv02d_sysfs_poweron() is called before accessing the hardware:

drivers/misc/lis3lv02d/lis3lv02d.c:lis3lv02d_position_show() {
    ...
    lis3lv02d_sysfs_poweron(lis3);
    mutex_lock(&lis3->mutex);
    lis3lv02d_get_xyz(lis3, &x, &y, &z);
    ...
}

However, lis3lv02d_sysfs_poweron() calls pm_runtime_get_sync(), but then
immediately drops the usage count via pm_runtime_put_noidle():

drivers/misc/lis3lv02d/lis3lv02d.c:lis3lv02d_sysfs_poweron() {
    ...
    pm_runtime_put_noidle(lis3->pm_dev);
    pm_schedule_suspend(lis3->pm_dev, LIS3_SYSFS_POWERDOWN_DELAY);
}

This drops the runtime PM usage count to 0 while the I2C/SPI registers
are actively being read. Could a concurrent system sleep physically power
off the device while communication is occurring, leading to bus lockups
or a kernel oops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611154105.3727-1-shofiqtest@gmail.com?part=1