[PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Fabien Dessenne]

The current implementation does not allow two different devices to use
a common hwspinlock. This patch set proposes to have, as an option, some
hwspinlocks shared between several users.

Below is an example that explain the need for this:
	exti: interrupt-controller@5000d000 {
		compatible = "st,stm32mp1-exti", "syscon";
		interrupt-controller;
		#interrupt-cells = <2>;
		reg = <0x5000d000 0x400>;
		hwlocks = <&hsem 1>;
	};
The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
With the current hwspinlock implementation, only the first driver succeeds
in requesting (hwspin_lock_request_specific) the hwlock. The second request
fails.


The proposed approach does not modify the API, but extends the DT 'hwlocks'
property with a second optional parameter (the first one identifies an
hwlock) that specifies whether an hwlock is requested for exclusive usage
(current behavior) or can be shared between several users.
Examples:
	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage

As a constraint, the #hwlock-cells value must be 1 or 2.
In the current implementation, this can have theorically any value but:
- all of the exisiting drivers use the same value : 1.
- the framework supports only one value : 1 (see implementation of
  of_hwspin_lock_simple_xlate())
Hence, it shall not be a problem to restrict this value to 1 or 2 since
it won't break any driver.

Fabien Dessenne (6):
  dt-bindings: hwlock: add support of shared locks
  hwspinlock: allow sharing of hwspinlocks
  dt-bindings: hwlock: update STM32 #hwlock-cells value
  ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
  ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
  ARM: dts: stm32: hwlocks for GPIO for stm32mp157

 .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
 .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
 Documentation/hwspinlock.txt                       | 10 ++-
 arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
 arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
 drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-----
 drivers/hwspinlock/hwspinlock_internal.h           |  2 +
 7 files changed, 108 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH 1/6] dt-bindings: hwlock: add support of shared locks

[Fabien Dessenne]

Use #hwlock-cells value to define whether the locks can be shared
by several users.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 .../devicetree/bindings/hwlock/hwlock.txt          | 27 ++++++++++++++++------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwlock/hwlock.txt b/Documentation/devicetree/bindings/hwlock/hwlock.txt
index 085d1f5..e98088a 100644
--- a/Documentation/devicetree/bindings/hwlock/hwlock.txt
+++ b/Documentation/devicetree/bindings/hwlock/hwlock.txt
@@ -13,7 +13,7 @@ hwlock providers:
 
 Required properties:
 - #hwlock-cells:        Specifies the number of cells needed to represent a
-                        specific lock.
+                        specific lock. Shall be 1 or 2 (see hwlocks below).
 
 hwlock users:
 =============
@@ -27,6 +27,11 @@ Required properties:
                         #hwlock-cells. The list can have just a single hwlock
                         or multiple hwlocks, with each hwlock represented by
                         a phandle and a corresponding args specifier.
+                        If #hwlock-cells is 1, all of the locks are exclusive
+                        (cannot be used by several users).
+                        If #hwlock-cells is 2, the value of the second cell
+                        defines whether the lock is for exclusive usage (0) or
+                        shared (1) i.e. can be used by several users.
 
 Optional properties:
 - hwlock-names:         List of hwlock name strings defined in the same order
@@ -46,14 +51,22 @@ of length 1.
 		...
 	};
 
-2. Example of a node using multiple specific hwlocks:
+2. Example of nodes using multiple and shared specific hwlocks:
 
