[PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

This patch follows the discussion based on the first RFC series posted on the
mailing list [1]. The discussion resulted in a couple of directives for
hwspinlocks that do not want the framework imposing a s/w spinlock around the
hwspinlock.

i. The default should only be a s/w spinlock around the hwspinlock to ensure
correctness of locking.

ii. Existing users may not use raw capability, unless the platform registers
support for it.

iii. Platform driver for hwspinlock should dictate which locks can be operated
in raw mode.

iv. Platform driver and the hw holds the responsibility to ensure the
correctness of acquiring the hwspinlock.

This patchset implements these directives.

Changes since RFC v1:
- Introduce 'raw' capability for hwspinlocks.
- Platform code now has to explicitly specify the raw capability of a lock.
- Check to ensure that only those locks explicitly marked as raw capable can be
  locked/unlocked through the _raw api 
- QCOM patch for making lock #7 raw capable added.
- Add documentation

Thanks,
Lina

[1]. https://patches.linaro.org/47895/

Lina Iyer (2):
  hwspinlock: Introduce raw capability for hwspinlocks
  hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

 Documentation/hwspinlock.txt             | 16 +++++++
 drivers/hwspinlock/hwspinlock_core.c     | 75 +++++++++++++++++++-------------
 drivers/hwspinlock/hwspinlock_internal.h |  6 +++
 drivers/hwspinlock/qcom_hwspinlock.c     | 22 +++++++---
 include/linux/hwspinlock.h               | 41 +++++++++++++++++
 5 files changed, 125 insertions(+), 35 deletions(-)

-- 
2.1.4

[PATCH RFC v2 1/2] hwspinlock: Introduce raw capability for hwspinlocks

[Lina Iyer]

The hwspinlock framework, uses a s/w spin lock around the hw spinlock to
ensure that only process acquires the lock at any time. This is the most
general use case. A special case is where a hwspinlock may be acquired
in Linux and a remote entity may release the lock. In such a case, the
s/w spinlock may never be unlocked as Linux would never call
hwspin_unlock on the hwlock.

This special case is needed for serializing the processor across context
switches from Linux to firmware. Multiple cores may enter cpu idle and
would switch context to firmware to power off. A cpu holding the
hwspinlock would cause other cpus to wait to acquire the lock, until the
lock is released by the firmware. The last core to power down per Linux
has the correct state of the shared resources and should be the one
considered by the firmware. However, a cpu may be stuck handling FIQs
and therefore the last man view of Linux and the firmware may differ. A
hwspinlock avoids this problem by serializing the entry from Linux to
firmware.

Introduce hwcaps member for hwspinlock_device. The hwcaps represents the
hw capability of each hwlock. The platform driver is responsible for
specifying this capability for each lock in the bank. A lock that has
HWL_CAP_ALLOW_RAW set, would indicate to the framework, the capability
for ensure locking correctness in the platform. Since no sw spinlock
guards the hwspinlock, it is the responsibility of the platform driver
to ensure that an unique value is written to the hwspinlock to ensure
locking correctness.

Drivers may use hwspin_trylock_raw() and hwspin_unlock_raw() api to lock
and unlock a hwlock with raw capability.

Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Andy Gross <agross@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 Documentation/hwspinlock.txt             | 16 +++++++
 drivers/hwspinlock/hwspinlock_core.c     | 75 +++++++++++++++++++-------------
 drivers/hwspinlock/hwspinlock_internal.h |  6 +++
 include/linux/hwspinlock.h               | 41 +++++++++++++++++
 4 files changed, 108 insertions(+), 30 deletions(-)

diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
index 62f7d4e..ca6de6c 100644
--- a/Documentation/hwspinlock.txt
+++ b/Documentation/hwspinlock.txt
@@ -122,6 +122,15 @@ independent, drivers.
      notably -EBUSY if the hwspinlock was already taken).
      The function will never sleep.
 
+  int hwspin_trylock_raw(struct hwspinlock *hwlock);
+   - attempt to lock a previously-assigned hwspinlock, but immediately fail if
+     it is already taken. The lock must have been declared by the platform
+     drv code with raw capability support.
+     Returns 0 on success and an appropriate error code otherwise (most
+     notably -EBUSY if the hwspinlock was already taken).
+     This function does not use a sw spinlock around the hwlock. The
+     responsiblity of the lock is guaranteed by the platform code.
+
   void hwspin_unlock(struct hwspinlock *hwlock);
    - unlock a previously-locked hwspinlock. Always succeed, and can be called
      from any context (the function never sleeps). Note: code should _never_
@@ -144,6 +153,13 @@ independent, drivers.
      and the state of the local interrupts is restored to the state saved at
      the given flags. This function will never sleep.
 
