Re: [PATCH v2] i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Mixing `goto`-based error handling with scope-based `__free()` cleanup in the same function.

Pre-existing issues:
- [High] Hardware reset is asserted before unregistering the I2C adapter, risking bus faults during concurrent access.
- [High] Indefinite sleep in `wait_for_completion()` can cause a permanent bus deadlock if the hardware misses an interrupt.
--

commit e9f19d117f2197dcc4d4457a0ba48848fb326e26
Author: Uday Khare <udaykhare77@gmail.com>

i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe

In p2wi_probe(), the device node reference obtained via
of_get_next_available_child() is stored in childnp. This reference is
never released, causing a device node reference leak.

Fix this by declaring childnp with the __free(device_node) cleanup
attribute, which automatically releases the device node reference
when childnp goes out of scope.

> diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c
> index dffbe776a195d..8469a0ea98d7d 100644
> --- a/drivers/i2c/busses/i2c-sun6i-p2wi.c
> +++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c
[ ... ]
> @@ -217,7 +217,9 @@ static int p2wi_probe(struct platform_device *pdev)
>  	 * In this case the target_addr is set to -1 and won't be checked when
>  	 * launching a P2WI transfer.
>  	 */
> -	childnp = of_get_next_available_child(np, NULL);
> +	struct device_node *childnp __free(device_node) =
> +		of_get_next_available_child(np, NULL);
> +

[Severity: Medium]
Does this mix scope-based cleanup with goto-based error handling?

p2wi_probe() continues to use a goto label for manual cleanup of the reset
controller further down:

	ret = devm_request_irq(dev, irq, p2wi_interrupt, 0, pdev->name, p2wi);
	if (ret) {
		dev_err_probe(dev, ret, "can't register interrupt handler irq%d\n", irq);
		goto err_reset_assert;
	}
	...
err_reset_assert:
	reset_control_assert(p2wi->rstc);

Mixing goto-based error handling and __free() declarations in the same routine
is discouraged in include/linux/cleanup.h as it can create confusing ownership
semantics and increases the risk of double-free bugs.

Should the entire routine either be converted to use scope-based cleanups
exclusively, or rely solely on goto with manual of_node_put()?

>  	if (childnp) {
>  		ret = of_property_read_u32(childnp, "reg", &target_addr);
>  		if (ret)

[Severity: High]
This is a pre-existing issue, but does p2wi_remove() tear down resources in
the wrong order?

p2wi_remove() asserts the hardware reset before unregistering the i2c adapter:

static void p2wi_remove(struct platform_device *dev)
{
	struct p2wi *p2wi = platform_get_drvdata(dev);

	reset_control_assert(p2wi->rstc);
	i2c_del_adapter(&p2wi->adapter);
}

This places the underlying hardware into a reset state while the i2c adapter
abstraction is still actively registered. If a concurrent i2c transfer
(p2wi_smbus_xfer) is executing or initiates, the driver will perform i/o
operations on hardware registers of a reset device, which could cause an
unrecoverable system crash or hang.

Should i2c_del_adapter() be called first to flush pending transfers and prevent
new ones before shutting down hardware resources?

[Severity: High]
This is another pre-existing issue, but could p2wi_smbus_xfer() cause a
permanent bus deadlock?

The transfer function p2wi_smbus_xfer() calls wait_for_completion() without
a timeout:

	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
	       p2wi->regs + P2WI_CTRL);

	wait_for_completion(&p2wi->complete);

This function is invoked with the i2c bus lock held by the caller. If the
hardware fails to generate an interrupt due to a bus glitch or hardware fault,
the task will block indefinitely in the uninterruptible state. This
permanently deadlocks the entire i2c bus.

Would it be safer to use wait_for_completion_timeout() to allow recovery from
hardware faults and propagate the error?

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