Re: [PATCH v4 2/3] iio: proximity: add driver for Sharp GP2AP070S proximity sensor

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling `regmap_reinit_cache` in device teardown path allocates memory and debugfs nodes improperly.
- [High] Using `REGCACHE_FLAT` without `reg_defaults` breaks threshold bounds checking logic.
- [High] Race condition between IRQ handler and IIO device unregistration leads to Use-After-Free.
- [Medium] Inaccurate timestamping due to unnecessary use of threaded IRQ.
--

--- Patch [2]: [PATCH v4 2/3] iio: proximity: add driver for Sharp GP2AP070S proximity sensor ---
commit cd2088367308936b58996051286de0d57904dcfc
Author: Kaustabh Chakraborty <[email protected]>

iio: proximity: add driver for Sharp GP2AP070S proximity sensor
    
The GP2AP070S is a proximity sensor designed and manufactured by Sharp
Corporation. This sensor is used in mobile devices, including, but not
limited to - the Samsung Galaxy J6.
    
Link: https://github.com/Exynos7870/android_kernel_samsung_universal7870/blob/lineage-16.0/drivers/sensors/gp2ap070s.c [1]

diff --git a/drivers/iio/proximity/gp2ap070s.c b/drivers/iio/proximity/gp2ap070s.c
new file mode 100644
index 0000000000000..49770c308ddbb
--- /dev/null
+++ b/drivers/iio/proximity/gp2ap070s.c
@@ -0,0 +1,491 @@
[ ... ]
> +static const struct regmap_config gp2ap070s_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_reg = gp2ap070s_regmap_volatile,
> +	.max_register = GP2AP070S_REG_MAX,
> +	.cache_type = REGCACHE_FLAT,
> +};

[Severity: High]
Does using REGCACHE_FLAT without providing initial hardware defaults break
the threshold bounds checking logic?

Because PS_THD_LO_LE16 and PS_THD_HI_LE16 are not written during probe, the
flat regmap cache implementation satisfies reads from its 0-initialized array
and does not fall back to hardware. This causes unwritten registers to be
cached as 0, which breaks the bounds check in
gp2ap070s_iio_write_event_value().

[ ... ]
> +static int gp2ap070s_iio_write_event_value(struct iio_dev *indio_dev,
[ ... ]
> +	case IIO_EV_DIR_RISING:
> +		ret = regmap_bulk_read(drvdata->regmap,
> +				       GP2AP070S_REG_PS_THD_LO_LE16, &value,
> +				       sizeof(value));
> +		if (ret)
> +			return ret;
> +
> +		/* Ensure lo_threshold < hi_threshold */
> +		threshold_other = le16_to_cpu(value);
> +		if (threshold_other >= val)
> +			return -EINVAL;
> +
> +		value = cpu_to_le16(val);
> +		ret = regmap_bulk_write(drvdata->regmap,
> +					GP2AP070S_REG_PS_THD_HI_LE16, &value,
> +					sizeof(value));
> +		if (ret)
> +			return ret;
> +
> +		return 0;
> +	case IIO_EV_DIR_FALLING:
> +		ret = regmap_bulk_read(drvdata->regmap,
> +				       GP2AP070S_REG_PS_THD_HI_LE16, &value,
> +				       sizeof(value));
> +		if (ret)
> +			return ret;
> +
> +		/* Ensure hi_threshold > lo_threshold */
> +		threshold_other = le16_to_cpu(value);
> +		if (threshold_other <= val)
> +			return -EINVAL;

[Severity: High]
Could this reject valid writes to the falling threshold?

If a user writes the falling threshold before the rising threshold,
threshold_other will be incorrectly read as 0 from the uninitialized cache.
The check (0 <= val) will always evaluate to true for positive values,
returning -EINVAL.

[ ... ]
> +static irqreturn_t gp2ap070s_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	s64 timestamp = iio_get_time_ns(indio_dev);

[Severity: Medium]
Will this timestamp suffer from thread scheduling latency?

Since the IRQ is requested with a NULL primary handler, this entire routine
executes in a kernel thread. Capturing the timestamp in threaded context
rather than hard IRQ context can introduce jitter to the IIO event
timestamps.

> +
> +	iio_push_event(indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0, IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       timestamp);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int gp2ap070s_reset(struct gp2ap070s_drvdata *drvdata)
> +{
> +	int ret;
> +
> +	ret = regmap_write(drvdata->regmap, GP2AP070S_REG_COM1, 0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_reinit_cache(drvdata->regmap, &gp2ap070s_regmap_config);
> +}
> +
> +static void gp2ap070s_reset_action(void *private)
> +{
> +	struct gp2ap070s_drvdata *drvdata = private;
> +
> +	gp2ap070s_reset(drvdata);
> +}

[Severity: High]
Is it safe to call regmap_reinit_cache() during device teardown?

When the module is unloaded or unbound, the devres group executes
gp2ap070s_reset_action(), which calls gp2ap070s_reset(). Calling
regmap_reinit_cache() allocates memory and attempts to re-create debugfs
nodes.

The I2C subsystem cleans up the client's debugfs directory before devres
actions run, so attempting to create files in a destroyed parent directory
can lead to kernel warnings or memory corruption.

[ ... ]
> +static int gp2ap070s_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					gp2ap070s_irq_handler, IRQF_ONESHOT,
> +					"gp2ap070s-irq", indio_dev);

[Severity: Medium]
Is it necessary to use a threaded IRQ here?

Since the interrupt handler only captures a timestamp and pushes an event
without performing blocking operations like I2C reads, could this run as a
primary hard IRQ handler to improve timestamp accuracy?

> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[Severity: High]
Does registering the IRQ before the IIO device introduce a use-after-free
race during module unload?

Due to devres LIFO ordering, devm_iio_device_register() cleanup runs first
on removal, calling iio_device_unregister() and destroying the event
interface.

If an interrupt fires in the window before the IRQ is freed,
gp2ap070s_irq_handler() will execute and call iio_push_event(), which races
with the teardown thread and can dereference the freed event interface.

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