+  void hwspin_unlock_raw(struct hwspinlock *hwlock);
+   - unlock a previously-locked hwspinlock. Always succeed, and can be called
+     from any context (the function never sleeps). Note: code should _never_
+     unlock an hwspinlock which is already unlocked (there is no protection
+     against this). The platform driver must support raw capability for this
+     hwlock.
+
   int hwspin_lock_get_id(struct hwspinlock *hwlock);
    - retrieve id number of a given hwspinlock. This is needed when an
      hwspinlock is dynamically assigned: before it can be used to achieve
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 461a0d7..18ed7cc 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -79,7 +79,10 @@ static DEFINE_MUTEX(hwspinlock_tree_lock);
  * whether he wants their previous state to be saved. It is up to the user
  * to choose the appropriate @mode of operation, exactly the same way users
  * should decide between spin_trylock, spin_trylock_irq and
- * spin_trylock_irqsave.
+ * spin_trylock_irqsave and even no spinlock, if the hwspinlock is always
+ * acquired in an interrupt disabled context. The platform driver that
+ * registers such a lock, would explicity specify the capability for the
+ * lock with the HWL_CAP_ALLOW_RAW capability flag.
  *
  * Returns 0 if we successfully locked the hwspinlock or -EBUSY if
  * the hwspinlock was already taken.
@@ -91,6 +94,7 @@ int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 
 	BUG_ON(!hwlock);
 	BUG_ON(!flags && mode == HWLOCK_IRQSTATE);
+	BUG_ON((hwlock->hwcaps & HWL_CAP_ALLOW_RAW) && (mode != HWLOCK_NOLOCK));
 
 	/*
 	 * This spin_lock{_irq, _irqsave} serves three purposes:
@@ -105,32 +109,36 @@ int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 	 *    problems with hwspinlock usage (e.g. scheduler checks like
 	 *    'scheduling while atomic' etc.)
 	 */
