diff mbox series

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

Message ID 20260618140525.11526-1-udaykhare77@gmail.com (mailing list archive)
State New
Headers
Series [v2] i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe |

Commit Message

Uday Khare June 18, 2026, 2:05 p.m. UTC 
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.

Fixes: 3e833490fae5 ("i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support")
Signed-off-by: Uday Khare <udaykhare77@gmail.com>
---
v2:
- Use __free(device_node) and include <linux/cleanup.h> to automate the device
  node reference cleanup instead of manually calling of_node_put() on error and
  success paths (suggested by Chen-Yu Tsai).

 drivers/i2c/busses/i2c-sun6i-p2wi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org June 18, 2026, 2:13 p.m. UTC | #1 
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?
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c
index dffbe776a195..8469a0ea98d7 100644
--- a/drivers/i2c/busses/i2c-sun6i-p2wi.c
+++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c
@@ -21,6 +21,7 @@ 
  * PMIC).
  *
  */
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/io.h>
@@ -184,7 +185,6 @@  static int p2wi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *childnp;
 	unsigned long parent_clk_freq;
 	u32 clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
 	struct p2wi *p2wi;
@@ -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);
+
 	if (childnp) {
 		ret = of_property_read_u32(childnp, "reg", &target_addr);
 		if (ret)