Re: [PATCH net-next 04/10] dpll: take domain locks around driver callbacks

[Petr Oros <poros@redhat.com>]

On 11/6/26 1:09 PM, Ivan Vecera wrote:
> 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 <ivecera@redhat.com>
> ---
>   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);
>   }

Reviewed-by: Petr Oros <poros@redhat.com>