-	if (mode == HWLOCK_IRQSTATE)
-		ret = spin_trylock_irqsave(&hwlock->lock, *flags);
-	else if (mode == HWLOCK_IRQ)
-		ret = spin_trylock_irq(&hwlock->lock);
-	else
-		ret = spin_trylock(&hwlock->lock);
-
-	/* is lock already taken by another context on the local cpu ? */
-	if (!ret)
-		return -EBUSY;
-
-	/* try to take the hwspinlock device */
-	ret = hwlock->bank->ops->trylock(hwlock);
-
-	/* if hwlock is already taken, undo spin_trylock_* and exit */
-	if (!ret) {
+	if (mode != HWLOCK_NOLOCK) {
 		if (mode == HWLOCK_IRQSTATE)
-			spin_unlock_irqrestore(&hwlock->lock, *flags);
+			ret = spin_trylock_irqsave(&hwlock->lock, *flags);
 		else if (mode == HWLOCK_IRQ)
-			spin_unlock_irq(&hwlock->lock);
+			ret = spin_trylock_irq(&hwlock->lock);
 		else
-			spin_unlock(&hwlock->lock);
+			ret = spin_trylock(&hwlock->lock);
 
-		return -EBUSY;
+		/* is lock already taken by another context on the local cpu? */
+		if (!ret)
+			return -EBUSY;
 	}
 
+	/* try to take the hwspinlock device */
+	ret = hwlock->bank->ops->trylock(hwlock);
+
+	if (mode != HWLOCK_NOLOCK) {
+		/* if hwlock is already taken, undo spin_trylock_* and exit */
+		if (!ret) {
+			if (mode == HWLOCK_IRQSTATE)
+				spin_unlock_irqrestore(&hwlock->lock, *flags);
+			else if (mode == HWLOCK_IRQ)
+				spin_unlock_irq(&hwlock->lock);
+			else
+				spin_unlock(&hwlock->lock);
+		}
+	}
+
+	if (!ret)
+		return -EBUSY;
 	/*
 	 * We can be sure the other core's memory operations
 	 * are observable to us only _after_ we successfully take
@@ -222,7 +230,10 @@ EXPORT_SYMBOL_GPL(__hwspin_lock_timeout);
  * if yes, whether he wants their previous state to be restored. It is up
  * to the user to choose the appropriate @mode of operation, exactly the
  * same way users decide between spin_unlock, spin_unlock_irq and
- * spin_unlock_irqrestore.
+ * spin_unlock_irqrestore and even no spinlock, if the hwspinlock is always
+ * acquired in an interrupt disabled context. The platform driver that
+ * registers such a lock, would explicity specify the capability for the
+ * lock with the HWL_CAP_ALLOW_RAW capability flag.
  *
  * The function will never sleep.
  */
@@ -230,6 +241,7 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 {
 	BUG_ON(!hwlock);
 	BUG_ON(!flags && mode == HWLOCK_IRQSTATE);
+	BUG_ON((hwlock->hwcaps & HWL_CAP_ALLOW_RAW) && (mode != HWLOCK_NOLOCK));
 
 	/*
 	 * We must make sure that memory operations (both reads and writes),
@@ -247,13 +259,15 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 
 	hwlock->bank->ops->unlock(hwlock);
 
-	/* Undo the spin_trylock{_irq, _irqsave} called while locking */
-	if (mode == HWLOCK_IRQSTATE)
-		spin_unlock_irqrestore(&hwlock->lock, *flags);
-	else if (mode == HWLOCK_IRQ)
-		spin_unlock_irq(&hwlock->lock);
-	else
-		spin_unlock(&hwlock->lock);
+	if (mode != HWLOCK_NOLOCK) {
+		/* Undo the spin_trylock{_irq, _irqsave} called while locking */
+		if (mode == HWLOCK_IRQSTATE)
+			spin_unlock_irqrestore(&hwlock->lock, *flags);
+		else if (mode == HWLOCK_IRQ)
+			spin_unlock_irq(&hwlock->lock);
+		else
+			spin_unlock(&hwlock->lock);
+	}
 }
 EXPORT_SYMBOL_GPL(__hwspin_unlock);
 
@@ -342,7 +356,8 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 	for (i = 0; i < num_locks; i++) {
 		hwlock = &bank->lock[i];
 
-		spin_lock_init(&hwlock->lock);
+		if (!(hwlock->hwcaps & HWL_CAP_ALLOW_RAW))
+			spin_lock_init(&hwlock->lock);
 		hwlock->bank = bank;
 
 		ret = hwspin_lock_register_single(hwlock, base_id + i);
diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index d26f78b..24a4d79 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -21,6 +21,9 @@
 #include <linux/spinlock.h>
 #include <linux/device.h>
 
+/* hwspinlock capability properties */
+#define HWL_CAP_ALLOW_RAW	BIT(1)
+
 struct hwspinlock_device;
 
 /**
@@ -44,11 +47,14 @@ struct hwspinlock_ops {
  * @bank: the hwspinlock_device structure which owns this lock
  * @lock: initialized and used by hwspinlock core
  * @priv: private data, owned by the underlying platform-specific hwspinlock drv
+ * @hwcaps: hardware capablity, like raw lock, that does not need s/w spinlock
+ * around the hwspinlock.
  */
 struct hwspinlock {
 	struct hwspinlock_device *bank;
 	spinlock_t lock;
 	void *priv;
+	int hwcaps;
 };
 
 /**
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 3343298..21232d0 100644
--- a/include/linux/hwspinlock.h
+++ b/include/linux/hwspinlock.h
@@ -24,6 +24,7 @@
 /* hwspinlock mode argument */
 #define HWLOCK_IRQSTATE	0x01	/* Disable interrupts, save state */
 #define HWLOCK_IRQ	0x02	/* Disable interrupts, don't save state */
+#define HWLOCK_NOLOCK	0xFF	/* Dont take any lock */
 
 struct device;
 struct hwspinlock;
@@ -189,6 +190,27 @@ static inline int hwspin_trylock(struct hwspinlock *hwlock)
 }
 
 /**
+ * hwspin_trylock_raw() - attempt to lock a specific hwspinlock without s/w
+ * spinlocks
+ * @hwlock: the hwspinlock which we want to trylock
+ *
+ * This function attempts to lock the hwspinlock without acquiring a s/w
+ * spinlock. The function will return failure if the lock is already taken.
+ *
+ * The function can only be used on a hwlock that has been initialized with
+ * raw capability by the platform drv.
+ *
+ * The function is expected to be called in an interrupt disabled context.
+ *
+ * Returns 0 if we successfully locked the hwspinlock, -EBUSY if the hwspinlock
+ * is already taken.
+ */
+static inline int hwspin_trylock_raw(struct hwspinlock *hwlock)
+{
+	return __hwspin_trylock(hwlock, HWLOCK_NOLOCK, NULL);
+}
+
+/**
  * hwspin_lock_timeout_irqsave() - lock hwspinlock, with timeout, disable irqs
  * @hwlock: the hwspinlock to be locked
  * @to: timeout value in msecs
@@ -310,4 +332,23 @@ static inline void hwspin_unlock(struct hwspinlock *hwlock)
 	__hwspin_unlock(hwlock, 0, NULL);
 }
 
+/**
+ * hwspin_unlock_raw() - unlock hwspinlock
+ * @hwlock: a previously acquired hwspinlock which we want to unlock
+ *
+ * This function will unlock a specific hwspinlock that was acquired using the
+ * hwspin_trylock_raw() call.
+ *
+ * The function can only be used on a hwlock that has been initialized with
+ * raw capability by the platform drv.
+ *
+ * @hwlock must be already locked (e.g. by hwspin_trylock()) before calling
+ * this function: it is a bug to call unlock on a @hwlock that is already
+ * unlocked.
+ */
+static inline void hwspin_unlock_raw(struct hwspinlock *hwlock)
+{
+	__hwspin_unlock(hwlock, HWLOCK_NOLOCK, NULL);
+}
+
 #endif /* __LINUX_HWSPINLOCK_H */
-- 
2.1.4

[PATCH RFC v2 2/2] hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

[Lina Iyer]

Hwspinlocks are widely used between processors in an SoC, and also
between elevation levels within in the same processor.  QCOM SoC's use
hwspinlock to serialize entry into a low power mode when the context
switches from Linux to secure monitor.

Lock #7 has been assigned for this purpose. In order to differentiate
between one cpu core holding a lock while another cpu is contending for
the same lock, the proc id written into the lock is (128 + cpu id). This
makes it unique value among the cpu cores and therefore when a core
locks the hwspinlock, other cores would wait for the lock to be released
since they would have a different proc id.  This value is specific for
the lock #7 only.

Declare lock #7 as raw capable, so the hwspinlock framework would not
enfore acquiring a s/w spinlock before acquiring the hwspinlock.

Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Cc: Andy Gross <agross@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/hwspinlock/qcom_hwspinlock.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c
index 93b62e0..59278b0 100644
--- a/drivers/hwspinlock/qcom_hwspinlock.c
+++ b/drivers/hwspinlock/qcom_hwspinlock.c
@@ -25,16 +25,26 @@
 
 #include "hwspinlock_internal.h"
 
-#define QCOM_MUTEX_APPS_PROC_ID	1
-#define QCOM_MUTEX_NUM_LOCKS	32
+#define QCOM_MUTEX_APPS_PROC_ID		1
+#define QCOM_MUTEX_CPUIDLE_OFFSET	128
+#define QCOM_CPUIDLE_LOCK		7
+#define QCOM_MUTEX_NUM_LOCKS		32
+
+static inline u32 __qcom_get_proc_id(struct hwspinlock *lock)
+{
+	return hwspin_lock_get_id(lock) == QCOM_CPUIDLE_LOCK ?
+			(QCOM_MUTEX_CPUIDLE_OFFSET + smp_processor_id()) :
+			QCOM_MUTEX_APPS_PROC_ID;
+}
 
 static int qcom_hwspinlock_trylock(struct hwspinlock *lock)
 {
 	struct regmap_field *field = lock->priv;
 	u32 lock_owner;
 	int ret;
+	u32 proc_id = __qcom_get_proc_id(lock);
 
-	ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID);
+	ret = regmap_field_write(field, proc_id);
 	if (ret)
 		return ret;
 
@@ -42,7 +52,7 @@ static int qcom_hwspinlock_trylock(struct hwspinlock *lock)
 	if (ret)
 		return ret;
 
-	return lock_owner == QCOM_MUTEX_APPS_PROC_ID;
+	return lock_owner == proc_id;
 }
 
 static void qcom_hwspinlock_unlock(struct hwspinlock *lock)
