Refactor the reference counting mechanism for DPLL devices and pins to
improve consistency and prevent potential lifetime issues.
Introduce internal helpers __dpll_{device,pin}_{hold,put}() to
centralize reference management.
Update the internal XArray reference helpers (dpll_xa_ref_*) to
automatically grab a reference to the target object when it is added to
a list, and release it when removed. This ensures that objects linked
internally (e.g., pins referenced by parent pins) are properly kept
alive without relying on the caller to manually manage the count.
Consequently, remove the now redundant manual `refcount_inc/dec` calls
in dpll_pin_on_pin_{,un}register()`, as ownership is now correctly handled
by the dpll_xa_ref_* functions.
Additionally, ensure that dpll_device_{,un}register()` takes/releases
a reference to the device, ensuring the device object remains valid for
the duration of its registration.
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
drivers/dpll/dpll_core.c | 74 +++++++++++++++++++++++++++-------------
1 file changed, 50 insertions(+), 24 deletions(-)
diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 156f95de8e8e8..f2a77eb1b9916 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -84,6 +84,45 @@ void dpll_pin_notify(struct dpll_pin *pin, unsigned long action)
call_dpll_notifiers(action, &info);
}
+static void __dpll_device_hold(struct dpll_device *dpll)
+{
+ refcount_inc(&dpll->refcount);
+}
+
+static void __dpll_device_put(struct dpll_device *dpll)
+{
+ if (refcount_dec_and_test(&dpll->refcount)) {
+ ASSERT_DPLL_NOT_REGISTERED(dpll);
+ WARN_ON_ONCE(!xa_empty(&dpll->pin_refs));
+ xa_destroy(&dpll->pin_refs);
+ xa_erase(&dpll_device_xa, dpll->id);
+ WARN_ON(!list_empty(&dpll->registration_list));
+ kfree(dpll);
+ }
+}
+
+static void __dpll_pin_hold(struct dpll_pin *pin)
+{
+ refcount_inc(&pin->refcount);
+}
+
+static void dpll_pin_idx_free(u32 pin_idx);
+static void dpll_pin_prop_free(struct dpll_pin_properties *prop);
+
+static void __dpll_pin_put(struct dpll_pin *pin)
+{
+ if (refcount_dec_and_test(&pin->refcount)) {
+ xa_erase(&dpll_pin_xa, pin->id);
+ xa_destroy(&pin->dpll_refs);
+ xa_destroy(&pin->parent_refs);
+ xa_destroy(&pin->ref_sync_pins);
+ dpll_pin_prop_free(&pin->prop);
+ fwnode_handle_put(pin->fwnode);
+ dpll_pin_idx_free(pin->pin_idx);
+ kfree_rcu(pin, rcu);
+ }
+}
+
struct dpll_device *dpll_device_get_by_id(int id)
{
if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
@@ -155,6 +194,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
reg->ops = ops;
reg->priv = priv;
reg->cookie = cookie;
+ __dpll_pin_hold(pin);
if (ref_exists)
refcount_inc(&ref->refcount);
list_add_tail(®->list, &ref->registration_list);
@@ -177,6 +217,7 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
if (WARN_ON(!reg))
return -EINVAL;
list_del(®->list);
+ __dpll_pin_put(pin);
kfree(reg);
if (refcount_dec_and_test(&ref->refcount)) {
xa_erase(xa_pins, i);
@@ -236,6 +277,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,
reg->ops = ops;
reg->priv = priv;
reg->cookie = cookie;
+ __dpll_device_hold(dpll);
if (ref_exists)
refcount_inc(&ref->refcount);
list_add_tail(®->list, &ref->registration_list);
@@ -258,6 +300,7 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
if (WARN_ON(!reg))
return;
list_del(®->list);
+ __dpll_device_put(dpll);
kfree(reg);
if (refcount_dec_and_test(&ref->refcount)) {
xa_erase(xa_dplls, i);
@@ -328,8 +371,8 @@ dpll_device_get(u64 clock_id, u32 device_idx, struct module *module)
if (dpll->clock_id == clock_id &&
dpll->device_idx == device_idx &&
dpll->module == module) {
+ __dpll_device_hold(dpll);
ret = dpll;
- refcount_inc(&ret->refcount);
break;
}
}
@@ -352,14 +395,7 @@ EXPORT_SYMBOL_GPL(dpll_device_get);
void dpll_device_put(struct dpll_device *dpll)
{
mutex_lock(&dpll_lock);
- if (refcount_dec_and_test(&dpll->refcount)) {
- ASSERT_DPLL_NOT_REGISTERED(dpll);
- WARN_ON_ONCE(!xa_empty(&dpll->pin_refs));
- xa_destroy(&dpll->pin_refs);
- xa_erase(&dpll_device_xa, dpll->id);
- WARN_ON(!list_empty(&dpll->registration_list));
- kfree(dpll);
- }
+ __dpll_device_put(dpll);
mutex_unlock(&dpll_lock);
}
EXPORT_SYMBOL_GPL(dpll_device_put);
@@ -421,6 +457,7 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
reg->ops = ops;
reg->priv = priv;
dpll->type = type;
+ __dpll_device_hold(dpll);
first_registration = list_empty(&dpll->registration_list);
list_add_tail(®->list, &dpll->registration_list);
if (!first_registration) {
@@ -460,6 +497,7 @@ void dpll_device_unregister(struct dpll_device *dpll,
return;
}
list_del(®->list);
+ __dpll_device_put(dpll);
kfree(reg);
if (!list_empty(&dpll->registration_list)) {
@@ -671,8 +709,8 @@ dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module,
if (pos->clock_id == clock_id &&
pos->pin_idx == pin_idx &&
pos->module == module) {
+ __dpll_pin_hold(pos);
ret = pos;
- refcount_inc(&ret->refcount);
break;
}
}
@@ -695,16 +733,7 @@ EXPORT_SYMBOL_GPL(dpll_pin_get);
void dpll_pin_put(struct dpll_pin *pin)
{
mutex_lock(&dpll_lock);
- if (refcount_dec_and_test(&pin->refcount)) {
- xa_erase(&dpll_pin_xa, pin->id);
- xa_destroy(&pin->dpll_refs);
- xa_destroy(&pin->parent_refs);
- xa_destroy(&pin->ref_sync_pins);
- dpll_pin_prop_free(&pin->prop);
- fwnode_handle_put(pin->fwnode);
- dpll_pin_idx_free(pin->pin_idx);
- kfree_rcu(pin, rcu);
- }
+ __dpll_pin_put(pin);
mutex_unlock(&dpll_lock);
}
EXPORT_SYMBOL_GPL(dpll_pin_put);
@@ -745,8 +774,8 @@ struct dpll_pin *fwnode_dpll_pin_find(struct fwnode_handle *fwnode)
mutex_lock(&dpll_lock);
xa_for_each(&dpll_pin_xa, index, pin) {
if (pin->fwnode == fwnode) {
+ __dpll_pin_hold(pin);
ret = pin;
- refcount_inc(&ret->refcount);
break;
}
}
@@ -971,7 +1000,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv, pin);
if (ret)
goto unlock;
- refcount_inc(&pin->refcount);
xa_for_each(&parent->dpll_refs, i, ref) {
ret = __dpll_pin_register(ref->dpll, pin, ops, priv, parent);
if (ret) {
@@ -991,7 +1019,6 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
parent);
dpll_pin_delete_ntf(pin);
}
- refcount_dec(&pin->refcount);
dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv, pin);
unlock:
mutex_unlock(&dpll_lock);
@@ -1018,7 +1045,6 @@ void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin,
mutex_lock(&dpll_lock);
dpll_pin_delete_ntf(pin);
dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv, pin);
- refcount_dec(&pin->refcount);
xa_for_each(&pin->dpll_refs, i, ref)
__dpll_pin_unregister(ref->dpll, pin, ops, priv, parent);
mutex_unlock(&dpll_lock);
--
2.52.0This is an AI-generated review of your patch.
Dunno if there's a reason for having this fixed by a later patch,
if not let's fix. I'm sending the review mostly because of the
comments on patch 12.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
dpll: Enhance and consolidate reference counting logic
This commit refactors DPLL reference counting by introducing centralized
__dpll_{device,pin}_{hold,put}() helpers and updating dpll_xa_ref_*
functions to automatically manage references when adding/removing entries.
> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> index 156f95de8e8e..f2a77eb1b991 100644
[ ... ]
> @@ -155,6 +194,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
> reg->ops = ops;
> reg->priv = priv;
> reg->cookie = cookie;
> + __dpll_pin_hold(pin);
> if (ref_exists)
> refcount_inc(&ref->refcount);
> list_add_tail(®->list, &ref->registration_list);
The new __dpll_pin_hold(pin) call is added here for the success path, but
looking at the context above this hunk, there is an early return path when
a duplicate registration is found:
reg = dpll_pin_registration_find(ref, ops, priv, cookie);
if (reg) {
refcount_inc(&ref->refcount);
return 0;
}
This early return path increments ref->refcount and returns success, but
does not call __dpll_pin_hold(pin). Meanwhile, dpll_xa_ref_pin_del() always
calls __dpll_pin_put(pin). Could this cause a refcount underflow if a caller
attempts duplicate registration and then later unregistration is attempted?
Note: This appears to be addressed by 934c22045c2a ("dpll: Prevent duplicate
registrations") later in the series, which changes this early return to
return -EEXIST instead of success.
[ ... ]
> @@ -236,6 +277,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,
> reg->ops = ops;
> reg->priv = priv;
> reg->cookie = cookie;
> + __dpll_device_hold(dpll);
> if (ref_exists)
> refcount_inc(&ref->refcount);
> list_add_tail(®->list, &ref->registration_list);
The same pattern exists in dpll_xa_ref_dpll_add(): the early return path for
duplicate registrations increments ref->refcount without calling
__dpll_device_hold(dpll), while dpll_xa_ref_dpll_del() always calls
__dpll_device_put(dpll). The same fix in 934c22045c2a addresses this.
--
pw-bot: cr
On 1/21/26 1:16 AM, Jakub Kicinski wrote: > This is an AI-generated review of your patch. > > Dunno if there's a reason for having this fixed by a later patch, > if not let's fix. I'm sending the review mostly because of the > comments on patch 12. Will reorder these patches... Maybe it would be better to send patch 9 separately to net as this is the fix for the bug we found during development of this series. Ivan
On Wed, Jan 21, 2026 at 09:18:02AM +0100, Ivan Vecera wrote: > On 1/21/26 1:16 AM, Jakub Kicinski wrote: > > This is an AI-generated review of your patch. > > > > Dunno if there's a reason for having this fixed by a later patch, > > if not let's fix. I'm sending the review mostly because of the > > comments on patch 12. > Will reorder these patches... Maybe it would be better to send patch 9 > separately to net as this is the fix for the bug we found during > development of this series. Hi Ivan, If it is a but in net, then yes, that sounds like a good idea to me. Please include a Fixes tag if you take that route.
On 1/23/26 3:58 PM, Simon Horman wrote: > On Wed, Jan 21, 2026 at 09:18:02AM +0100, Ivan Vecera wrote: >> On 1/21/26 1:16 AM, Jakub Kicinski wrote: >>> This is an AI-generated review of your patch. >>> >>> Dunno if there's a reason for having this fixed by a later patch, >>> if not let's fix. I'm sending the review mostly because of the >>> comments on patch 12. >> Will reorder these patches... Maybe it would be better to send patch 9 >> separately to net as this is the fix for the bug we found during >> development of this series. > > Hi Ivan, > > If it is a but in net, then yes, that sounds like a good idea to me. > > Please include a Fixes tag if you take that route. > Yes, it was submitted to net with Fixes and recently merged as https://git.kernel.org/netdev/net/c/f3ddbaaaaf4d Ivan
On Fri, Jan 23, 2026 at 04:27:21PM +0100, Ivan Vecera wrote: > On 1/23/26 3:58 PM, Simon Horman wrote: > > On Wed, Jan 21, 2026 at 09:18:02AM +0100, Ivan Vecera wrote: > > > On 1/21/26 1:16 AM, Jakub Kicinski wrote: > > > > This is an AI-generated review of your patch. > > > > > > > > Dunno if there's a reason for having this fixed by a later patch, > > > > if not let's fix. I'm sending the review mostly because of the > > > > comments on patch 12. > > > Will reorder these patches... Maybe it would be better to send patch 9 > > > separately to net as this is the fix for the bug we found during > > > development of this series. > > > > Hi Ivan, > > > > If it is a but in net, then yes, that sounds like a good idea to me. > > > > Please include a Fixes tag if you take that route. > > > Yes, it was submitted to net with Fixes and recently merged as > > https://git.kernel.org/netdev/net/c/f3ddbaaaaf4d Thanks, sorry for missing that earlier.
© 2016 - 2026 Red Hat, Inc.