| Message ID | 20260818175750.4205-1-dragonliu2018@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: qcom-cci: fix device_node refcount leak in cci_probe()/cci_remove() | expand |
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>
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
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.
On 8/19/26 9:02 AM, Vladimir Zapolskiy wrote: > 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. Just seeing (void (*)(void*))foo scares me So for the current iteration Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> Konrad
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); } }
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(-)