@@ -57,7 +67,7 @@ static void qcom_hwspinlock_unlock(struct hwspinlock *lock)
 		return;
 	}
 
-	if (lock_owner != QCOM_MUTEX_APPS_PROC_ID) {
+	if (lock_owner != __qcom_get_proc_id(lock)) {
 		pr_err("%s: spinlock not owned by us (actual owner is %d)\n",
 				__func__, lock_owner);
 	}
@@ -129,6 +139,8 @@ static int qcom_hwspinlock_probe(struct platform_device *pdev)
 							     regmap, field);
 	}
 
+	bank->lock[QCOM_CPUIDLE_LOCK].hwcaps = HWL_CAP_ALLOW_RAW;
+
 	pm_runtime_enable(&pdev->dev);
 
 	ret = hwspin_lock_register(bank, &pdev->dev, &qcom_hwspinlock_ops,
-- 
2.1.4

Re: [PATCH RFC v2 1/2] hwspinlock: Introduce raw capability for hwspinlocks

[Jeffrey Hugo]

Two minor nits I noticed.

On 6/9/2015 10:23 AM, Lina Iyer wrote:
> The hwspinlock framework, uses a s/w spin lock around the hw spinlock to
> ensure that only process acquires the lock at any time. This is the most

Should this be "ensure that only one process"?

<snip>

> Introduce hwcaps member for hwspinlock_device. The hwcaps represents the

Technically you added it to the hwspinlock struct, not 
hwspinlock_device.  Mentioning that the hwcaps member was added to 
hwspinlock_device in the description here may be slightly confusing.

-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH RFC v2 2/2] hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

[Bjorn Andersson]

On Tue, Jun 9, 2015 at 9:23 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Hwspinlocks are widely used between processors in an SoC, and also
> between elevation levels within in the same processor.  QCOM SoC's use
> hwspinlock to serialize entry into a low power mode when the context
> switches from Linux to secure monitor.
>
> Lock #7 has been assigned for this purpose. In order to differentiate
> between one cpu core holding a lock while another cpu is contending for
> the same lock, the proc id written into the lock is (128 + cpu id). This
> makes it unique value among the cpu cores and therefore when a core
> locks the hwspinlock, other cores would wait for the lock to be released
> since they would have a different proc id.  This value is specific for
> the lock #7 only.
>
> Declare lock #7 as raw capable, so the hwspinlock framework would not
> enfore acquiring a s/w spinlock before acquiring the hwspinlock.
>

Hi Lina,

Very sorry for slacking off and missing v1 of this.