-The following example has a node requesting two hwlocks, a hwlock within
-the hwlock device node 'hwlock1' with #hwlock-cells value of 1, and another
-hwlock within the hwlock device node 'hwlock2' with #hwlock-cells value of 2.
+The following example has a nodeA requesting two hwlocks:
+- an exclusive one (#hwlock-cells = 1) within the hwlock device node 'hwlock1'
+- a shared one (#hwlock-cells = 2, second cell = 1) within the hwlock device
+  node 'hwlock2'.
+The shared lock is also be used by nodeB.
 
-	node {
+	nodeA {
 		...
-		hwlocks = <&hwlock1 2>, <&hwlock2 0 3>;
+		hwlocks = <&hwlock1 2>, <&hwlock2 0 1>;
 		...
 	};
+
+	nodeB {
+		...
+		hwlocks = <&hwlock2 0 1>;
+		...
+	};
\ No newline at end of file
-- 
2.7.4

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

[Fabien Dessenne]

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>
---
 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

[PATCH 3/6] dt-bindings: hwlock: update STM32 #hwlock-cells value

[Fabien Dessenne]

Use a value of 2, so users can share hwlocks.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 Documentation/devicetree/bindings/hwlock/st,stm32-hwspinlock.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwlock/st,stm32-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/st,stm32-hwspinlock.txt
index adf4f000..60a3716 100644
--- a/Documentation/devicetree/bindings/hwlock/st,stm32-hwspinlock.txt
+++ b/Documentation/devicetree/bindings/hwlock/st,stm32-hwspinlock.txt
@@ -4,8 +4,8 @@ STM32 Hardware Spinlock Device Binding
 Required properties :
 - compatible : should be "st,stm32-hwspinlock".
 - reg : the register address of hwspinlock.
-- #hwlock-cells : hwlock users only use the hwlock id to represent a specific
-	hwlock, so the number of cells should be <1> here.
+- #hwlock-cells : should be <2> so the hwlock users use the hwlock id to
+	represent a specific hwlock and define its shared / exclusive attribute.
 - clock-names : Must contain "hsem".
 - clocks : Must contain a phandle entry for the clock in clock-names, see the
 	common clock bindings.
@@ -16,7 +16,7 @@ Please look at the generic hwlock binding for usage information for consumers,
 Example of hwlock provider:
 	hwspinlock@4c000000 {
 		compatible = "st,stm32-hwspinlock";
-		#hwlock-cells = <1>;
+		#hwlock-cells = <2>;
 		reg = <0x4c000000 0x400>;
 		clocks = <&rcc HSEM>;
 		clock-names = "hsem";
-- 
2.7.4

[PATCH 4/6] ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC

[Fabien Dessenne]

Declare hwspinlock device for stm32mp157 SoC

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 arch/arm/boot/dts/stm32mp157c.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
index 105e21f..18baaea 100644
--- a/arch/arm/boot/dts/stm32mp157c.dtsi
+++ b/arch/arm/boot/dts/stm32mp157c.dtsi
@@ -886,6 +886,15 @@
 			status = "disabled";
 		};
 
+		hsem: hwspinlock@4c000000 {
+			compatible = "st,stm32-hwspinlock";
+			#hwlock-cells = <2>;
+			reg = <0x4c000000 0x400>;
+			clocks = <&rcc HSEM>;
+			clock-names = "hsem";
+			status = "okay";
+		};
+
 		ipcc: mailbox@4c001000 {
 			compatible = "st,stm32mp1-ipcc";
 			#mbox-cells = <1>;
-- 
2.7.4

[PATCH 5/6] ARM: dts: stm32: Add hwlock for irqchip on stm32mp157

[Fabien Dessenne]

Define a hwspinlock to be used by the irq controller driver and the
syscon driver.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
index 18baaea..0795124 100644
--- a/arch/arm/boot/dts/stm32mp157c.dtsi
+++ b/arch/arm/boot/dts/stm32mp157c.dtsi
@@ -922,6 +922,7 @@
 			interrupt-controller;
 			#interrupt-cells = <2>;
 			reg = <0x5000d000 0x400>;
+			hwlocks = <&hsem 1 1>;
 		};
 
 		syscfg: syscon@50020000 {
-- 
2.7.4

[PATCH 6/6] ARM: dts: stm32: hwlocks for GPIO for stm32mp157

[Fabien Dessenne]

Declare a shared hwlock to be used by all gpio / pin controllers.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 arch/arm/boot/dts/stm32mp157-pinctrl.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi
index 6b3a9c6..9fd562a 100644
--- a/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp157-pinctrl.dtsi
@@ -14,6 +14,7 @@
 			ranges = <0 0x50002000 0xa400>;
 			interrupt-parent = <&exti>;
 			st,syscfg = <&exti 0x60 0xff>;
+			hwlocks = <&hsem 0 1>;
 			pins-are-numbered;
 
 			gpioa: gpio@50002000 {
@@ -424,6 +425,7 @@
 			pins-are-numbered;
 			interrupt-parent = <&exti>;
 			st,syscfg = <&exti 0x60 0xff>;
+			hwlocks = <&hsem 0 1>;
 
 			gpioz: gpio@54004000 {
 				gpio-controller;
-- 
2.7.4

Re: [PATCH 1/6] dt-bindings: hwlock: add support of shared locks

[Rob Herring]

On Wed, 13 Mar 2019 16:50:32 +0100, Fabien Dessenne wrote:
> Use #hwlock-cells value to define whether the locks can be shared
> by several users.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  .../devicetree/bindings/hwlock/hwlock.txt          | 27 ++++++++++++++++------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 4/6] ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC

[Rob Herring]

On Wed, Mar 13, 2019 at 04:50:35PM +0100, Fabien Dessenne wrote:
> Declare hwspinlock device for stm32mp157 SoC
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  arch/arm/boot/dts/stm32mp157c.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
> index 105e21f..18baaea 100644
> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
> @@ -886,6 +886,15 @@
>  			status = "disabled";
>  		};
>  
> +		hsem: hwspinlock@4c000000 {
> +			compatible = "st,stm32-hwspinlock";
> +			#hwlock-cells = <2>;
> +			reg = <0x4c000000 0x400>;
> +			clocks = <&rcc HSEM>;
> +			clock-names = "hsem";
> +			status = "okay";

okay is the default, so you can remove it.

> +		};
> +
>  		ipcc: mailbox@4c001000 {
>  			compatible = "st,stm32mp1-ipcc";
>  			#mbox-cells = <1>;
> -- 
> 2.7.4
> 

Re: [PATCH 3/6] dt-bindings: hwlock: update STM32 #hwlock-cells value

[Rob Herring]

On Wed, 13 Mar 2019 16:50:34 +0100, Fabien Dessenne wrote:
> Use a value of 2, so users can share hwlocks.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  Documentation/devicetree/bindings/hwlock/st,stm32-hwspinlock.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Loic PALLARDY]

> -----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

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Bjorn Andersson]

On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:

> The current implementation does not allow two different devices to use
> a common hwspinlock. This patch set proposes to have, as an option, some
> hwspinlocks shared between several users.
> 
> Below is an example that explain the need for this:
> 	exti: interrupt-controller@5000d000 {
> 		compatible = "st,stm32mp1-exti", "syscon";
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
> 		reg = <0x5000d000 0x400>;
> 		hwlocks = <&hsem 1>;
> 	};
> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> With the current hwspinlock implementation, only the first driver succeeds
> in requesting (hwspin_lock_request_specific) the hwlock. The second request
> fails.
> 
> 
> The proposed approach does not modify the API, but extends the DT 'hwlocks'
> property with a second optional parameter (the first one identifies an
> hwlock) that specifies whether an hwlock is requested for exclusive usage
> (current behavior) or can be shared between several users.
> Examples:
> 	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
> 	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
> 	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage
> 
> As a constraint, the #hwlock-cells value must be 1 or 2.
> In the current implementation, this can have theorically any value but:
> - all of the exisiting drivers use the same value : 1.
> - the framework supports only one value : 1 (see implementation of
>   of_hwspin_lock_simple_xlate())
> Hence, it shall not be a problem to restrict this value to 1 or 2 since
> it won't break any driver.
> 

Hi Fabien,

Your series looks good, but it makes me wonder why the hardware locks
should be an exclusive resource.

How about just making all (specific) locks shared?

Regards,
Bjorn

> Fabien Dessenne (6):
>   dt-bindings: hwlock: add support of shared locks
>   hwspinlock: allow sharing of hwspinlocks
>   dt-bindings: hwlock: update STM32 #hwlock-cells value
>   ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
>   ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
>   ARM: dts: stm32: hwlocks for GPIO for stm32mp157
> 
>  .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
>  .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
>  Documentation/hwspinlock.txt                       | 10 ++-
>  arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
>  arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
>  drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-----
>  drivers/hwspinlock/hwspinlock_internal.h           |  2 +
>  7 files changed, 108 insertions(+), 31 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Fabien DESSENNE]

On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>
>> The current implementation does not allow two different devices to use
>> a common hwspinlock. This patch set proposes to have, as an option, some
>> hwspinlocks shared between several users.
>>
>> Below is an example that explain the need for this:
>> 	exti: interrupt-controller@5000d000 {
>> 		compatible = "st,stm32mp1-exti", "syscon";
>> 		interrupt-controller;
>> 		#interrupt-cells = <2>;
>> 		reg = <0x5000d000 0x400>;
>> 		hwlocks = <&hsem 1>;
>> 	};
>> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
>> With the current hwspinlock implementation, only the first driver succeeds
>> in requesting (hwspin_lock_request_specific) the hwlock. The second request
>> fails.
>>
>>
>> The proposed approach does not modify the API, but extends the DT 'hwlocks'
>> property with a second optional parameter (the first one identifies an
>> hwlock) that specifies whether an hwlock is requested for exclusive usage
>> (current behavior) or can be shared between several users.
>> Examples:
>> 	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
>> 	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
>> 	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage
>>
>> As a constraint, the #hwlock-cells value must be 1 or 2.
>> In the current implementation, this can have theorically any value but:
>> - all of the exisiting drivers use the same value : 1.
>> - the framework supports only one value : 1 (see implementation of
>>    of_hwspin_lock_simple_xlate())
>> Hence, it shall not be a problem to restrict this value to 1 or 2 since
>> it won't break any driver.
>>
> Hi Fabien,
>
> Your series looks good, but it makes me wonder why the hardware locks
> should be an exclusive resource.
>
> How about just making all (specific) locks shared?

Hi Bjorn,

Making all locks shared is a possible implementation (my first 
implementation
was going this way) but there are some drawbacks we must be aware of:

A/ This theoretically break the legacy behavior (the legacy works with
exclusive (UNUSED radix tag) usage). As a consequence, an existing driver
that is currently failing to request a lock (already claimed by another
user) would now work fine. Not sure that there are such drivers, so this
point is probably not a real issue.

B/ This would introduce some inconsistency between the two 'request' API
which are hwspin_lock_request() and hwspin_lock_request_specific().
hwspin_lock_request() looks for an unused lock, so requests for an exclusive
usage. On the other side, request_specific() would request shared locks.
Worst the following sequence can transform an exclusive usage into a shared

one:
   -hwspin_lock_request() -> returns Id#0 (exclusive)
   -hwspin_lock_request() -> returns Id#1 (exclusive)
   -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
Honestly I am not sure that this is a real issue, but it's better to have it
in mind before we take ay decision
I could not find any driver using the hwspin_lock_request() API, we may 
decide
to remove (or to make deprecated) this API, having everything 'shared 
without
any conditions'.


I can see three options:
1- Keep my initial proposition
2- Have hwspin_lock_request_specific() using shared locks and
    hwspin_lock_request() using unused (so 'initially' exclusive) locks.
3- Have hwspin_lock_request_specific() using shared locks and
    remove/make deprecated hwspin_lock_request().

Just let me know what is your preference.

BR

Fabien

>
> Regards,
> Bjorn
>
>> Fabien Dessenne (6):
>>    dt-bindings: hwlock: add support of shared locks
>>    hwspinlock: allow sharing of hwspinlocks
>>    dt-bindings: hwlock: update STM32 #hwlock-cells value
>>    ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
>>    ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
>>    ARM: dts: stm32: hwlocks for GPIO for stm32mp157
>>
>>   .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
>>   .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
>>   Documentation/hwspinlock.txt                       | 10 ++-
>>   arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
>>   arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
>>   drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-----
>>   drivers/hwspinlock/hwspinlock_internal.h           |  2 +
>>   7 files changed, 108 insertions(+), 31 deletions(-)
>>
>> -- 
>> 2.7.4
>>

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Bjorn Andersson]

On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:

> 
> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
> > On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
> >
> >> The current implementation does not allow two different devices to use
> >> a common hwspinlock. This patch set proposes to have, as an option, some
> >> hwspinlocks shared between several users.
> >>
> >> Below is an example that explain the need for this:
> >> 	exti: interrupt-controller@5000d000 {
> >> 		compatible = "st,stm32mp1-exti", "syscon";
> >> 		interrupt-controller;
> >> 		#interrupt-cells = <2>;
> >> 		reg = <0x5000d000 0x400>;
> >> 		hwlocks = <&hsem 1>;
> >> 	};
> >> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> >> With the current hwspinlock implementation, only the first driver succeeds
> >> in requesting (hwspin_lock_request_specific) the hwlock. The second request
> >> fails.
> >>
> >>
> >> The proposed approach does not modify the API, but extends the DT 'hwlocks'
> >> property with a second optional parameter (the first one identifies an
> >> hwlock) that specifies whether an hwlock is requested for exclusive usage
> >> (current behavior) or can be shared between several users.
> >> Examples:
> >> 	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
> >> 	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
> >> 	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage
> >>
> >> As a constraint, the #hwlock-cells value must be 1 or 2.
> >> In the current implementation, this can have theorically any value but:
> >> - all of the exisiting drivers use the same value : 1.
> >> - the framework supports only one value : 1 (see implementation of
> >>    of_hwspin_lock_simple_xlate())
> >> Hence, it shall not be a problem to restrict this value to 1 or 2 since
> >> it won't break any driver.
> >>
> > Hi Fabien,
> >
> > Your series looks good, but it makes me wonder why the hardware locks
> > should be an exclusive resource.
> >
> > How about just making all (specific) locks shared?
> 
> Hi Bjorn,
> 
> Making all locks shared is a possible implementation (my first 
> implementation
> was going this way) but there are some drawbacks we must be aware of:
> 
> A/ This theoretically break the legacy behavior (the legacy works with
> exclusive (UNUSED radix tag) usage). As a consequence, an existing driver
> that is currently failing to request a lock (already claimed by another
> user) would now work fine. Not sure that there are such drivers, so this
> point is probably not a real issue.
> 

Right, it's possible that a previously misconfigured system now
successfully probes more than one device that uses a particular
spinlock. But such system would be suffering from issues related to e.g.
probe ordering.

So I think we should ignore this issue.

> B/ This would introduce some inconsistency between the two 'request' API
> which are hwspin_lock_request() and hwspin_lock_request_specific().
> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
> usage. On the other side, request_specific() would request shared locks.
> Worst the following sequence can transform an exclusive usage into a shared
> 

There is already an inconsistency in between these; as with above any
system that uses both request() and request_specific() will be suffering
from intermittent failures due to probe ordering.

> one:
>    -hwspin_lock_request() -> returns Id#0 (exclusive)
>    -hwspin_lock_request() -> returns Id#1 (exclusive)
>    -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
> Honestly I am not sure that this is a real issue, but it's better to have it
> in mind before we take ay decision

The case where I can see a
problem with this would be if the two clients somehow would nest their
locking regions.

But generally I think this could consider this an improvement, because
the request_specific() would now be able to acquire its hwlock, with
some additional contention due to the multiple use.

> I could not find any driver using the hwspin_lock_request() API, we
> may decide to remove (or to make deprecated) this API, having
> everything 'shared without any conditions'.
> 

It would be nice to have an upstream user of this API.

> 
> I can see three options:
> 1- Keep my initial proposition
> 2- Have hwspin_lock_request_specific() using shared locks and
>     hwspin_lock_request() using unused (so 'initially' exclusive) locks.
> 3- Have hwspin_lock_request_specific() using shared locks and
>     remove/make deprecated hwspin_lock_request().
> 
> Just let me know what is your preference.
> 

I think we should start with #2 and would like input from e.g. Suman
regarding #3.

Regards,
Bjorn

> BR
> 
> Fabien
> 
> >
> > Regards,
> > Bjorn
> >
> >> Fabien Dessenne (6):
> >>    dt-bindings: hwlock: add support of shared locks
> >>    hwspinlock: allow sharing of hwspinlocks
> >>    dt-bindings: hwlock: update STM32 #hwlock-cells value
> >>    ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
> >>    ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
> >>    ARM: dts: stm32: hwlocks for GPIO for stm32mp157
> >>
> >>   .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
> >>   .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
> >>   Documentation/hwspinlock.txt                       | 10 ++-
> >>   arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
> >>   arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
> >>   drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-----
> >>   drivers/hwspinlock/hwspinlock_internal.h           |  2 +
> >>   7 files changed, 108 insertions(+), 31 deletions(-)
> >>
> >> -- 
> >> 2.7.4
> >>

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

[Fabien DESSENNE]

Hi Suman,

Could you please let us know your thoughts or comments?

BR

Fabien


> -----Original Message-----
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> Sent: lundi 5 août 2019 19:47
> To: Fabien DESSENNE <fabien.dessenne@st.com>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>; 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; Benjamin GAIGNARD
> <benjamin.gaignard@st.com>
> Subject: Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks
> 
> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
> 
> >
> > On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
> > > On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
> > >
> > >> The current implementation does not allow two different devices to
> > >> use a common hwspinlock. This patch set proposes to have, as an
> > >> option, some hwspinlocks shared between several users.
> > >>
> > >> Below is an example that explain the need for this:
> > >> 	exti: interrupt-controller@5000d000 {
> > >> 		compatible = "st,stm32mp1-exti", "syscon";
> > >> 		interrupt-controller;
> > >> 		#interrupt-cells = <2>;
> > >> 		reg = <0x5000d000 0x400>;
> > >> 		hwlocks = <&hsem 1>;
> > >> 	};
> > >> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> > >> With the current hwspinlock implementation, only the first driver
> > >> succeeds in requesting (hwspin_lock_request_specific) the hwlock.
> > >> The second request fails.
> > >>
> > >>
> > >> The proposed approach does not modify the API, but extends the DT
> 'hwlocks'
> > >> property with a second optional parameter (the first one identifies
> > >> an
> > >> hwlock) that specifies whether an hwlock is requested for exclusive
> > >> usage (current behavior) or can be shared between several users.
> > >> Examples:
> > >> 	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
> > >> 	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
> > >> 	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage
> > >>
> > >> As a constraint, the #hwlock-cells value must be 1 or 2.
> > >> In the current implementation, this can have theorically any value but:
> > >> - all of the exisiting drivers use the same value : 1.
> > >> - the framework supports only one value : 1 (see implementation of
> > >>    of_hwspin_lock_simple_xlate())
> > >> Hence, it shall not be a problem to restrict this value to 1 or 2
> > >> since it won't break any driver.
> > >>
> > > Hi Fabien,
> > >
> > > Your series looks good, but it makes me wonder why the hardware
> > > locks should be an exclusive resource.
> > >
> > > How about just making all (specific) locks shared?
> >
> > Hi Bjorn,
> >
> > Making all locks shared is a possible implementation (my first
> > implementation was going this way) but there are some drawbacks we
> > must be aware of:
> >
> > A/ This theoretically break the legacy behavior (the legacy works with
> > exclusive (UNUSED radix tag) usage). As a consequence, an existing
> > driver that is currently failing to request a lock (already claimed by
> > another
> > user) would now work fine. Not sure that there are such drivers, so
> > this point is probably not a real issue.
> >
> 
> Right, it's possible that a previously misconfigured system now successfully
> probes more than one device that uses a particular spinlock. But such system
> would be suffering from issues related to e.g.
> probe ordering.
> 
> So I think we should ignore this issue.
> 
> > B/ This would introduce some inconsistency between the two 'request'
> > API which are hwspin_lock_request() and hwspin_lock_request_specific().
> > hwspin_lock_request() looks for an unused lock, so requests for an
> > exclusive usage. On the other side, request_specific() would request shared
> locks.
> > Worst the following sequence can transform an exclusive usage into a
> > shared
> >
> 
> There is already an inconsistency in between these; as with above any system
> that uses both request() and request_specific() will be suffering from intermittent
> failures due to probe ordering.
> 
> > one:
> >    -hwspin_lock_request() -> returns Id#0 (exclusive)
> >    -hwspin_lock_request() -> returns Id#1 (exclusive)
> >    -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0
> > shared Honestly I am not sure that this is a real issue, but it's
> > better to have it in mind before we take ay decision
> 
> The case where I can see a
> problem with this would be if the two clients somehow would nest their locking
> regions.
> 
> But generally I think this could consider this an improvement, because the
> request_specific() would now be able to acquire its hwlock, with some additional
> contention due to the multiple use.
> 
> > I could not find any driver using the hwspin_lock_request() API, we
> > may decide to remove (or to make deprecated) this API, having
> > everything 'shared without any conditions'.
> >
> 
> It would be nice to have an upstream user of this API.
> 
> >
> > I can see three options:
> > 1- Keep my initial proposition
> > 2- Have hwspin_lock_request_specific() using shared locks and
> >     hwspin_lock_request() using unused (so 'initially' exclusive) locks.
> > 3- Have hwspin_lock_request_specific() using shared locks and
> >     remove/make deprecated hwspin_lock_request().
> >
> > Just let me know what is your preference.
> >
> 
> I think we should start with #2 and would like input from e.g. Suman regarding #3.
> 
> Regards,
> Bjorn
> 
> > BR
> >
> > Fabien
> >
> > >
> > > Regards,
> > > Bjorn
> > >
> > >> Fabien Dessenne (6):
> > >>    dt-bindings: hwlock: add support of shared locks
> > >>    hwspinlock: allow sharing of hwspinlocks
> > >>    dt-bindings: hwlock: update STM32 #hwlock-cells value
> > >>    ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
> > >>    ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
> > >>    ARM: dts: stm32: hwlocks for GPIO for stm32mp157
> > >>
> > >>   .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
> > >>   .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
> > >>   Documentation/hwspinlock.txt                       | 10 ++-
> > >>   arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
> > >>   arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
> > >>   drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-
> ----
> > >>   drivers/hwspinlock/hwspinlock_internal.h           |  2 +
> > >>   7 files changed, 108 insertions(+), 31 deletions(-)
> > >>
> > >> --
> > >> 2.7.4
> > >>

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Suman Anna]

