Skip to content

Commit

Permalink
dpll: fix dpll_pin_on_pin_register() for multiple parent pins
Browse files Browse the repository at this point in the history 
In scenario where pin is registered with multiple parent pins via
dpll_pin_on_pin_register(..), all belonging to the same dpll device.
A second call to dpll_pin_on_pin_unregister(..) would cause a call trace,
as it tries to use already released registration resources (due to fix
introduced in b446631). In this scenario pin was registered twice,
so resources are not yet expected to be release until each registered
pin/pin pair is unregistered.

Currently, the following crash/call trace is produced when ice driver is
removed on the system with installed E810T NIC which includes dpll device:

WARNING: CPU: 51 PID: 9155 at drivers/dpll/dpll_core.c:809 dpll_pin_ops+0x20/0x30
RIP: 0010:dpll_pin_ops+0x20/0x30
Call Trace:
 ? __warn+0x7f/0x130
 ? dpll_pin_ops+0x20/0x30
 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]

Fix by adding a parent pointer as a cookie when creating a registration,
also when searching for it. For the regular pins pass NULL, this allows to
create separated registration for each parent the pin is registered with.

Fixes: b446631 ("dpll: fix dpll_xa_ref_*_del() for multiple registrations")
Signed-off-by: Arkadiusz Kubalewski <[email protected]>
Reviewed-by: Jiri Pirko <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
  • Loading branch information
@kubalewski @kuba-moo
kubalewski authored and kuba-moo committed Apr 25, 2024
1 parent 0c81ea5 commit 38d7b94
Showing 1 changed file with 33 additions and 25 deletions.
58 changes: 33 additions & 25 deletions drivers/dpll/dpll_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ struct dpll_pin_registration {
struct list_head list;
const struct dpll_pin_ops *ops;
void *priv;
void *cookie;
};

struct dpll_device *dpll_device_get_by_id(int id)
Expand All @@ -54,20 +55,23 @@ struct dpll_device *dpll_device_get_by_id(int id)

static struct dpll_pin_registration *
dpll_pin_registration_find(struct dpll_pin_ref *ref,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv,
void *cookie)
{
struct dpll_pin_registration *reg;

list_for_each_entry(reg, &ref->registration_list, list) {
if (reg->ops == ops && reg->priv == priv)
if (reg->ops == ops && reg->priv == priv &&
reg->cookie == cookie)
return reg;
}
return NULL;
}

static int
dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv,
void *cookie)
{
struct dpll_pin_registration *reg;
struct dpll_pin_ref *ref;
Expand All @@ -78,7 +82,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
xa_for_each(xa_pins, i, ref) {
if (ref->pin != pin)
continue;
reg = dpll_pin_registration_find(ref, ops, priv);
reg = dpll_pin_registration_find(ref, ops, priv, cookie);
if (reg) {
refcount_inc(&ref->refcount);
return 0;
Expand Down Expand Up @@ -111,6 +115,7 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
}
reg->ops = ops;
reg->priv = priv;
reg->cookie = cookie;
if (ref_exists)
refcount_inc(&ref->refcount);
list_add_tail(&reg->list, &ref->registration_list);
Expand All @@ -119,7 +124,8 @@ dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin,
}

static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv,
void *cookie)
{
struct dpll_pin_registration *reg;
struct dpll_pin_ref *ref;
Expand All @@ -128,7 +134,7 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
xa_for_each(xa_pins, i, ref) {
if (ref->pin != pin)
continue;
reg = dpll_pin_registration_find(ref, ops, priv);
reg = dpll_pin_registration_find(ref, ops, priv, cookie);
if (WARN_ON(!reg))
return -EINVAL;
list_del(&reg->list);
Expand All @@ -146,7 +152,7 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,

static int
dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv, void *cookie)
{
struct dpll_pin_registration *reg;
struct dpll_pin_ref *ref;
Expand All @@ -157,7 +163,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,
xa_for_each(xa_dplls, i, ref) {
if (ref->dpll != dpll)
continue;
reg = dpll_pin_registration_find(ref, ops, priv);
reg = dpll_pin_registration_find(ref, ops, priv, cookie);
if (reg) {
refcount_inc(&ref->refcount);
return 0;
Expand Down Expand Up @@ -190,6 +196,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,
}
reg->ops = ops;
reg->priv = priv;
reg->cookie = cookie;
if (ref_exists)
refcount_inc(&ref->refcount);
list_add_tail(&reg->list, &ref->registration_list);
Expand All @@ -199,7 +206,7 @@ dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll,

static void
dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv, void *cookie)
{
struct dpll_pin_registration *reg;
struct dpll_pin_ref *ref;
Expand All @@ -208,7 +215,7 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
xa_for_each(xa_dplls, i, ref) {
if (ref->dpll != dpll)
continue;
reg = dpll_pin_registration_find(ref, ops, priv);
reg = dpll_pin_registration_find(ref, ops, priv, cookie);
if (WARN_ON(!reg))
return;
list_del(&reg->list);
Expand Down Expand Up @@ -594,14 +601,14 @@ EXPORT_SYMBOL_GPL(dpll_pin_put);

static int
__dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv, void *cookie)
{
int ret;

ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv);
ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv, cookie);
if (ret)
return ret;
ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv);
ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv, cookie);
if (ret)
goto ref_pin_del;
xa_set_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED);
Expand All @@ -610,7 +617,7 @@ __dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
return ret;