I'm puzzled to the concept of using the hwspinlock framework for
lock-only locks. The patch your proposed is rather clean and as long
as there's no lock-debugging added to the framework it would work...


Blindly declaring lock #7 as special on all Qualcomm hwspinlocks I do
however not like at all. There's nothing in either the SFPB nor TCSR
mutex hardware that dictates this fact, it's a system configuration
fact. As such this "requirement" should be described in the device
tree.

The puzzling part of the value to be written is strongly cpuidle
implementation defined makes me wonder if it belong in this driver at
all.

At least this should be configured/flagged by some devicetree
property. "qcom,lock-by-cpu-id-locks = <7, ...>"?


The other alternative to these patches would be to just consume the
syscon in cpuidle and opencode the locking there. It isolates the
cpuidle specifics of this to the original place and it isn't using
only one side of the hwspinlock framework...

Regards,
Bjorn

Re: [PATCH RFC v2 2/2] hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

[Lina Iyer]

On Wed, Jun 10 2015 at 11:33 -0600, Bjorn Andersson wrote:
>On Tue, Jun 9, 2015 at 9:23 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Hwspinlocks are widely used between processors in an SoC, and also
>> between elevation levels within in the same processor.  QCOM SoC's use
>> hwspinlock to serialize entry into a low power mode when the context
>> switches from Linux to secure monitor.
>>
>> Lock #7 has been assigned for this purpose. In order to differentiate
>> between one cpu core holding a lock while another cpu is contending for
>> the same lock, the proc id written into the lock is (128 + cpu id). This
>> makes it unique value among the cpu cores and therefore when a core
>> locks the hwspinlock, other cores would wait for the lock to be released
>> since they would have a different proc id.  This value is specific for
>> the lock #7 only.
>>
>> Declare lock #7 as raw capable, so the hwspinlock framework would not
>> enfore acquiring a s/w spinlock before acquiring the hwspinlock.
>>
>
>Hi Lina,
>
>Very sorry for slacking off and missing v1 of this.
>
No worries. Thanks for reviewing.

>I'm puzzled to the concept of using the hwspinlock framework for
>lock-only locks. The patch your proposed is rather clean and as long
>as there's no lock-debugging added to the framework it would work...
>
>
>Blindly declaring lock #7 as special on all Qualcomm hwspinlocks I do
>however not like at all. There's nothing in either the SFPB nor TCSR
>mutex hardware that dictates this fact, it's a system configuration
>fact. As such this "requirement" should be described in the device
>tree.
>
Its not a mutable entity, but sure.

>The puzzling part of the value to be written is strongly cpuidle
>implementation defined makes me wonder if it belong in this driver at
>all.
>
>At least this should be configured/flagged by some devicetree
>property. "qcom,lock-by-cpu-id-locks = <7, ...>"?
>
Okay.

>
>The other alternative to these patches would be to just consume the
>syscon in cpuidle and opencode the locking there. It isolates the
>cpuidle specifics of this to the original place and it isn't using
>only one side of the hwspinlock framework...
>
Well, ultimately a hwspinlock is just a writel, so that is a
possibility, if we want. But it is a hwspinlock, therefore the use of
the framework seems appropriate, even amidst the unique behavior of the
lock.

Thanks,
Lina

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

Hi Ohad,

Any comments?

Thanks,
Lina

On Tue, Jun 09 2015 at 10:23 -0600, Lina Iyer wrote:
>This patch follows the discussion based on the first RFC series posted on the
>mailing list [1]. The discussion resulted in a couple of directives for
>hwspinlocks that do not want the framework imposing a s/w spinlock around the
>hwspinlock.
>
>i. The default should only be a s/w spinlock around the hwspinlock to ensure
>correctness of locking.
>
>ii. Existing users may not use raw capability, unless the platform registers
>support for it.
>
>iii. Platform driver for hwspinlock should dictate which locks can be operated
>in raw mode.
>
>iv. Platform driver and the hw holds the responsibility to ensure the
>correctness of acquiring the hwspinlock.
>
>This patchset implements these directives.
>
>Changes since RFC v1:
>- Introduce 'raw' capability for hwspinlocks.
>- Platform code now has to explicitly specify the raw capability of a lock.
>- Check to ensure that only those locks explicitly marked as raw capable can be
>  locked/unlocked through the _raw api
>- QCOM patch for making lock #7 raw capable added.
>- Add documentation
>
>Thanks,
>Lina
>
>[1]. https://patches.linaro.org/47895/
>
>Lina Iyer (2):
>  hwspinlock: Introduce raw capability for hwspinlocks
>  hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id
>
> Documentation/hwspinlock.txt             | 16 +++++++
> drivers/hwspinlock/hwspinlock_core.c     | 75 +++++++++++++++++++-------------
> drivers/hwspinlock/hwspinlock_internal.h |  6 +++
> drivers/hwspinlock/qcom_hwspinlock.c     | 22 +++++++---
> include/linux/hwspinlock.h               | 41 +++++++++++++++++
> 5 files changed, 125 insertions(+), 35 deletions(-)
>
>-- 
>2.1.4
>

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

