Messages in this thread Patch in this message | | | Date | Mon, 22 Apr 2024 15:31:18 +0200 | From | Jiri Pirko <> | Subject | Re: [PATCH net] dpll: fix dpll_pin_registration missing refcount |
| |
Fri, Apr 19, 2024 at 09:47:11PM CEST, arkadiusz.kubalewski@intel.com wrote: >In scenario where pin is registered with multiple parent pins via >dpll_pin_on_pin_register(..), belonging to the same dpll device, >and each time with the same set of ops/priv data, a reference >between a pin and dpll is created once and then refcounted, at the same >time the dpll_pin_registration is only checked for existence and created >if does not exist. This is wrong, as for the same ops/priv data a >registration shall be also refcounted, a child pin is also registered >with dpll device, until each child is unregistered the registration data >shall exist.
I read this 3 time, don't undestand clearly the matter of the problem. Could you perhaps make it somehow visual?
> >Add refcount and check if all registrations are dropped before releasing >dpll_pin_registration resources. > >Currently, the following crash/call trace is produced when ice driver is >removed on the system with installed NIC which includes dpll device: > >WARNING: CPU: 51 PID: 9155 at drivers/dpll/dpll_core.c:809 dpll_pin_ops+0x20/0x30 >Call Trace: > dpll_msg_add_pin_freq+0x37/0x1d0 > dpll_cmd_pin_get_one+0x1c0/0x400 > ? __nlmsg_put+0x63/0x80 > dpll_pin_event_send+0x93/0x140 > dpll_pin_on_pin_unregister+0x3f/0x100 > ice_dpll_deinit_pins+0xa1/0x230 [ice] > ice_remove+0xf1/0x210 [ice] > >Fixes: b446631f355e ("dpll: fix dpll_xa_ref_*_del() for multiple registrations") >Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- > drivers/dpll/dpll_core.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > >diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >index 64eaca80d736..7ababa327c0c 100644 >--- a/drivers/dpll/dpll_core.c >+++ b/drivers/dpll/dpll_core.c >@@ -40,6 +40,7 @@ struct dpll_device_registration { > > struct dpll_pin_registration { > struct list_head list; >+ refcount_t refcount; > const struct dpll_pin_ops *ops; > void *priv; > }; >@@ -81,6 +82,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin, > reg = dpll_pin_registration_find(ref, ops, priv); > if (reg) { > refcount_inc(&ref->refcount); >+ refcount_inc(®->refcount);
I don't like this. Registration is supposed to be created for a single registration. Not you create one for many and refcount it.
Instead of this, I suggest to extend __dpll_pin_register() for a "void *cookie" arg. That would be NULL for dpll_pin_register() caller. For dpll_pin_on_pin_register() caller, it would pass "parent" pointer.
Than dpll_xa_ref_pin_add() can pass this cookie value to dpll_pin_registration_find(). The if case there would look like: if (reg->ops == ops && reg->priv == priv && reg->cookie == cookie)
This way, we will create separate "sub-registration" for each parent.
Makes sense?
> return 0; > } > ref_exists = true; >@@ -113,6 +115,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin, > reg->priv = priv; > if (ref_exists) > refcount_inc(&ref->refcount); >+ refcount_set(®->refcount, 1); > list_add_tail(®->list, &ref->registration_list); > > return 0; >@@ -131,8 +134,10 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin, > reg = dpll_pin_registration_find(ref, ops, priv); > if (WARN_ON(!reg)) > return -EINVAL; >- list_del(®->list); >- kfree(reg); >+ if (refcount_dec_and_test(®->refcount)) { >+ list_del(®->list); >+ kfree(reg); >+ } > if (refcount_dec_and_test(&ref->refcount)) { > xa_erase(xa_pins, i); > WARN_ON(!list_empty(&ref->registration_list)); >@@ -160,6 +165,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll, > reg = dpll_pin_registration_find(ref, ops, priv); > if (reg) { > refcount_inc(&ref->refcount); >+ refcount_inc(®->refcount); > return 0; > } > ref_exists = true; >@@ -192,6 +198,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll, > reg->priv = priv; > if (ref_exists) > refcount_inc(&ref->refcount); >+ refcount_set(®->refcount, 1); > list_add_tail(®->list, &ref->registration_list); > > return 0; >@@ -211,8 +218,10 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll, > reg = dpll_pin_registration_find(ref, ops, priv); > if (WARN_ON(!reg)) > return; >- list_del(®->list); >- kfree(reg); >+ if (refcount_dec_and_test(®->refcount)) { >+ list_del(®->list); >+ kfree(reg); >+ } > if (refcount_dec_and_test(&ref->refcount)) { > xa_erase(xa_dplls, i); > WARN_ON(!list_empty(&ref->registration_list)); > >base-commit: ac1a21db32eda8a09076bad025d7b848dd086d28 >-- >2.38.1 >
| |