Hi Fabien,

On 8/5/19 12:46 PM, Bjorn Andersson wrote:
> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
> 
>>
>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>>>
>>>> The current implementation does not allow two different devices to use
>>>> a common hwspinlock. This patch set proposes to have, as an option, some
>>>> hwspinlocks shared between several users.
>>>>
>>>> Below is an example that explain the need for this:
>>>> 	exti: interrupt-controller@5000d000 {
>>>> 		compatible = "st,stm32mp1-exti", "syscon";
>>>> 		interrupt-controller;
>>>> 		#interrupt-cells = <2>;
>>>> 		reg = <0x5000d000 0x400>;
>>>> 		hwlocks = <&hsem 1>;
>>>> 	};
>>>> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
>>>> With the current hwspinlock implementation, only the first driver succeeds
>>>> in requesting (hwspin_lock_request_specific) the hwlock. The second request
>>>> fails.

Help me understand the problem that you are trying to solve here. Is
this a case of you having two clients on Linux-side needing to use the
same lock but still requiring the arbitration with software running on
some other remote processor? Are they talking to the same entity on the
remote-side or different peers.

I see the series is all about getting a handle so that they can use the
API, and is the expected usage that the same entity will lock and unlock
before the other driver can lock it.

>>>>
>>>>
>>>> The proposed approach does not modify the API, but extends the DT 'hwlocks'
>>>> property with a second optional parameter (the first one identifies an
>>>> hwlock) that specifies whether an hwlock is requested for exclusive usage
>>>> (current behavior) or can be shared between several users.
>>>> Examples:
>>>> 	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
>>>> 	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
>>>> 	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage
>>>>
>>>> As a constraint, the #hwlock-cells value must be 1 or 2.
>>>> In the current implementation, this can have theorically any value but:
>>>> - all of the exisiting drivers use the same value : 1.
>>>> - the framework supports only one value : 1 (see implementation of
>>>>    of_hwspin_lock_simple_xlate())
>>>> Hence, it shall not be a problem to restrict this value to 1 or 2 since
>>>> it won't break any driver.
>>>>
>>> Hi Fabien,
>>>
>>> Your series looks good, but it makes me wonder why the hardware locks
>>> should be an exclusive resource.
>>>
>>> How about just making all (specific) locks shared?
>>
>> Hi Bjorn,
>>
>> Making all locks shared is a possible implementation (my first 
>> implementation
>> was going this way) but there are some drawbacks we must be aware of:
>>
>> A/ This theoretically break the legacy behavior (the legacy works with
>> exclusive (UNUSED radix tag) usage). As a consequence, an existing driver
>> that is currently failing to request a lock (already claimed by another
>> user) would now work fine. Not sure that there are such drivers, so this
>> point is probably not a real issue.
>>
> 
> Right, it's possible that a previously misconfigured system now
> successfully probes more than one device that uses a particular
> spinlock. But such system would be suffering from issues related to e.g.
> probe ordering.
> 
> So I think we should ignore this issue.
> 
>> B/ This would introduce some inconsistency between the two 'request' API
>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>> usage. On the other side, request_specific() would request shared locks.
>> Worst the following sequence can transform an exclusive usage into a shared
>>
> 
> There is already an inconsistency in between these; as with above any
> system that uses both request() and request_specific() will be suffering
> from intermittent failures due to probe ordering.
> 
>> one:
>>    -hwspin_lock_request() -> returns Id#0 (exclusive)
>>    -hwspin_lock_request() -> returns Id#1 (exclusive)
>>    -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>> Honestly I am not sure that this is a real issue, but it's better to have it
>> in mind before we take ay decision