Hi Lina,

On Sat, Jun 27, 2015 at 6:05 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Hi Ohad,
>
> Any comments?

Sorry, I was under the impression the discussion with Bjorn is still open.

Like Bjorn, I'm not so sure too we want to bind a specific lock to the
RAW capability since this is not a lock-specific hardware detail.

As far as I can see, the hardware-specific differences (if any) are at
the vendor level and not at the lock level, therefore it might make
more sense to add the caps member to hwspinlock_device rather than to
the hwspinlock struct (Jeffrey commented about this too).

Thanks,
Ohad.

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Sat, Jun 27 2015 at 05:25 -0600, Ohad Ben-Cohen wrote:
>Hi Lina,
>
>On Sat, Jun 27, 2015 at 6:05 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Hi Ohad,
>>
>> Any comments?
>
>Sorry, I was under the impression the discussion with Bjorn is still open.
>
I am of the opinion that the platform driver and the framework should
handle this request. This variation is still within the bounds of proper
usage of the hw remote lock. hwspinlock frameowkr imposes a s/w spinlock
around access to every hw remote lock and the current QCOM platform
driver assumes that the value written into the hardware, has to be a
constant.  Both of these are assumptions are the limitations in Linux
and is not a hw remote lock behavior.

I do not agree that the cpuidle driver has to memory map a hwspinlock
region and treat it as a register write, because we dont want to
complicate the hwspinlock platform driver.

>Like Bjorn, I'm not so sure too we want to bind a specific lock to the
>RAW capability since this is not a lock-specific hardware detail.
>
You are right, RAW capability is not lock specific. But we dont want to
impose this on every lock in the bank either. Drivers rely on the
framework's s/w spinlock to ensure that different processes in Linux,
trying to lock the same hwspinlock may correctly acquire. The framework
shall guarantee that the hwspinlock is correctly acquired for regular
usecases (where a constant value is written to the h/w t olock). The RAW
capability assumes that the driver acquiring the RAW lock, knows that
the platform will write a unique value to the h/w and therefore the
correctness of locking is assured by the h/w.

>As far as I can see, the hardware-specific differences (if any) are at
>the vendor level and not at the lock level, therefore it might make
>more sense to add the caps member to hwspinlock_device rather than to
>the hwspinlock struct (Jeffrey commented about this too).
>
Jeff's comment is about my commit text pointing to the wrong structure.
I believe he is fine with the implementation. We debated this idea,
before I came up with this patch.

Thanks,
Lina

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

Hi Lina,

On Thu, Jul 2, 2015 at 11:30 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> You are right, RAW capability is not lock specific. But we dont want to
> impose this on every lock in the bank either.

I'm not sure I'm following your concern here: drivers still need to
explicitly indicate RAW in order for this to kick in, so this lenient
approach is not being imposed on them.

Your original patch allowed every driver on all platforms to disable
the sw spinlock mechanism. What I'm merely suggesting is that the
underlying platform-specific driver should first allow this before it
is being used, as some vendors prohibit this completely.

Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.

Thanks,
Ohad.

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Sat, Jul 18 2015 at 05:31 -0600, Ohad Ben-Cohen wrote:
>Hi Lina,
>
>On Thu, Jul 2, 2015 at 11:30 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> You are right, RAW capability is not lock specific. But we dont want to
>> impose this on every lock in the bank either.
>
>I'm not sure I'm following your concern here: drivers still need to
>explicitly indicate RAW in order for this to kick in, so this lenient
>approach is not being imposed on them.
>
Correct.

>Your original patch allowed every driver on all platforms to disable
>the sw spinlock mechanism. What I'm merely suggesting is that the
>underlying platform-specific driver should first allow this before it
>is being used, as some vendors prohibit this completely.
>
Agreed. Thats why the platform driver specifies the capability in hwcaps
flag.

>Let's not make this more complicated than needed, so please add the
>hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
>could always change this later if it proves to be insufficient.
>
But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API, while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would). That is why you should have the capability on
hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
capable should be used as RAW only.

QCOM platform hwlock #7 is unique that different CPUs trying to acquire
the lock would write different values and hence would be fine. But, the
same is not true for other locks in the bank.

Please let me know if this is not clear.

Thanks,
Lina

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Let's not make this more complicated than needed, so please add the
>> hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
>> could always change this later if it proves to be insufficient.
>>
> But this could yield wrong locking scenarios. If banks are allowed RAW
> capability and is not enforced on a per-lock basis, a driver may lock
> using non-raw lock using the _raw API, while another driver may
> 'acquire' the lock (since the value written to the lock would be the
> same as raw api would). That is why you should have the capability on
> hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
> capable should be used as RAW only.
>
> QCOM platform hwlock #7 is unique that different CPUs trying to acquire
> the lock would write different values and hence would be fine. But, the
> same is not true for other locks in the bank.

As far as I understand, there is nothing special about QCOM's hwlock
#7 in terms of hardware. It's exactly the same lock as all the others.

