Hold the domain lock of the addressed object wherever driver callbacks can run: - netlink requests addressing a device or a pin, from pre_doit to post_doit - dumps, for each object - notifications, while the message is being built and sent Callbacks of a device run under the domain of the device. Callbacks of a pin run under the domain of the pin, also when another driver registered the pin with its dpll or under its parent pin. All callbacks a message about a pin needs are thus covered by one lock, and only one domain lock is ever held. Domains have no lock order, so registrations can connect them in any way. A request is atomic against other requests addressing objects of that domain, and messages of one object are sent in the order they were built. Requests resolving an id run no callbacks, so they do not take the lock. __dpll_{device,pin}_change_ntf() now also require the domain lock of the object. A callback can therefore only notify objects of the domain it runs under. Callbacks stay serialized by dpll_lock for now. Signed-off-by: Ivan Vecera --- drivers/dpll/dpll_core.c | 18 +++++---- drivers/dpll/dpll_core.h | 6 ++- drivers/dpll/dpll_netlink.c | 78 ++++++++++++++++++++++++++++--------- 3 files changed, 76 insertions(+), 26 deletions(-) diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c index d39c4905172f2d..dd09f528170994 100644 --- a/drivers/dpll/dpll_core.c +++ b/drivers/dpll/dpll_core.c @@ -520,7 +520,7 @@ dpll_device_registration_find(struct dpll_device *dpll, * * Make dpll device available for user space. * - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and the domain lock of @dpll * Return: * * 0 on success * * negative - error value @@ -579,7 +579,7 @@ EXPORT_SYMBOL_GPL(dpll_device_register); * * Unregister device, make it unavailable for userspace. * Note: It does not free the memory - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and the domain lock of @dpll */ void dpll_device_unregister(struct dpll_device *dpll, const struct dpll_device_ops *ops, void *priv) @@ -928,7 +928,7 @@ __dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, * @ops: ops for a dpll pin ops * @priv: pointer to private information of owner * - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and domain locks * Return: * * 0 on success * * negative - error value @@ -989,7 +989,9 @@ static void dpll_pin_ref_sync_pair_del(u32 ref_sync_pin_id) ref_sync_pin = xa_load(&pin->ref_sync_pins, ref_sync_pin_id); if (ref_sync_pin) { xa_erase(&pin->ref_sync_pins, ref_sync_pin_id); + mutex_lock(&pin->domain->lock); __dpll_pin_change_ntf(pin); + mutex_unlock(&pin->domain->lock); } } } @@ -1016,7 +1018,7 @@ __dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, * @priv: pointer to private information of owner * * Note: It does not free the memory - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and domain locks */ void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, const struct dpll_pin_ops *ops, void *priv) @@ -1042,7 +1044,7 @@ EXPORT_SYMBOL_GPL(dpll_pin_unregister); * Register a pin with a parent pin, create references between them and * between newly registered pin and dplls connected with a parent pin. * - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and domain locks * Return: * * 0 on success * * negative - error value @@ -1099,7 +1101,7 @@ EXPORT_SYMBOL_GPL(dpll_pin_on_pin_register); * @ops: ops for a dpll pin * @priv: pointer to private information of owner * - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and domain locks * Note: It does not free the memory */ void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin, @@ -1129,7 +1131,7 @@ EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister); * * Once pins are paired, the user-space configuration of reference sync pair * is possible. - * Context: Acquires a lock (dpll_lock) + * Context: Acquires dpll_lock and domain locks * Return: * * 0 on success * * negative - error value @@ -1142,7 +1144,9 @@ int dpll_pin_ref_sync_pair_add(struct dpll_pin *pin, mutex_lock(&dpll_lock); ret = xa_insert(&pin->ref_sync_pins, ref_sync_pin->id, ref_sync_pin, GFP_KERNEL); + mutex_lock(&pin->domain->lock); __dpll_pin_change_ntf(pin); + mutex_unlock(&pin->domain->lock); mutex_unlock(&dpll_lock); return ret; diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h index 6705e81bc11de9..630c5a1ca572c2 100644 --- a/drivers/dpll/dpll_core.h +++ b/drivers/dpll/dpll_core.h @@ -22,10 +22,14 @@ * @clock_id: clock_id of the objects in the domain * @module: module of the objects in the domain * @refcount: number of dpll devices and pins in the domain - * @lock: lock of the domain + * @lock: serializes driver callbacks of the domain * * All dpll devices and pins created with the same (module, clock_id) pair * belong to one domain, in practice to one chip or card. + * + * Callbacks of a device run under the domain of the device and callbacks of a + * pin under the domain of the pin, whichever driver registered them. Only one + * domain lock is held at a time, so domains have no lock order. **/ struct dpll_domain { u32 id; diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c index e5380d95f238b3..1b4e3a0166a744 100644 --- a/drivers/dpll/dpll_netlink.c +++ b/drivers/dpll/dpll_netlink.c @@ -861,27 +861,39 @@ dpll_device_event_send(enum dpll_cmd event, struct dpll_device *dpll) int dpll_device_create_ntf(struct dpll_device *dpll) { + int ret; + + mutex_lock(&dpll->domain->lock); dpll_device_notify(dpll, DPLL_DEVICE_CREATED); - return dpll_device_event_send(DPLL_CMD_DEVICE_CREATE_NTF, dpll); + ret = dpll_device_event_send(DPLL_CMD_DEVICE_CREATE_NTF, dpll); + mutex_unlock(&dpll->domain->lock); + + return ret; } int dpll_device_delete_ntf(struct dpll_device *dpll) { + int ret; + + mutex_lock(&dpll->domain->lock); dpll_device_notify(dpll, DPLL_DEVICE_DELETED); - return dpll_device_event_send(DPLL_CMD_DEVICE_DELETE_NTF, dpll); + ret = dpll_device_event_send(DPLL_CMD_DEVICE_DELETE_NTF, dpll); + mutex_unlock(&dpll->domain->lock); + + return ret; } /** * __dpll_device_change_ntf - notify that the dpll device has been changed * @dpll: registered dpll pointer * - * Context: caller must hold dpll_lock. Suitable for use inside device - * callbacks which are already invoked under dpll_lock. + * Context: caller must hold dpll_lock and the domain lock of @dpll. + * Suitable for use inside callbacks running under these locks. * Return: 0 if succeeds, error code otherwise. */ int __dpll_device_change_ntf(struct dpll_device *dpll) { - lockdep_assert_held(&dpll_lock); + lockdep_assert_held(&dpll->domain->lock); dpll_device_notify(dpll, DPLL_DEVICE_CHANGED); return dpll_device_event_send(DPLL_CMD_DEVICE_CHANGE_NTF, dpll); } @@ -891,7 +903,7 @@ EXPORT_SYMBOL_GPL(__dpll_device_change_ntf); * dpll_device_change_ntf - notify that the dpll device has been changed * @dpll: registered dpll pointer * - * Context: acquires and holds a dpll_lock. + * Context: acquires and holds dpll_lock and the domain lock of @dpll. * Return: 0 if succeeds, error code otherwise. */ int dpll_device_change_ntf(struct dpll_device *dpll) @@ -899,7 +911,9 @@ int dpll_device_change_ntf(struct dpll_device *dpll) int ret; mutex_lock(&dpll_lock); + mutex_lock(&dpll->domain->lock); ret = __dpll_device_change_ntf(dpll); + mutex_unlock(&dpll->domain->lock); mutex_unlock(&dpll_lock); return ret; @@ -941,27 +955,39 @@ dpll_pin_event_send(enum dpll_cmd event, struct dpll_pin *pin) int dpll_pin_create_ntf(struct dpll_pin *pin, u64 src_clock_id) { + int ret; + + mutex_lock(&pin->domain->lock); dpll_pin_notify(pin, src_clock_id, DPLL_PIN_CREATED); - return dpll_pin_event_send(DPLL_CMD_PIN_CREATE_NTF, pin); + ret = dpll_pin_event_send(DPLL_CMD_PIN_CREATE_NTF, pin); + mutex_unlock(&pin->domain->lock); + + return ret; } int dpll_pin_delete_ntf(struct dpll_pin *pin, u64 src_clock_id) { + int ret; + + mutex_lock(&pin->domain->lock); dpll_pin_notify(pin, src_clock_id, DPLL_PIN_DELETED); - return dpll_pin_event_send(DPLL_CMD_PIN_DELETE_NTF, pin); + ret = dpll_pin_event_send(DPLL_CMD_PIN_DELETE_NTF, pin); + mutex_unlock(&pin->domain->lock); + + return ret; } /** * __dpll_pin_change_ntf - notify that the pin has been changed * @pin: registered pin pointer * - * Context: caller must hold dpll_lock. Suitable for use inside pin - * callbacks which are already invoked under dpll_lock. + * Context: caller must hold dpll_lock and the domain lock of @pin. + * Suitable for use inside callbacks running under these locks. * Return: 0 if succeeds, error code otherwise. */ int __dpll_pin_change_ntf(struct dpll_pin *pin) { - lockdep_assert_held(&dpll_lock); + lockdep_assert_held(&pin->domain->lock); dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED); return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin); } @@ -971,7 +997,7 @@ EXPORT_SYMBOL_GPL(__dpll_pin_change_ntf); * dpll_pin_change_ntf - notify that the pin has been changed * @pin: registered pin pointer * - * Context: acquires and holds a dpll_lock. + * Context: acquires and holds dpll_lock and the domain lock of @pin. * Return: 0 if succeeds, error code otherwise. */ int dpll_pin_change_ntf(struct dpll_pin *pin) @@ -979,7 +1005,9 @@ int dpll_pin_change_ntf(struct dpll_pin *pin) int ret; mutex_lock(&dpll_lock); + mutex_lock(&pin->domain->lock); ret = __dpll_pin_change_ntf(pin); + mutex_unlock(&pin->domain->lock); mutex_unlock(&dpll_lock); return ret; @@ -1780,7 +1808,9 @@ int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) ret = -EMSGSIZE; break; } + mutex_lock(&pin->domain->lock); ret = dpll_cmd_pin_get_one(skb, pin, cb->extack); + mutex_unlock(&pin->domain->lock); if (ret) { genlmsg_cancel(skb, hdr); if (ret == -ENODEV) { @@ -1999,7 +2029,9 @@ int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) ret = -EMSGSIZE; break; } + mutex_lock(&dpll->domain->lock); ret = dpll_device_get_one(dpll, skb, cb->extack); + mutex_unlock(&dpll->domain->lock); if (ret) { genlmsg_cancel(skb, hdr); break; @@ -2018,6 +2050,7 @@ int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, struct genl_info *info) { + struct dpll_device *dpll; u32 id; if (GENL_REQ_ATTR_CHECK(info, DPLL_A_ID)) @@ -2025,11 +2058,13 @@ int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, mutex_lock(&dpll_lock); id = nla_get_u32(info->attrs[DPLL_A_ID]); - info->user_ptr[0] = dpll_device_get_by_id(id); - if (!info->user_ptr[0]) { + dpll = dpll_device_get_by_id(id); + if (!dpll) { NL_SET_ERR_MSG(info->extack, "device not found"); goto unlock; } + info->user_ptr[0] = dpll; + mutex_lock(&dpll->domain->lock); return 0; unlock: mutex_unlock(&dpll_lock); @@ -2039,6 +2074,9 @@ int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, void dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb, struct genl_info *info) { + struct dpll_device *dpll = info->user_ptr[0]; + + mutex_unlock(&dpll->domain->lock); mutex_unlock(&dpll_lock); } @@ -2061,6 +2099,7 @@ dpll_unlock_doit(const struct genl_split_ops *ops, struct sk_buff *skb, int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, struct genl_info *info) { + struct dpll_pin *pin; int ret; mutex_lock(&dpll_lock); @@ -2068,14 +2107,14 @@ int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, ret = -EINVAL; goto unlock_dev; } - info->user_ptr[0] = xa_load(&dpll_pin_xa, - nla_get_u32(info->attrs[DPLL_A_PIN_ID])); - if (!info->user_ptr[0] || - !dpll_pin_available(info->user_ptr[0])) { + pin = xa_load(&dpll_pin_xa, nla_get_u32(info->attrs[DPLL_A_PIN_ID])); + if (!pin || !dpll_pin_available(pin)) { NL_SET_ERR_MSG(info->extack, "pin not found"); ret = -ENODEV; goto unlock_dev; } + info->user_ptr[0] = pin; + mutex_lock(&pin->domain->lock); return 0; @@ -2087,5 +2126,8 @@ int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb, void dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb, struct genl_info *info) { + struct dpll_pin *pin = info->user_ptr[0]; + + mutex_unlock(&pin->domain->lock); mutex_unlock(&dpll_lock); } -- 2.56.0