[PATCH] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[PATCH] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[Liu Zhenlong]

cci_probe() calls of_node_get() to take an extra reference on the
child device_node when assigning it to the adapter device.  The
matching of_node_put() calls exist in both the error cleanup path
and cci_remove(), but they are placed after i2c_del_adapter().

i2c_del_adapter() clears adap->dev with memset() at the end (commit
bd4bc3dbded9 ("i2c: Clear i2c_adapter.dev on adapter removal")), which
zeroes adap->dev.of_node before of_node_put() runs, turning it into a
no-op.  The reference taken by of_node_get() is never released, leaking
the device_node on every cleanup of already-registered adapters and
every adapter removal.

The commit that added of_node_get() and the matching of_node_put()
calls placed the puts after i2c_del_adapter(), so the bug has been
present since the fix was introduced.

Cache the pointer before calling i2c_del_adapter(), the same approach
used in i2c-mux (i2c_mux_del_adapters) and mtd (commit 56570bdad5e3
("mtd: core: Fix refcount error in del_mtd_device()")).

The of_node_put() in the i2c_add_adapter() failure path (before any
i2c_del_adapter() runs) is correct and left unchanged.

Fixes: 02a4a69667a2 ("i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Liu Zhenlong <dragonliu2018@gmail.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index bdeda3979c48..61c535b8196f 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -618,8 +618,10 @@ static int cci_probe(struct platform_device *pdev)
 
 	for (--i ; i >= 0; i--) {
 		if (cci->master[i].cci) {
+			struct device_node *node = cci->master[i].adap.dev.of_node;
+
 			i2c_del_adapter(&cci->master[i].adap);
-			of_node_put(cci->master[i].adap.dev.of_node);
+			of_node_put(node);
 		}
 	}
 disable_clocks:
@@ -635,8 +637,10 @@ static void cci_remove(struct platform_device *pdev)
 
 	for (i = 0; i < cci->data->num_masters; i++) {
 		if (cci->master[i].cci) {
+			struct device_node *node = cci->master[i].adap.dev.of_node;
+
 			i2c_del_adapter(&cci->master[i].adap);
-			of_node_put(cci->master[i].adap.dev.of_node);
+			of_node_put(node);
 			cci_halt(cci, i);
 		}
 	}
-- 
2.55.0

Re: [PATCH] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[Konrad Dybcio]

On 8/15/26 4:09 PM, Liu Zhenlong wrote:
> cci_probe() calls of_node_get() to take an extra reference on the
> child device_node when assigning it to the adapter device.  The
> matching of_node_put() calls exist in both the error cleanup path
> and cci_remove(), but they are placed after i2c_del_adapter().
> 
> i2c_del_adapter() clears adap->dev with memset() at the end (commit
> bd4bc3dbded9 ("i2c: Clear i2c_adapter.dev on adapter removal")), which
> zeroes adap->dev.of_node before of_node_put() runs, turning it into a
> no-op.  The reference taken by of_node_get() is never released, leaking
> the device_node on every cleanup of already-registered adapters and
> every adapter removal.
> 
> The commit that added of_node_get() and the matching of_node_put()
> calls placed the puts after i2c_del_adapter(), so the bug has been
> present since the fix was introduced.
> 
> Cache the pointer before calling i2c_del_adapter(), the same approach
> used in i2c-mux (i2c_mux_del_adapters) and mtd (commit 56570bdad5e3
> ("mtd: core: Fix refcount error in del_mtd_device()")).
> 
> The of_node_put() in the i2c_add_adapter() failure path (before any
> i2c_del_adapter() runs) is correct and left unchanged.
> 
> Fixes: 02a4a69667a2 ("i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Liu Zhenlong <dragonliu2018@gmail.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index bdeda3979c48..61c535b8196f 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -618,8 +618,10 @@ static int cci_probe(struct platform_device *pdev)
>  
>  	for (--i ; i >= 0; i--) {
>  		if (cci->master[i].cci) {
> +			struct device_node *node = cci->master[i].adap.dev.of_node;
> +
>  			i2c_del_adapter(&cci->master[i].adap);
> -			of_node_put(cci->master[i].adap.dev.of_node);
> +			of_node_put(node);

The fix seems correct, but the way it's done is still fragile - someone/a bot
will surely come around in a couple weeks with a ""simplification"" undoing
your change. Would a devm action here be a better choice?

Konrad

[PATCH v2] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[Liu Zhenlong]

The of_node_put() matching of_node_get() runs after i2c_del_adapter(),
whose trailing memset() zeroes adap->dev and thus adap->dev.of_node,
making the put a no-op and leaking the node on every adapter removal
and error cleanup.

Use a devm action: the pointer is captured at registration, out of
reach of that memset(), and devres runs the put once on probe failure
and detach, replacing the three manual of_node_put() calls.  The
setup loop uses the scoped iterator form so the child node is released
automatically if devm_add_action_or_reset() fails mid-loop.

Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Fixes: 02a4a69667a2 ("i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Liu Zhenlong <dragonliu2018@gmail.com>
---
Changes in v2:
- Rework the fix to use a devm action (cci_put_of_node) instead of
  caching the pointer before i2c_del_adapter(), per Konrad Dybcio.
  The pointer is captured at registration, out of reach of the
  memset() in i2c_del_adapter(); the three manual of_node_put() calls
  are removed.
- Use for_each_available_child_of_node_scoped() so the child
  reference is released if devm_add_action_or_reset() fails mid-loop.

 drivers/i2c/busses/i2c-qcom-cci.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index bdeda3979c48..d3528c7d15bd 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -497,10 +497,14 @@ static const struct dev_pm_ops qcom_cci_pm = {
 	SET_RUNTIME_PM_OPS(cci_suspend_runtime, cci_resume_runtime, NULL)
 };
 
+static void cci_put_of_node(void *data)
+{
+	of_node_put(data);
+}
+
 static int cci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *child;
 	struct resource *r;
 	struct cci *cci;
 	int ret, i;
@@ -516,7 +520,7 @@ static int cci_probe(struct platform_device *pdev)
 	if (!cci->data)
 		return -ENOENT;
 
-	for_each_available_child_of_node(dev->of_node, child) {
+	for_each_available_child_of_node_scoped(dev->of_node, child) {
 		struct cci_master *master;
 		u32 idx;
 
@@ -537,6 +541,9 @@ static int cci_probe(struct platform_device *pdev)
 		master->adap.algo = &cci_algo;
 		master->adap.dev.parent = dev;
 		master->adap.dev.of_node = of_node_get(child);
+		ret = devm_add_action_or_reset(dev, cci_put_of_node, child);
+		if (ret)
+			return ret;
 		master->master = idx;
 		master->cci = cci;
 
@@ -604,10 +611,8 @@ static int cci_probe(struct platform_device *pdev)
 			continue;
 
 		ret = i2c_add_adapter(&cci->master[i].adap);
-		if (ret < 0) {
-			of_node_put(cci->master[i].adap.dev.of_node);
+		if (ret < 0)
 			goto error_i2c;
-		}
 	}
 
 	return 0;
@@ -617,10 +622,8 @@ static int cci_probe(struct platform_device *pdev)
 	pm_runtime_dont_use_autosuspend(dev);
 
 	for (--i ; i >= 0; i--) {
-		if (cci->master[i].cci) {
+		if (cci->master[i].cci)
 			i2c_del_adapter(&cci->master[i].adap);
-			of_node_put(cci->master[i].adap.dev.of_node);
-		}
 	}
 disable_clocks:
 	cci_disable_clocks(cci);
@@ -636,7 +639,6 @@ static void cci_remove(struct platform_device *pdev)
 	for (i = 0; i < cci->data->num_masters; i++) {
 		if (cci->master[i].cci) {
 			i2c_del_adapter(&cci->master[i].adap);
-			of_node_put(cci->master[i].adap.dev.of_node);
 			cci_halt(cci, i);
 		}
 	}
-- 
2.55.0

Re: [PATCH v2] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[Vladimir Zapolskiy]

Hi Liu.

On 8/18/26 20:57, Liu Zhenlong wrote:
> The of_node_put() matching of_node_get() runs after i2c_del_adapter(),
> whose trailing memset() zeroes adap->dev and thus adap->dev.of_node,
> making the put a no-op and leaking the node on every adapter removal
> and error cleanup.
> 
> Use a devm action: the pointer is captured at registration, out of
> reach of that memset(), and devres runs the put once on probe failure
> and detach, replacing the three manual of_node_put() calls.  The
> setup loop uses the scoped iterator form so the child node is released
> automatically if devm_add_action_or_reset() fails mid-loop.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Fixes: 02a4a69667a2 ("i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Liu Zhenlong <dragonliu2018@gmail.com>
> ---
> Changes in v2:
> - Rework the fix to use a devm action (cci_put_of_node) instead of
>    caching the pointer before i2c_del_adapter(), per Konrad Dybcio.
>    The pointer is captured at registration, out of reach of the
>    memset() in i2c_del_adapter(); the three manual of_node_put() calls
>    are removed.
> - Use for_each_available_child_of_node_scoped() so the child
>    reference is released if devm_add_action_or_reset() fails mid-loop.
> 
>   drivers/i2c/busses/i2c-qcom-cci.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index bdeda3979c48..d3528c7d15bd 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -497,10 +497,14 @@ static const struct dev_pm_ops qcom_cci_pm = {
>   	SET_RUNTIME_PM_OPS(cci_suspend_runtime, cci_resume_runtime, NULL)
>   };
>   
> +static void cci_put_of_node(void *data)
> +{
> +	of_node_put(data);
> +}
> +
>   static int cci_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> -	struct device_node *child;
>   	struct resource *r;
>   	struct cci *cci;
>   	int ret, i;
> @@ -516,7 +520,7 @@ static int cci_probe(struct platform_device *pdev)
>   	if (!cci->data)
>   		return -ENOENT;
>   
> -	for_each_available_child_of_node(dev->of_node, child) {
> +	for_each_available_child_of_node_scoped(dev->of_node, child) {
>   		struct cci_master *master;
>   		u32 idx;
>   
> @@ -537,6 +541,9 @@ static int cci_probe(struct platform_device *pdev)
>   		master->adap.algo = &cci_algo;
>   		master->adap.dev.parent = dev;
>   		master->adap.dev.of_node = of_node_get(child);
> +		ret = devm_add_action_or_reset(dev, cci_put_of_node, child);

I believe the new cci_put_of_node() and the original of_node_put() functions
are type compatible, therefore a function type cast could be sufficient here:

    (void (*)(void *))of_node_put

In any case the change seems to correct, thank you for the fix!

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

Re: [PATCH v2] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[Liu Zhenlong]

Hi Vladimir,

Thanks for the review and the Reviewed-by!

On the cast: I considered (void (*)(void *))of_node_put, but kept the
small wrapper.  Casting to an incompatible function pointer type and
calling through it is technically undefined, and tegra_dc_of_node_put()
in drivers/gpu/drm/tegra/rgb.c uses the same one-line wrapper around
of_node_put(), so I followed that existing pattern.  Happy to switch
to the cast if you'd still prefer it.

Regards,
Liu Zhenlong

Re: [PATCH v2] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove()

[Vladimir Zapolskiy]

On 8/19/26 06:03, Liu Zhenlong wrote:
> Hi Vladimir,
> 
> Thanks for the review and the Reviewed-by!
> 
> On the cast: I considered (void (*)(void *))of_node_put, but kept the
> small wrapper.  Casting to an incompatible function pointer type and
> calling through it is technically undefined, 

Exactly, according to my reading of C99 section 6.7.5.3, paragraph 15,
I believe it is a cast to a compatible function, as I've said earlier.

> and tegra_dc_of_node_put()
> in drivers/gpu/drm/tegra/rgb.c uses the same one-line wrapper around
> of_node_put(), so I followed that existing pattern.  Happy to switch
> to the cast if you'd still prefer it.
> 

I would not insist on the suggested change, as for me both versions
have incomparable advantages.

-- 
Best wishes,
Vladimr