The only difference in hwlock #7 is the way you use it, and that
sounds like a decision the driver should be able to make. It's a
policy, and I'm not sure we should put it in the DT. I'm also not sure
we need this hwlock-specific complexity in the hwspinlock framework.

The driver already makes a decision whether to disable the interrupts
or not and whether to save their state or not. So it can also make a
decision whether to take a sw spinlock at all or not --- if the
hardware allows it. and that if should be encoded in an accessible
vendor specific (not hwlock specific) struct, which is setup by the
underlying vendor specific hwspinlock driver (no DT involved).

Let's go over your aforementioned concerns:
> But this could yield wrong locking scenarios. If banks are allowed RAW
> capability and is not enforced on a per-lock basis, a driver may lock
> using non-raw lock using the _raw API

If this is allowed by the hardware, then this is a valid scenario.
There's no such thing a non-raw lock: a lock is raw if a raw
functionality is required.

> while another driver may
> 'acquire' the lock (since the value written to the lock would be the
> same as raw api would).

Not sure I understand this one. If a lock has already been assigned to
a driver, it cannot be re-assigned to another driver.

Thanks,
Ohad.

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Andy Gross]

On Thu, Aug 13, 2015 at 09:34:13AM +0300, Ohad Ben-Cohen wrote:
> On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
> >> Let's not make this more complicated than needed, so please add the
> >> hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
> >> could always change this later if it proves to be insufficient.
> >>
> > But this could yield wrong locking scenarios. If banks are allowed RAW
> > capability and is not enforced on a per-lock basis, a driver may lock
> > using non-raw lock using the _raw API, while another driver may
> > 'acquire' the lock (since the value written to the lock would be the
> > same as raw api would). That is why you should have the capability on
> > hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
> > capable should be used as RAW only.
> >
> > QCOM platform hwlock #7 is unique that different CPUs trying to acquire
> > the lock would write different values and hence would be fine. But, the
> > same is not true for other locks in the bank.
> 
> As far as I understand, there is nothing special about QCOM's hwlock
> #7 in terms of hardware. It's exactly the same lock as all the others.
> 
> The only difference in hwlock #7 is the way you use it, and that
> sounds like a decision the driver should be able to make. It's a
> policy, and I'm not sure we should put it in the DT. I'm also not sure
> we need this hwlock-specific complexity in the hwspinlock framework.

The issue in hardwiring this into the driver itself means forfeiting
extensibility.  So on one side (w/ raw support), we get the ability to deal with
the lock number changing.  On the other side (w/o raw), we'd have to probably
tie this to chip compat to figure out which lock is the 'special' if it ever
changes.

> 
> The driver already makes a decision whether to disable the interrupts
> or not and whether to save their state or not. So it can also make a
> decision whether to take a sw spinlock at all or not --- if the
> hardware allows it. and that if should be encoded in an accessible
> vendor specific (not hwlock specific) struct, which is setup by the
> underlying vendor specific hwspinlock driver (no DT involved).

It's arbitrary right now.  The remote processor selected a number, not the
processor running Linux.

> 
> Let's go over your aforementioned concerns:
> > But this could yield wrong locking scenarios. If banks are allowed RAW
> > capability and is not enforced on a per-lock basis, a driver may lock
> > using non-raw lock using the _raw API
> 
> If this is allowed by the hardware, then this is a valid scenario.
> There's no such thing a non-raw lock: a lock is raw if a raw
> functionality is required.
> 
> > while another driver may
> > 'acquire' the lock (since the value written to the lock would be the
> > same as raw api would).
> 
> Not sure I understand this one. If a lock has already been assigned to
> a driver, it cannot be re-assigned to another driver.
> 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross <agross@codeaurora.org> wrote:
> The issue in hardwiring this into the driver itself means forfeiting
> extensibility.  So on one side (w/ raw support), we get the ability to deal with
> the lock number changing.  On the other side (w/o raw), we'd have to probably
> tie this to chip compat to figure out which lock is the 'special' if it ever
> changes.

It sounds like the decision "which lock to use" is a separate problem
from "can it go raw".

If the hardware doesn't prohibit raw mode, then every lock can be used
in raw mode. So you just have to pick one and make sure both sides
know which lock you use --- which is a classic multi-processor
synchronization issue.

> It's arbitrary right now.  The remote processor selected a number, not the
> processor running Linux.

Is the number hardcoded right now? and you're using
hwspin_lock_request_specific on the Linux side to acquire the lock?

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Fri, Aug 14 2015 at 04:52 -0600, Ohad Ben-Cohen wrote:
>On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross <agross@codeaurora.org> wrote:
>> The issue in hardwiring this into the driver itself means forfeiting
>> extensibility.  So on one side (w/ raw support), we get the ability to deal with
>> the lock number changing.  On the other side (w/o raw), we'd have to probably
>> tie this to chip compat to figure out which lock is the 'special' if it ever
>> changes.
>
>It sounds like the decision "which lock to use" is a separate problem
>from "can it go raw".
>
Absolutely.

