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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

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