ref_pin_del:
dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv);
dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv, cookie);
return ret;
}

Expand Down Expand Up @@ -642,7 +649,7 @@ dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
dpll->clock_id == pin->clock_id)))
ret = -EINVAL;
else
ret = __dpll_pin_register(dpll, pin, ops, priv);
ret = __dpll_pin_register(dpll, pin, ops, priv, NULL);
mutex_unlock(&dpll_lock);

return ret;
Expand All @@ -651,11 +658,11 @@ EXPORT_SYMBOL_GPL(dpll_pin_register);

static void
__dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin,
const struct dpll_pin_ops *ops, void *priv)
const struct dpll_pin_ops *ops, void *priv, void *cookie)
{
ASSERT_DPLL_PIN_REGISTERED(pin);
dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv);
dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv);
dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv, cookie);
dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv, cookie);
if (xa_empty(&pin->dpll_refs))
xa_clear_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED);
}
Expand All @@ -680,7 +687,7 @@ void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin,

mutex_lock(&dpll_lock);
dpll_pin_delete_ntf(pin);
__dpll_pin_unregister(dpll, pin, ops, priv);
__dpll_pin_unregister(dpll, pin, ops, priv, NULL);
mutex_unlock(&dpll_lock);
}
EXPORT_SYMBOL_GPL(dpll_pin_unregister);
Expand Down Expand Up @@ -716,12 +723,12 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
return -EINVAL;

mutex_lock(&dpll_lock);
ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv);
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);
ret = __dpll_pin_register(ref->dpll, pin, ops, priv, parent);
if (ret) {
stop = i;
goto dpll_unregister;
Expand All @@ -735,11 +742,12 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin,
dpll_unregister:
xa_for_each(&parent->dpll_refs, i, ref)
if (i < stop) {
__dpll_pin_unregister(ref->dpll, pin, ops, priv);
__dpll_pin_unregister(ref->dpll, pin, ops, priv,
parent);
dpll_pin_delete_ntf(pin);
}
refcount_dec(&pin->refcount);
dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv);
dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv, pin);
unlock:
mutex_unlock(&dpll_lock);
return ret;
Expand All @@ -764,10 +772,10 @@ 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);
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);
__dpll_pin_unregister(ref->dpll, pin, ops, priv, parent);
mutex_unlock(&dpll_lock);
}
EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister);
Expand Down

0 comments on commit 38d7b94

Please sign in to comment.