>If the hardware doesn't prohibit raw mode, then every lock can be used
>in raw mode. So you just have to pick one and make sure both sides
>know which lock you use --- which is a classic multi-processor
>synchronization issue.
>
>> It's arbitrary right now.  The remote processor selected a number, not the
>> processor running Linux.
>
>Is the number hardcoded right now? and you're using
>hwspin_lock_request_specific on the Linux side to acquire the lock?
>
Yes, the number is hardcoded in the SPM power controller driver. It
explicitly requests Lock #7

-- Lina

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Thu, Aug 13 2015 at 00:34 -0600, Ohad Ben-Cohen wrote:
>On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> Let's not make this more complicated than needed, so please add the
>>> hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
>>> could always change this later if it proves to be insufficient.
>>>
>> But this could yield wrong locking scenarios. If banks are allowed RAW
>> capability and is not enforced on a per-lock basis, a driver may lock
>> using non-raw lock using the _raw API, while another driver may
>> 'acquire' the lock (since the value written to the lock would be the
>> same as raw api would). That is why you should have the capability on
>> hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
>> capable should be used as RAW only.
>>
>> QCOM platform hwlock #7 is unique that different CPUs trying to acquire
>> the lock would write different values and hence would be fine. But, the
>> same is not true for other locks in the bank.
>
>As far as I understand, there is nothing special about QCOM's hwlock
>#7 in terms of hardware. It's exactly the same lock as all the others.
>
>The only difference in hwlock #7 is the way you use it, and that
>sounds like a decision the driver should be able to make. It's a
>policy, and I'm not sure we should put it in the DT. I'm also not sure
>we need this hwlock-specific complexity in the hwspinlock framework.
>
The way I see it, we made a design assumption that all hwspinlocks would
need a s/w spinlock around it. The lock #7 here challenges that
assumption. The framework imposes a s/w lock and it is only appropriate
that the framework provide an option to overcome that.

The hwspinlock bank is just a way to initalize a set of locks in a SoC.
Nobody needs the bank after that. Drivers access locks individually. It
only seems appropriate that the raw be a property of the lock access.

>The driver already makes a decision whether to disable the interrupts
>or not and whether to save their state or not. So it can also make a
>decision whether to take a sw spinlock at all or not --- if the
>hardware allows it. and that if should be encoded in an accessible
>vendor specific (not hwlock specific) struct, which is setup by the
>underlying vendor specific hwspinlock driver (no DT involved).
>
Would you rather query the hwspinlock driver to see if the framework
should take a s/w spinlock or not, IOW, raw-accessible or not?
That could work.
Every time we call into the raw access API, the framework could query
the hwspinlock driver and then bail out if the hwlock is not raw
accessible.

>Let's go over your aforementioned concerns:
>> But this could yield wrong locking scenarios. If banks are allowed RAW
>> capability and is not enforced on a per-lock basis, a driver may lock
>> using non-raw lock using the _raw API
>
>If this is allowed by the hardware, then this is a valid scenario.
>There's no such thing a non-raw lock: a lock is raw if a raw
>functionality is required.
>
Agreed. I believe, we are saying the same thing.
A raw access is a request from the calling driver. It is a request from
the driver to directly talk to its hwspinlock driver, without any
encumberance from the framework.

>> while another driver may
>> 'acquire' the lock (since the value written to the lock would be the
>> same as raw api would).
>
>Not sure I understand this one. If a lock has already been assigned to
>a driver, it cannot be re-assigned to another driver.
>
Nevermind, not a good example.

Thanks,
Lina

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Fri, Aug 14, 2015 at 6:24 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Would you rather query the hwspinlock driver to see if the framework
> should take a s/w spinlock or not, IOW, raw-accessible or not?

Sorry, I'm afraid I rather not. This seems to make things even more
complicated without introducing any technical merit.

>> Let's go over your aforementioned concerns:
>>>
>>> But this could yield wrong locking scenarios. If banks are allowed RAW
>>> capability and is not enforced on a per-lock basis, a driver may lock
>>> using non-raw lock using the _raw API
>>
>>
>> If this is allowed by the hardware, then this is a valid scenario.
>> There's no such thing a non-raw lock: a lock is raw if a raw
>> functionality is required.
>>
> Agreed. I believe, we are saying the same thing.
> A raw access is a request from the calling driver. It is a request from
> the driver to directly talk to its hwspinlock driver, without any
> encumberance from the framework.
>
>>> while another driver may
>>> 'acquire' the lock (since the value written to the lock would be the
>>> same as raw api would).
>>
>>
>> Not sure I understand this one. If a lock has already been assigned to
>> a driver, it cannot be re-assigned to another driver.
>>
> Nevermind, not a good example.

Please let me know then if you have any other technical concern that
requires this capability to be maintained separately for every lock.

So far, though, it seems like maintaining this capability separately
per lock is adding complexity, without it solving any technical
problem, that the simpler proposal doesn't solve as well.

If I'm missing anything please let me know,

Thanks,
Ohad.