Wouldn't it be actually simpler to just introduce a new specific API
variant for this, similar to the reset core for example (it uses a
separate exclusive API), without having to modify the bindings at all.
It is just a case of your driver using the right API, and the core can
be modified to use the additional tag semantics based on the API. It
should avoid any confusion with say using a different second cell value
for the same lock in two different nodes.

If you are sharing a hwlock on the Linux side, surely your driver should
be aware that it is a shared lock. The tag can be set during the first
request API, and you look through both tags when giving out a handle.

Obviously, the hwspin_lock_request() API usage semantics always had the
implied additional need for communicating the lock id to the other peer
entity, so a realistic usage is most always the specific API variant. I
doubt this API would be of much use for the shared driver usage. This
also implies that the client user does not care about specifying a lock
in DT.

regards
Suman

> 
> The case where I can see a
> problem with this would be if the two clients somehow would nest their
> locking regions.
> 
> But generally I think this could consider this an improvement, because
> the request_specific() would now be able to acquire its hwlock, with
> some additional contention due to the multiple use.
> 
>> I could not find any driver using the hwspin_lock_request() API, we
>> may decide to remove (or to make deprecated) this API, having
>> everything 'shared without any conditions'.
>>
> 
> It would be nice to have an upstream user of this API.
> 
>>
>> I can see three options:
>> 1- Keep my initial proposition
>> 2- Have hwspin_lock_request_specific() using shared locks and
>>     hwspin_lock_request() using unused (so 'initially' exclusive) locks.
>> 3- Have hwspin_lock_request_specific() using shared locks and
>>     remove/make deprecated hwspin_lock_request().
>>
>> Just let me know what is your preference.
>>
> 
> I think we should start with #2 and would like input from e.g. Suman
> regarding #3.
> 
> Regards,
> Bjorn
> 
>> BR
>>
>> Fabien
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> Fabien Dessenne (6):
>>>>    dt-bindings: hwlock: add support of shared locks
>>>>    hwspinlock: allow sharing of hwspinlocks
>>>>    dt-bindings: hwlock: update STM32 #hwlock-cells value
>>>>    ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
>>>>    ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
>>>>    ARM: dts: stm32: hwlocks for GPIO for stm32mp157
>>>>
>>>>   .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
>>>>   .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
>>>>   Documentation/hwspinlock.txt                       | 10 ++-
>>>>   arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
>>>>   arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
>>>>   drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-----
>>>>   drivers/hwspinlock/hwspinlock_internal.h           |  2 +
>>>>   7 files changed, 108 insertions(+), 31 deletions(-)
>>>>
>>>> -- 
>>>> 2.7.4
>>>>

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Bjorn Andersson]

On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:

> Hi Fabien,
> 
> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
> > On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
> > 
> >>
> >> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
> >>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
[..]
> >> B/ This would introduce some inconsistency between the two 'request' API
> >> which are hwspin_lock_request() and hwspin_lock_request_specific().
> >> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
> >> usage. On the other side, request_specific() would request shared locks.
> >> Worst the following sequence can transform an exclusive usage into a shared
> >>
> > 
> > There is already an inconsistency in between these; as with above any
> > system that uses both request() and request_specific() will be suffering
> > from intermittent failures due to probe ordering.
> > 
> >> one:
> >>    -hwspin_lock_request() -> returns Id#0 (exclusive)
> >>    -hwspin_lock_request() -> returns Id#1 (exclusive)
> >>    -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
> >> Honestly I am not sure that this is a real issue, but it's better to have it
> >> in mind before we take ay decision
> 
> Wouldn't it be actually simpler to just introduce a new specific API
> variant for this, similar to the reset core for example (it uses a
> separate exclusive API), without having to modify the bindings at all.
> It is just a case of your driver using the right API, and the core can
> be modified to use the additional tag semantics based on the API. It
> should avoid any confusion with say using a different second cell value
> for the same lock in two different nodes.
> 

But this implies that there is an actual need to hold these locks
exclusively. Given that they are (except for the raw case) all wrapped
by Linux locking primitives there shouldn't be a problem sharing a lock
(except possibly for the raw case).


I agree that we shouldn't specify this property in DT - if anything it
should be a variant of the API.

> If you are sharing a hwlock on the Linux side, surely your driver should
> be aware that it is a shared lock. The tag can be set during the first
> request API, and you look through both tags when giving out a handle.
> 

Why would the driver need to know about it?

> Obviously, the hwspin_lock_request() API usage semantics always had the
> implied additional need for communicating the lock id to the other peer
> entity, so a realistic usage is most always the specific API variant. I
> doubt this API would be of much use for the shared driver usage. This
> also implies that the client user does not care about specifying a lock
> in DT.
> 

