RE: [PATCH 2/6] hwspinlock: allow sharing of hwspinlocks

[Loic PALLARDY <loic.pallardy@st.com>]

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org <linux-remoteproc-
> owner@vger.kernel.org> On Behalf Of Fabien Dessenne
> Sent: mercredi 13 mars 2019 16:51
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Alexandre TORGUE
> <alexandre.torgue@st.com>; Jonathan Corbet <corbet@lwn.net>; linux-
> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> linux-arm-kernel@lists.infradead.org; linux-doc@vger.kernel.org
> Cc: Fabien DESSENNE <fabien.dessenne@st.com>; Benjamin GAIGNARD
> <benjamin.gaignard@st.com>
> Subject: [PATCH 2/6] hwspinlock: allow sharing of hwspinlocks
> 
> The current implementation does not allow different devices to use a
> common hwspinlock. Offer the possibility to use the same hwspinlock by
> several users.
> If a device registers to the framework with #hwlock-cells = 2, then
> the second parameter of the 'hwlocks' DeviceTree property defines
> whether an hwlock is requested for an exclusive or a shared usage.
> If a device registers with #hwlock-cells = 1, then all the hwlocks are
> for an exclusive usage.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>

Looks good for me.
Acked-by: Loic Pallardy <loic.pallardy@st.com>
Regards,
Loic

