* Re: [PATCH v2] i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe
2026-06-18 14:05 ` [PATCH v2] i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe Uday Khare
@ 2026-06-18 14:13 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-18 14:13 UTC (permalink / raw)
To: Uday Khare; +Cc: linux-sunxi
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
^ permalink raw reply [flat|nested] 2+ messages in thread