Afaict if the lock are shared then there shouldn't be a problem with
some clients using the request API and others request_specific(). As any
collisions would simply mean that there are more contention on the lock.

With the current exclusive model that is not possible and the success of
the request_specific will depend on probe order.

But perhaps it should be explicitly prohibited to use both APIs on the
same hwspinlock instance?

Regards,
Bjorn

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Suman Anna]

On 8/6/19 1:21 PM, Bjorn Andersson wrote:
> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
> 
>> Hi Fabien,
>>
>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>>>
>>>>
>>>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
> [..]
>>>> B/ This would introduce some inconsistency between the two 'request' API
>>>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>>>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>>>> usage. On the other side, request_specific() would request shared locks.
>>>> Worst the following sequence can transform an exclusive usage into a shared
>>>>
>>>
>>> There is already an inconsistency in between these; as with above any
>>> system that uses both request() and request_specific() will be suffering
>>> from intermittent failures due to probe ordering.
>>>
>>>> one:
>>>>    -hwspin_lock_request() -> returns Id#0 (exclusive)
>>>>    -hwspin_lock_request() -> returns Id#1 (exclusive)
>>>>    -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>>>> Honestly I am not sure that this is a real issue, but it's better to have it
>>>> in mind before we take ay decision
>>
>> Wouldn't it be actually simpler to just introduce a new specific API
>> variant for this, similar to the reset core for example (it uses a
>> separate exclusive API), without having to modify the bindings at all.
>> It is just a case of your driver using the right API, and the core can
>> be modified to use the additional tag semantics based on the API. It
>> should avoid any confusion with say using a different second cell value
>> for the same lock in two different nodes.
>>
> 
> But this implies that there is an actual need to hold these locks
> exclusively. Given that they are (except for the raw case) all wrapped
> by Linux locking primitives there shouldn't be a problem sharing a lock
> (except possibly for the raw case).

Yes agreed, the HWLOCK_RAW and HWLOCK_IN_ATOMIC cases are unprotected. I
am still trying to understand better the usecase to see if the same lock
is being multiplexed for different protection contexts, or if all of
them are protecting the same context.

> 
> I agree that we shouldn't specify this property in DT - if anything it
> should be a variant of the API.
> 
>> If you are sharing a hwlock on the Linux side, surely your driver should
>> be aware that it is a shared lock. The tag can be set during the first
>> request API, and you look through both tags when giving out a handle.
>>
> 
> Why would the driver need to know about it?

Just the semantics if we were to support single user vs multiple users
on Linux-side to even get a handle. Your point is that this may be moot
since we have protection anyway other than the raw cases. But we need to
be able to have the same API work across all cases.

So far, it had mostly been that there would be one user on Linux
competing with other equivalent peer entities on different processors.
It is not common to have multiple users since these protection schemes
are usually needed only at the lowest levels of a stack, so the
exclusive handle stuff had been sufficient.

> 
>> Obviously, the hwspin_lock_request() API usage semantics always had the
>> implied additional need for communicating the lock id to the other peer
>> entity, so a realistic usage is most always the specific API variant. I
>> doubt this API would be of much use for the shared driver usage. This
>> also implies that the client user does not care about specifying a lock
>> in DT.
>>
> 
> Afaict if the lock are shared then there shouldn't be a problem with
> some clients using the request API and others request_specific(). As any
> collisions would simply mean that there are more contention on the lock.
> 
> With the current exclusive model that is not possible and the success of
> the request_specific will depend on probe order.
> 
> But perhaps it should be explicitly prohibited to use both APIs on the
> same hwspinlock instance?

Yeah, they are meant to be complimentary usage, though I doubt we will
ever have any realistic users for the generic API if we haven't had a
usage so far. I had posted a concept of reserved locks long back [1] to
keep away certain locks from the generic requestor, but dropped it since
we did not have an actual use-case needing it.

regards
Suman

[1] https://lwn.net/Articles/611944/

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Fabien DESSENNE]

Hi


On 06/08/2019 11:30 PM, Suman Anna wrote:
> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
>>
>>> Hi Fabien,
>>>
>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>>> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>>>>
>>>>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>>>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>> [..]
>>>>> B/ This would introduce some inconsistency between the two 'request' API
>>>>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>>>>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>>>>> usage. On the other side, request_specific() would request shared locks.
>>>>> Worst the following sequence can transform an exclusive usage into a shared
>>>>>
>>>> There is already an inconsistency in between these; as with above any
>>>> system that uses both request() and request_specific() will be suffering
>>>> from intermittent failures due to probe ordering.
>>>>
>>>>> one:
>>>>>     -hwspin_lock_request() -> returns Id#0 (exclusive)
>>>>>     -hwspin_lock_request() -> returns Id#1 (exclusive)
>>>>>     -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>>>>> Honestly I am not sure that this is a real issue, but it's better to have it
>>>>> in mind before we take ay decision
>>> Wouldn't it be actually simpler to just introduce a new specific API
>>> variant for this, similar to the reset core for example (it uses a
>>> separate exclusive API), without having to modify the bindings at all.
>>> It is just a case of your driver using the right API, and the core can
>>> be modified to use the additional tag semantics based on the API. It
>>> should avoid any confusion with say using a different second cell value
>>> for the same lock in two different nodes.
>>>
>> But this implies that there is an actual need to hold these locks
>> exclusively. Given that they are (except for the raw case) all wrapped
>> by Linux locking primitives there shouldn't be a problem sharing a lock
>> (except possibly for the raw case).
> Yes agreed, the HWLOCK_RAW and HWLOCK_IN_ATOMIC cases are unprotected. I
> am still trying to understand better the usecase to see if the same lock
> is being multiplexed for different protection contexts, or if all of
> them are protecting the same context.


Here are two different examples that explain the need for changes.
In both cases the Linux clients are talking to a single entity on the
remote-side.

Example 1:
     exti: interrupt-controller@5000d000 {
         compatible = "st,stm32mp1-exti", "syscon";
         interrupt-controller;
         #interrupt-cells = <2>;
         reg = <0x5000d000 0x400>;
         hwlocks = <&hsem 1>;
     };
The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
With the current hwspinlock implementation, only the first driver succeeds
in requesting (hwspin_lock_request_specific) the hwlock. The second request
fails.
Here, we really need to share the hwlock between the two drivers.
Note: hardware spinlock support for regmap was 'recently' introduced in 4.15
see https://lore.kernel.org/patchwork/patch/845941/