> ---
>  Documentation/hwspinlock.txt             | 10 ++--
>  drivers/hwspinlock/hwspinlock_core.c     | 82
> +++++++++++++++++++++++++-------
>  drivers/hwspinlock/hwspinlock_internal.h |  2 +
>  3 files changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
> index ed640a2..e6ce2dd 100644
> --- a/Documentation/hwspinlock.txt
> +++ b/Documentation/hwspinlock.txt
> @@ -54,9 +54,11 @@ Should be called from a process context (might sleep).
>    struct hwspinlock *hwspin_lock_request_specific(unsigned int id);
> 
>  Assign a specific hwspinlock id and return its address, or NULL
> -if that hwspinlock is already in use. Usually board code will
> -be calling this function in order to reserve specific hwspinlock
> -ids for predefined purposes.
> +if that hwspinlock is already in use and not shared. If that specific
> +hwspinlock is declared as shared, it can be requested and used by
> +several users.
> +Usually board code will be calling this function in order to reserve
> +specific hwspinlock ids for predefined purposes.
> 
>  Should be called from a process context (might sleep).
> 
> @@ -368,11 +370,13 @@ of which represents a single hardware lock::
>  	* struct hwspinlock - this struct represents a single hwspinlock
> instance
>  	* @bank: the hwspinlock_device structure which owns this lock
>  	* @lock: initialized and used by hwspinlock core
> +	* @refcount: number of users (when shared)
>  	* @priv: private data, owned by the underlying platform-specific
> hwspinlock drv
>  	*/
>  	struct hwspinlock {
>  		struct hwspinlock_device *bank;
>  		spinlock_t lock;
> +		unsigned int refcount;
>  		void *priv;
>  	};
> 
> diff --git a/drivers/hwspinlock/hwspinlock_core.c
> b/drivers/hwspinlock/hwspinlock_core.c
> index 2bad40d..53afdeb 100644
> --- a/drivers/hwspinlock/hwspinlock_core.c
> +++ b/drivers/hwspinlock/hwspinlock_core.c
> @@ -25,6 +25,8 @@
> 
>  /* radix tree tags */
>  #define HWSPINLOCK_UNUSED	(0) /* tags an hwspinlock as unused
> */
> +#define HWSPINLOCK_EXCLUSIVE	(1) /* tags an hwspinlock as exclusive
> */
> +#define HWSPINLOCK_SHARED	(2) /* tags an hwspinlock as shared */
> 
>  /*
>   * A radix tree is used to maintain the available hwspinlock instances.
> @@ -291,7 +293,7 @@ EXPORT_SYMBOL_GPL(__hwspin_unlock);
>   * @hwlock_spec: hwlock specifier as found in the device tree
>   *
>   * This is a simple translation function, suitable for hwspinlock platform
> - * drivers that only has a lock specifier length of 1.
> + * drivers that only has a lock specifier length of 1 or 2.
>   *
>   * Returns a relative index of the lock within a specified bank on success,
>   * or -EINVAL on invalid specifier cell count.
> @@ -299,7 +301,8 @@ EXPORT_SYMBOL_GPL(__hwspin_unlock);
>  static inline int
>  of_hwspin_lock_simple_xlate(const struct of_phandle_args *hwlock_spec)
>  {
> -	if (WARN_ON(hwlock_spec->args_count != 1))
> +	if (WARN_ON(hwlock_spec->args_count != 1 &&
> +		    hwlock_spec->args_count != 2))
>  		return -EINVAL;
> 
>  	return hwlock_spec->args[0];
> @@ -322,11 +325,12 @@ of_hwspin_lock_simple_xlate(const struct
> of_phandle_args *hwlock_spec)
>  int of_hwspin_lock_get_id(struct device_node *np, int index)
>  {
>  	struct of_phandle_args args;
> -	struct hwspinlock *hwlock;
> +	struct hwspinlock *hwlock, *tmp;
>  	struct radix_tree_iter iter;
>  	void **slot;
>  	int id;
>  	int ret;
> +	unsigned int tag;
> 
>  	ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells",
> index,
>  					 &args);
> @@ -361,6 +365,37 @@ int of_hwspin_lock_get_id(struct device_node *np,
> int index)
>  	}
>  	id += hwlock->bank->base_id;
> 
> +	/* Set the EXCLUSIVE / SHARED tag */
> +	if (args.args_count == 2 && args.args[1]) {
> +		/* Tag SHARED unless already tagged EXCLUSIVE */
> +		if (radix_tree_tag_get(&hwspinlock_tree, id,
> +				       HWSPINLOCK_EXCLUSIVE)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		tag = HWSPINLOCK_SHARED;
> +	} else {
> +		/* Tag EXCLUSIVE unless already tagged SHARED */
> +		if (radix_tree_tag_get(&hwspinlock_tree, id,
> +				       HWSPINLOCK_SHARED)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		tag = HWSPINLOCK_EXCLUSIVE;
> +	}
> +
> +	/* mark this hwspinlock */
> +	hwlock = radix_tree_lookup(&hwspinlock_tree, id);
> +	if (!hwlock) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	tmp = radix_tree_tag_set(&hwspinlock_tree, id, tag);
> +
> +	/* self-sanity check which should never fail */
> +	WARN_ON(tmp != hwlock);
> +
>  out:
>  	of_node_put(args.np);
>  	return ret ? ret : id;
> @@ -483,6 +518,7 @@ int hwspin_lock_register(struct hwspinlock_device
> *bank, struct device *dev,
> 
>  		spin_lock_init(&hwlock->lock);
>  		hwlock->bank = bank;
> +		hwlock->refcount = 0;
> 
>  		ret = hwspin_lock_register_single(hwlock, base_id + i);
>  		if (ret)
> @@ -625,7 +661,7 @@ static int __hwspin_lock_request(struct hwspinlock
> *hwlock)
>  {
>  	struct device *dev = hwlock->bank->dev;
>  	struct hwspinlock *tmp;
> -	int ret;
> +	int ret, id;
> 
>  	/* prevent underlying implementation from being removed */
>  	if (!try_module_get(dev->driver->owner)) {
> @@ -642,13 +678,18 @@ static int __hwspin_lock_request(struct hwspinlock
> *hwlock)
>  		return ret;
>  	}
> 
> +	/* update shareable refcount */
> +	id = hwlock_to_id(hwlock);
> +	if (radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_SHARED)
> &&
> +	    hwlock->refcount++)
> +		goto out;
> +
>  	/* mark hwspinlock as used, should not fail */
> -	tmp = radix_tree_tag_clear(&hwspinlock_tree,
> hwlock_to_id(hwlock),
> -
> 	HWSPINLOCK_UNUSED);
> +	tmp = radix_tree_tag_clear(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
> 
>  	/* self-sanity check that should never fail */
>  	WARN_ON(tmp != hwlock);
> -
> +out:
>  	return ret;
>  }
> 
> @@ -742,9 +783,9 @@ struct hwspinlock
> *hwspin_lock_request_specific(unsigned int id)
>  	/* sanity check (this shouldn't happen) */
>  	WARN_ON(hwlock_to_id(hwlock) != id);
> 
> -	/* make sure this hwspinlock is unused */
> -	ret = radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
> -	if (ret == 0) {
> +	/* make sure this hwspinlock is unused or shareable */
> +	if (!radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_SHARED) &&
> +	    !radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED)) {
>  		pr_warn("hwspinlock %u is already in use\n", id);
>  		hwlock = NULL;
>  		goto out;
> @@ -777,7 +818,7 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
>  {
>  	struct device *dev;
>  	struct hwspinlock *tmp;
> -	int ret;
> +	int ret, id;
> 
>  	if (!hwlock) {
>  		pr_err("invalid hwlock\n");
> @@ -788,30 +829,35 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
>  	mutex_lock(&hwspinlock_tree_lock);
> 
>  	/* make sure the hwspinlock is used */
> -	ret = radix_tree_tag_get(&hwspinlock_tree, hwlock_to_id(hwlock),
> -
> 	HWSPINLOCK_UNUSED);
> +	id = hwlock_to_id(hwlock);
> +	ret = radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
>  	if (ret == 1) {
>  		dev_err(dev, "%s: hwlock is already free\n", __func__);
>  		dump_stack();
>  		ret = -EINVAL;
> -		goto out;
> +		goto unlock;
>  	}
> 
>  	/* notify the underlying device that power is not needed */
>  	ret = pm_runtime_put(dev);
>  	if (ret < 0)
> -		goto out;
> +		goto unlock;
> +
> +	/* update shareable refcount */
> +	if (radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_SHARED)
> &&
> +	    --hwlock->refcount)
> +		goto put;
> 
>  	/* mark this hwspinlock as available */
> -	tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock),
> -
> 	HWSPINLOCK_UNUSED);
> +	tmp = radix_tree_tag_set(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
> 
>  	/* sanity check (this shouldn't happen) */
>  	WARN_ON(tmp != hwlock);
> 
> +put:
>  	module_put(dev->driver->owner);
> 
> -out:
> +unlock:
>  	mutex_unlock(&hwspinlock_tree_lock);
>  	return ret;
>  }
> diff --git a/drivers/hwspinlock/hwspinlock_internal.h
> b/drivers/hwspinlock/hwspinlock_internal.h
> index 9eb6bd0..c808e11 100644
> --- a/drivers/hwspinlock/hwspinlock_internal.h
> +++ b/drivers/hwspinlock/hwspinlock_internal.h
> @@ -35,11 +35,13 @@ struct hwspinlock_ops {
>   * struct hwspinlock - this struct represents a single hwspinlock instance
>   * @bank: the hwspinlock_device structure which owns this lock
>   * @lock: initialized and used by hwspinlock core
> + * @refcount: number of users (when shared)
>   * @priv: private data, owned by the underlying platform-specific hwspinlock
> drv
>   */
>  struct hwspinlock {
>  	struct hwspinlock_device *bank;
>  	spinlock_t lock;
> +	unsigned int refcount;
>  	void *priv;
>  };
> 
> --
> 2.7.4