Example 2:
Here it is more a question of optimization : we want to save the number of
hwlocks used to protect resources, using an unique hwlock to protect all
pinctrl resources:
         pinctrl: pin-controller@50002000 {
             compatible = "st,stm32mp157-pinctrl";
             ranges = <0 0x50002000 0xa400>;
             hwlocks = <&hsem 0 1>;

         pinctrl_z: pin-controller-z@54004000 {
             compatible = "st,stm32mp157-z-pinctrl";
             ranges = <0 0x54004000 0x400>;
             pins-are-numbered;
             hwlocks = <&hsem 0 1>;

>
>> I agree that we shouldn't specify this property in DT - if anything it
>> should be a variant of the API.


If we decide to add a 'shared' API, then, what about the generic regmap 
driver?

In the context of above example1, this would require to update the 
regmap driver.

But would this be acceptable for any driver using syscon/regmap?


I think it is better to keep the existing API (modifying it so it always 
allows

hwlocks sharing, so no need for bindings update) than adding another API.



>>
>>> If you are sharing a hwlock on the Linux side, surely your driver should
>>> be aware that it is a shared lock. The tag can be set during the first
>>> request API, and you look through both tags when giving out a handle.
>>>
>> Why would the driver need to know about it?
> Just the semantics if we were to support single user vs multiple users
> on Linux-side to even get a handle. Your point is that this may be moot
> since we have protection anyway other than the raw cases. But we need to
> be able to have the same API work across all cases.
>
> So far, it had mostly been that there would be one user on Linux
> competing with other equivalent peer entities on different processors.
> It is not common to have multiple users since these protection schemes
> are usually needed only at the lowest levels of a stack, so the
> exclusive handle stuff had been sufficient.
>
>>> Obviously, the hwspin_lock_request() API usage semantics always had the
>>> implied additional need for communicating the lock id to the other peer
>>> entity, so a realistic usage is most always the specific API variant. I
>>> doubt this API would be of much use for the shared driver usage. This
>>> also implies that the client user does not care about specifying a lock
>>> in DT.
>>>
>> Afaict if the lock are shared then there shouldn't be a problem with
>> some clients using the request API and others request_specific(). As any
>> collisions would simply mean that there are more contention on the lock.
>>
>> With the current exclusive model that is not possible and the success of
>> the request_specific will depend on probe order.
>>
>> But perhaps it should be explicitly prohibited to use both APIs on the
>> same hwspinlock instance?
> Yeah, they are meant to be complimentary usage, though I doubt we will
> ever have any realistic users for the generic API if we haven't had a
> usage so far. I had posted a concept of reserved locks long back [1] to
> keep away certain locks from the generic requestor, but dropped it since
> we did not have an actual use-case needing it.
>
> regards
> Suman
>
> [1] https://lwn.net/Articles/611944/

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Suman Anna]

Hi Fabien,

On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
> Hi
> 
> On 06/08/2019 11:30 PM, Suman Anna wrote:
>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
>>>
>>>> Hi Fabien,
>>>>
>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>>>> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>>>>>
>>>>>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>>>>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>>> [..]
>>>>>> B/ This would introduce some inconsistency between the two 'request' API
>>>>>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>>>>>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>>>>>> usage. On the other side, request_specific() would request shared locks.
>>>>>> Worst the following sequence can transform an exclusive usage into a shared
>>>>>>
>>>>> There is already an inconsistency in between these; as with above any
>>>>> system that uses both request() and request_specific() will be suffering
>>>>> from intermittent failures due to probe ordering.
>>>>>
>>>>>> one:
>>>>>>     -hwspin_lock_request() -> returns Id#0 (exclusive)
>>>>>>     -hwspin_lock_request() -> returns Id#1 (exclusive)
>>>>>>     -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>>>>>> Honestly I am not sure that this is a real issue, but it's better to have it
>>>>>> in mind before we take ay decision
>>>> Wouldn't it be actually simpler to just introduce a new specific API
>>>> variant for this, similar to the reset core for example (it uses a
>>>> separate exclusive API), without having to modify the bindings at all.
>>>> It is just a case of your driver using the right API, and the core can
>>>> be modified to use the additional tag semantics based on the API. It
>>>> should avoid any confusion with say using a different second cell value
>>>> for the same lock in two different nodes.
>>>>
>>> But this implies that there is an actual need to hold these locks
>>> exclusively. Given that they are (except for the raw case) all wrapped
>>> by Linux locking primitives there shouldn't be a problem sharing a lock
>>> (except possibly for the raw case).
>> Yes agreed, the HWLOCK_RAW and HWLOCK_IN_ATOMIC cases are unprotected. I
>> am still trying to understand better the usecase to see if the same lock
>> is being multiplexed for different protection contexts, or if all of
>> them are protecting the same context.
> 
> 
> Here are two different examples that explain the need for changes.
> In both cases the Linux clients are talking to a single entity on the
> remote-side.
> 
> Example 1:
>      exti: interrupt-controller@5000d000 {
>          compatible = "st,stm32mp1-exti", "syscon";
>          interrupt-controller;
>          #interrupt-cells = <2>;
>          reg = <0x5000d000 0x400>;
>          hwlocks = <&hsem 1>;
>      };
> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> With the current hwspinlock implementation, only the first driver succeeds
> in requesting (hwspin_lock_request_specific) the hwlock. The second request
> fails.
> Here, we really need to share the hwlock between the two drivers.
> Note: hardware spinlock support for regmap was 'recently' introduced in 4.15
> see https://lore.kernel.org/patchwork/patch/845941/
> 
> 
> 
> Example 2:
> Here it is more a question of optimization : we want to save the number of
> hwlocks used to protect resources, using an unique hwlock to protect all
> pinctrl resources:
>          pinctrl: pin-controller@50002000 {
>              compatible = "st,stm32mp157-pinctrl";
>              ranges = <0 0x50002000 0xa400>;
>              hwlocks = <&hsem 0 1>;
> 
>          pinctrl_z: pin-controller-z@54004000 {
>              compatible = "st,stm32mp157-z-pinctrl";
>              ranges = <0 0x54004000 0x400>;
>              pins-are-numbered;
>              hwlocks = <&hsem 0 1>;

Thanks for the examples.

> 
>>
>>> I agree that we shouldn't specify this property in DT - if anything it
>>> should be a variant of the API.
> 
> 
> If we decide to add a 'shared' API, then, what about the generic regmap 
> driver?
> 
> In the context of above example1, this would require to update the 
> regmap driver.
> 
> But would this be acceptable for any driver using syscon/regmap?
> 
> 
> I think it is better to keep the existing API (modifying it so it always 
> allows
> 
> hwlocks sharing, so no need for bindings update) than adding another API.

For your usecases, you would definitely need the syscon/regmap behavior
to be shared right. Whether we introduce a 'shared' API or an
'exclusive' API and change the current API behavior to shared, it is
definitely a case-by-case usage scenario for the existing drivers and
usage right. The main contention point is what to do with the
unprotected usecases like Bjorn originally pointed out.

regards
Suman

> 
> 
> 
>>>
>>>> If you are sharing a hwlock on the Linux side, surely your driver should
>>>> be aware that it is a shared lock. The tag can be set during the first
>>>> request API, and you look through both tags when giving out a handle.
>>>>
>>> Why would the driver need to know about it?
>> Just the semantics if we were to support single user vs multiple users
>> on Linux-side to even get a handle. Your point is that this may be moot
>> since we have protection anyway other than the raw cases. But we need to
>> be able to have the same API work across all cases.
>>
>> So far, it had mostly been that there would be one user on Linux
>> competing with other equivalent peer entities on different processors.
>> It is not common to have multiple users since these protection schemes
>> are usually needed only at the lowest levels of a stack, so the
>> exclusive handle stuff had been sufficient.
>>
>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
>>>> implied additional need for communicating the lock id to the other peer
>>>> entity, so a realistic usage is most always the specific API variant. I
>>>> doubt this API would be of much use for the shared driver usage. This
>>>> also implies that the client user does not care about specifying a lock
>>>> in DT.
>>>>
>>> Afaict if the lock are shared then there shouldn't be a problem with
>>> some clients using the request API and others request_specific(). As any
>>> collisions would simply mean that there are more contention on the lock.
>>>
>>> With the current exclusive model that is not possible and the success of
>>> the request_specific will depend on probe order.
>>>
>>> But perhaps it should be explicitly prohibited to use both APIs on the
>>> same hwspinlock instance?
>> Yeah, they are meant to be complimentary usage, though I doubt we will
>> ever have any realistic users for the generic API if we haven't had a
>> usage so far. I had posted a concept of reserved locks long back [1] to
>> keep away certain locks from the generic requestor, but dropped it since
>> we did not have an actual use-case needing it.
>>
>> regards
>> Suman
>>
>> [1] https://lwn.net/Articles/611944/

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Fabien DESSENNE]

On 07/08/2019 6:19 PM, Suman Anna wrote:
> Hi Fabien,
>
> On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
>> Hi
>>
>> On 06/08/2019 11:30 PM, Suman Anna wrote:
>>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
>>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
>>>>
>>>>> Hi Fabien,
>>>>>
>>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>>>>> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>>>>>>
>>>>>>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>>>>>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>>>> [..]
>>>>>>> B/ This would introduce some inconsistency between the two 'request' API
>>>>>>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>>>>>>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>>>>>>> usage. On the other side, request_specific() would request shared locks.
>>>>>>> Worst the following sequence can transform an exclusive usage into a shared
>>>>>>>
>>>>>> There is already an inconsistency in between these; as with above any
>>>>>> system that uses both request() and request_specific() will be suffering
>>>>>> from intermittent failures due to probe ordering.
>>>>>>
>>>>>>> one:
>>>>>>>      -hwspin_lock_request() -> returns Id#0 (exclusive)
>>>>>>>      -hwspin_lock_request() -> returns Id#1 (exclusive)
>>>>>>>      -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>>>>>>> Honestly I am not sure that this is a real issue, but it's better to have it
>>>>>>> in mind before we take ay decision
>>>>> Wouldn't it be actually simpler to just introduce a new specific API
>>>>> variant for this, similar to the reset core for example (it uses a
>>>>> separate exclusive API), without having to modify the bindings at all.
>>>>> It is just a case of your driver using the right API, and the core can
>>>>> be modified to use the additional tag semantics based on the API. It
>>>>> should avoid any confusion with say using a different second cell value
>>>>> for the same lock in two different nodes.
>>>>>
>>>> But this implies that there is an actual need to hold these locks
>>>> exclusively. Given that they are (except for the raw case) all wrapped
>>>> by Linux locking primitives there shouldn't be a problem sharing a lock
>>>> (except possibly for the raw case).
>>> Yes agreed, the HWLOCK_RAW and HWLOCK_IN_ATOMIC cases are unprotected. I
>>> am still trying to understand better the usecase to see if the same lock
>>> is being multiplexed for different protection contexts, or if all of
>>> them are protecting the same context.
>>
>> Here are two different examples that explain the need for changes.
>> In both cases the Linux clients are talking to a single entity on the
>> remote-side.
>>
>> Example 1:
>>       exti: interrupt-controller@5000d000 {
>>           compatible = "st,stm32mp1-exti", "syscon";
>>           interrupt-controller;
>>           #interrupt-cells = <2>;
>>           reg = <0x5000d000 0x400>;
>>           hwlocks = <&hsem 1>;
>>       };
>> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
>> With the current hwspinlock implementation, only the first driver succeeds
>> in requesting (hwspin_lock_request_specific) the hwlock. The second request
>> fails.
>> Here, we really need to share the hwlock between the two drivers.
>> Note: hardware spinlock support for regmap was 'recently' introduced in 4.15
>> see https://lore.kernel.org/patchwork/patch/845941/
>>
>>
>>
>> Example 2:
>> Here it is more a question of optimization : we want to save the number of
>> hwlocks used to protect resources, using an unique hwlock to protect all
>> pinctrl resources:
>>           pinctrl: pin-controller@50002000 {
>>               compatible = "st,stm32mp157-pinctrl";
>>               ranges = <0 0x50002000 0xa400>;
>>               hwlocks = <&hsem 0 1>;
>>
>>           pinctrl_z: pin-controller-z@54004000 {
>>               compatible = "st,stm32mp157-z-pinctrl";
>>               ranges = <0 0x54004000 0x400>;
>>               pins-are-numbered;
>>               hwlocks = <&hsem 0 1>;
> Thanks for the examples.
>
>>>> I agree that we shouldn't specify this property in DT - if anything it
>>>> should be a variant of the API.
>>
>> If we decide to add a 'shared' API, then, what about the generic regmap
>> driver?
>>
>> In the context of above example1, this would require to update the
>> regmap driver.
>>
>> But would this be acceptable for any driver using syscon/regmap?
>>
>>
>> I think it is better to keep the existing API (modifying it so it always
>> allows
>>
>> hwlocks sharing, so no need for bindings update) than adding another API.
> For your usecases, you would definitely need the syscon/regmap behavior
> to be shared right. Whether we introduce a 'shared' API or an
> 'exclusive' API and change the current API behavior to shared, it is
> definitely a case-by-case usage scenario for the existing drivers and
> usage right. The main contention point is what to do with the
> unprotected usecases like Bjorn originally pointed out.

OK, I see : the hwspinlock framework does not offer any lock protection 
with the RAW/IN_ATOMIC modes.
This is an issue if several different 'local' drivers try to get a 
shared lock in the same time.
And this is a personal problem since I need to use shared locks in 
...atomic mode.

I have tried to see how it is possible to put a constraint on the 
callers, just like this is documented for the RAW mode which is:
    "Caution: If the mode is HWLOCK_RAW, that means user must protect 
the routine
     of getting hardware lock with mutex or spinlock.."
I do not think that it is acceptable to ask several drivers to share a 
common mutex/spinlock for shared locks.
But I think about another option: the driver implementing the trylock 
ops may offer such protection. This is the case if the driver returns 
"busy" if the lock is already taken, not only by the remote processor, 
but also by the local host.

So what do you think about adding such a documentation note :
"Caution : the HWLOCK_RAW / HWLOCK_IN_ATOMIC modes shall not be used 
with shared locks unless the hwspinlock driver supports local lock 
protection"

Optionally, we may add a "local_lock_protection" flag in the 
hwspinlock_device struct, set by the driver before it calls 
hwspin_lock_register().
This flag can then be checked by hwspinlock core to allow/deny use of 
shared locks in the raw/atomic modes.

Let me know what you think about it.

BR

Fabien

>
> regards
> Suman
>
>>
>>
>>>>> If you are sharing a hwlock on the Linux side, surely your driver should
>>>>> be aware that it is a shared lock. The tag can be set during the first
>>>>> request API, and you look through both tags when giving out a handle.
>>>>>
>>>> Why would the driver need to know about it?
>>> Just the semantics if we were to support single user vs multiple users
>>> on Linux-side to even get a handle. Your point is that this may be moot
>>> since we have protection anyway other than the raw cases. But we need to
>>> be able to have the same API work across all cases.
>>>
>>> So far, it had mostly been that there would be one user on Linux
>>> competing with other equivalent peer entities on different processors.
>>> It is not common to have multiple users since these protection schemes
>>> are usually needed only at the lowest levels of a stack, so the
>>> exclusive handle stuff had been sufficient.
>>>
>>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
>>>>> implied additional need for communicating the lock id to the other peer
>>>>> entity, so a realistic usage is most always the specific API variant. I
>>>>> doubt this API would be of much use for the shared driver usage. This
>>>>> also implies that the client user does not care about specifying a lock
>>>>> in DT.
>>>>>
>>>> Afaict if the lock are shared then there shouldn't be a problem with
>>>> some clients using the request API and others request_specific(). As any
>>>> collisions would simply mean that there are more contention on the lock.
>>>>
>>>> With the current exclusive model that is not possible and the success of
>>>> the request_specific will depend on probe order.
>>>>
>>>> But perhaps it should be explicitly prohibited to use both APIs on the
>>>> same hwspinlock instance?
>>> Yeah, they are meant to be complimentary usage, though I doubt we will
>>> ever have any realistic users for the generic API if we haven't had a
>>> usage so far. I had posted a concept of reserved locks long back [1] to
>>> keep away certain locks from the generic requestor, but dropped it since
>>> we did not have an actual use-case needing it.
>>>
>>> regards
>>> Suman
>>>
>>> [1] https://lwn.net/Articles/611944/

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Bjorn Andersson]

On Thu 08 Aug 05:52 PDT 2019, Fabien DESSENNE wrote:

> 
> On 07/08/2019 6:19 PM, Suman Anna wrote:
> > Hi Fabien,
> >
> > On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
> >> Hi
> >>
> >> On 06/08/2019 11:30 PM, Suman Anna wrote:
> >>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
> >>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
> >>>>
> >>>>> Hi Fabien,
> >>>>>
> >>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
> >>>> I agree that we shouldn't specify this property in DT - if anything it
> >>>> should be a variant of the API.
> >>
> >> If we decide to add a 'shared' API, then, what about the generic regmap
> >> driver?
> >>
> >> In the context of above example1, this would require to update the
> >> regmap driver.
> >>
> >> But would this be acceptable for any driver using syscon/regmap?
> >>
> >>
> >> I think it is better to keep the existing API (modifying it so it always
> >> allows
> >>
> >> hwlocks sharing, so no need for bindings update) than adding another API.
> > For your usecases, you would definitely need the syscon/regmap behavior
> > to be shared right. Whether we introduce a 'shared' API or an
> > 'exclusive' API and change the current API behavior to shared, it is
> > definitely a case-by-case usage scenario for the existing drivers and
> > usage right. The main contention point is what to do with the
> > unprotected usecases like Bjorn originally pointed out.
> 
> OK, I see : the hwspinlock framework does not offer any lock protection 
> with the RAW/IN_ATOMIC modes.
> This is an issue if several different 'local' drivers try to get a 
> shared lock in the same time.
> And this is a personal problem since I need to use shared locks in 
> ...atomic mode.
> 

Why can't you use HWLOCK_IRQSTATE in this mode?

> I have tried to see how it is possible to put a constraint on the 
> callers, just like this is documented for the RAW mode which is:
>     "Caution: If the mode is HWLOCK_RAW, that means user must protect 
> the routine
>      of getting hardware lock with mutex or spinlock.."
> I do not think that it is acceptable to ask several drivers to share a 
> common mutex/spinlock for shared locks.

No it's not.

> But I think about another option: the driver implementing the trylock 
> ops may offer such protection. This is the case if the driver returns 
> "busy" if the lock is already taken, not only by the remote processor, 
> but also by the local host.
> 

I think it's typical for hwspinlock hardware to not be able to
distinguish between different clients within Linux, so we would need to
wrap the usage in some construct that ensures mutual exclusion in Linux
- like a spinlock...

> So what do you think about adding such a documentation note :
> "Caution : the HWLOCK_RAW / HWLOCK_IN_ATOMIC modes shall not be used 
> with shared locks unless the hwspinlock driver supports local lock 
> protection"
> 

But having local lock protection in the hwspinlock driver would defeat
the purpose of HWLOCK_RAW.

Also this kind of warning will at best be consumed by the client driver
authors, it will not be read by the dts authors.

Regards,
Bjorn

> Optionally, we may add a "local_lock_protection" flag in the 
> hwspinlock_device struct, set by the driver before it calls 
> hwspin_lock_register().
> This flag can then be checked by hwspinlock core to allow/deny use of 
> shared locks in the raw/atomic modes.
> 
> Let me know what you think about it.
> 
> BR
> 
> Fabien
> 
> >
> > regards
> > Suman
> >
> >>
> >>
> >>>>> If you are sharing a hwlock on the Linux side, surely your driver should
> >>>>> be aware that it is a shared lock. The tag can be set during the first
> >>>>> request API, and you look through both tags when giving out a handle.
> >>>>>
> >>>> Why would the driver need to know about it?
> >>> Just the semantics if we were to support single user vs multiple users
> >>> on Linux-side to even get a handle. Your point is that this may be moot
> >>> since we have protection anyway other than the raw cases. But we need to
> >>> be able to have the same API work across all cases.
> >>>
> >>> So far, it had mostly been that there would be one user on Linux
> >>> competing with other equivalent peer entities on different processors.
> >>> It is not common to have multiple users since these protection schemes
> >>> are usually needed only at the lowest levels of a stack, so the
> >>> exclusive handle stuff had been sufficient.
> >>>
> >>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
> >>>>> implied additional need for communicating the lock id to the other peer
> >>>>> entity, so a realistic usage is most always the specific API variant. I
> >>>>> doubt this API would be of much use for the shared driver usage. This
> >>>>> also implies that the client user does not care about specifying a lock
> >>>>> in DT.
> >>>>>
> >>>> Afaict if the lock are shared then there shouldn't be a problem with
> >>>> some clients using the request API and others request_specific(). As any
> >>>> collisions would simply mean that there are more contention on the lock.
> >>>>
> >>>> With the current exclusive model that is not possible and the success of
> >>>> the request_specific will depend on probe order.
> >>>>
> >>>> But perhaps it should be explicitly prohibited to use both APIs on the
> >>>> same hwspinlock instance?
> >>> Yeah, they are meant to be complimentary usage, though I doubt we will
> >>> ever have any realistic users for the generic API if we haven't had a
> >>> usage so far. I had posted a concept of reserved locks long back [1] to
> >>> keep away certain locks from the generic requestor, but dropped it since
> >>> we did not have an actual use-case needing it.
> >>>
> >>> regards
> >>> Suman
> >>>
> >>> [1] https://lwn.net/Articles/611944/

Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Fabien DESSENNE]

Hi Bjorn,


On 08/08/2019 5:37 PM, Bjorn Andersson wrote:
> On Thu 08 Aug 05:52 PDT 2019, Fabien DESSENNE wrote:
>
>> On 07/08/2019 6:19 PM, Suman Anna wrote:
>>> Hi Fabien,
>>>
>>> On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
>>>> Hi
>>>>
>>>> On 06/08/2019 11:30 PM, Suman Anna wrote:
>>>>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
>>>>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
>>>>>>
>>>>>>> Hi Fabien,
>>>>>>>
>>>>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>>>>> I agree that we shouldn't specify this property in DT - if anything it
>>>>>> should be a variant of the API.
>>>> If we decide to add a 'shared' API, then, what about the generic regmap
>>>> driver?
>>>>
>>>> In the context of above example1, this would require to update the
>>>> regmap driver.
>>>>
>>>> But would this be acceptable for any driver using syscon/regmap?
>>>>
>>>>
>>>> I think it is better to keep the existing API (modifying it so it always
>>>> allows
>>>>
>>>> hwlocks sharing, so no need for bindings update) than adding another API.
>>> For your usecases, you would definitely need the syscon/regmap behavior
>>> to be shared right. Whether we introduce a 'shared' API or an
>>> 'exclusive' API and change the current API behavior to shared, it is
>>> definitely a case-by-case usage scenario for the existing drivers and
>>> usage right. The main contention point is what to do with the
>>> unprotected usecases like Bjorn originally pointed out.
>> OK, I see : the hwspinlock framework does not offer any lock protection
>> with the RAW/IN_ATOMIC modes.
>> This is an issue if several different 'local' drivers try to get a
>> shared lock in the same time.
>> And this is a personal problem since I need to use shared locks in
>> ...atomic mode.
>>
> Why can't you use HWLOCK_IRQSTATE in this mode?
>
>> I have tried to see how it is possible to put a constraint on the
>> callers, just like this is documented for the RAW mode which is:
>>      "Caution: If the mode is HWLOCK_RAW, that means user must protect
>> the routine
>>       of getting hardware lock with mutex or spinlock.."
>> I do not think that it is acceptable to ask several drivers to share a
>> common mutex/spinlock for shared locks.
> No it's not.
>
>> But I think about another option: the driver implementing the trylock
>> ops may offer such protection. This is the case if the driver returns
>> "busy" if the lock is already taken, not only by the remote processor,
>> but also by the local host.
>>
> I think it's typical for hwspinlock hardware to not be able to
> distinguish between different clients within Linux, so we would need to


Agree with that, let's forget this idea.


> wrap the usage in some construct that ensures mutual exclusion in Linux
> - like a spinlock...
>
>> So what do you think about adding such a documentation note :
>> "Caution : the HWLOCK_RAW / HWLOCK_IN_ATOMIC modes shall not be used
>> with shared locks unless the hwspinlock driver supports local lock
>> protection"
>>
> But having local lock protection in the hwspinlock driver would defeat
> the purpose of HWLOCK_RAW.


My understanding is that the purpose of the RAW mode is to allow the 
user to do some time-consuming or sleepable operations under the 
hardware spinlock protection.

This is probably the reason why the RAW mode does not uses any spinlock 
is used in RAW mode.

But I do not think that this is a requirement to not use any local 
protection.

So, in this mode, instead of using a spinlock, what about calling the 
atomic bitop test_and_set_bit()  ?

This would ensure safe concurrency between the hwspinlock linux users, 
and will respect the purpose of the RAW mode.

Let me know if this is acceptable.


BR

Fabien


>
> Also this kind of warning will at best be consumed by the client driver
> authors, it will not be read by the dts authors.
>
> Regards,
> Bjorn
>
>> Optionally, we may add a "local_lock_protection" flag in the
>> hwspinlock_device struct, set by the driver before it calls
>> hwspin_lock_register().
>> This flag can then be checked by hwspinlock core to allow/deny use of
>> shared locks in the raw/atomic modes.
>>
>> Let me know what you think about it.
>>
>> BR
>>
>> Fabien
>>
>>> regards
>>> Suman
>>>
>>>>
>>>>>>> If you are sharing a hwlock on the Linux side, surely your driver should
>>>>>>> be aware that it is a shared lock. The tag can be set during the first
>>>>>>> request API, and you look through both tags when giving out a handle.
>>>>>>>
>>>>>> Why would the driver need to know about it?
>>>>> Just the semantics if we were to support single user vs multiple users
>>>>> on Linux-side to even get a handle. Your point is that this may be moot
>>>>> since we have protection anyway other than the raw cases. But we need to
>>>>> be able to have the same API work across all cases.
>>>>>
>>>>> So far, it had mostly been that there would be one user on Linux
>>>>> competing with other equivalent peer entities on different processors.
>>>>> It is not common to have multiple users since these protection schemes
>>>>> are usually needed only at the lowest levels of a stack, so the
>>>>> exclusive handle stuff had been sufficient.
>>>>>
>>>>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
>>>>>>> implied additional need for communicating the lock id to the other peer
>>>>>>> entity, so a realistic usage is most always the specific API variant. I
>>>>>>> doubt this API would be of much use for the shared driver usage. This
>>>>>>> also implies that the client user does not care about specifying a lock
>>>>>>> in DT.
>>>>>>>
>>>>>> Afaict if the lock are shared then there shouldn't be a problem with
>>>>>> some clients using the request API and others request_specific(). As any
>>>>>> collisions would simply mean that there are more contention on the lock.
>>>>>>
>>>>>> With the current exclusive model that is not possible and the success of
>>>>>> the request_specific will depend on probe order.
>>>>>>
>>>>>> But perhaps it should be explicitly prohibited to use both APIs on the
>>>>>> same hwspinlock instance?
>>>>> Yeah, they are meant to be complimentary usage, though I doubt we will
>>>>> ever have any realistic users for the generic API if we haven't had a
>>>>> usage so far. I had posted a concept of reserved locks long back [1] to
>>>>> keep away certain locks from the generic requestor, but dropped it since
>>>>> we did not have an actual use-case needing it.
>>>>>
>>>>> regards
>>>>> Suman
>>>>>
>>>>> [1] https://lwn.net/Articles/611944/