[PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Lina Iyer]

Some uses of the hwspinlock could be that one entity acquires the lock
and the other entity releases the lock. This allows for a serialized
traversal path from the locking entity to the other.

For example, the cpuidle entry from Linux to the firmware to power down
the core, can be serialized across the context switch by locking the
hwspinlock in Linux and releasing it in the firmware.

Do not force the caller of __hwspin_trylock() to acquire a kernel
spinlock before acquiring the hwspinlock.

Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Suman Anna <s-anna@ti.com>
Cc: Andy Gross <agross@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/hwspinlock/hwspinlock_core.c | 56 ++++++++++++++++++++----------------
 include/linux/hwspinlock.h           |  1 +
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 461a0d7..bdc59f2 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -105,30 +105,34 @@ 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);
+	if (mode != HWLOCK_NOLOCK) {
+		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;
+		/* 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_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) {
+		/* 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);
 
-		return -EBUSY;
+			return -EBUSY;
+		}
 	}
 
 	/*
@@ -247,13 +251,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);
 
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 3343298..219b333 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;
-- 
2.1.4

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Ohad Ben-Cohen]

Hi Lina,

On Fri, May 1, 2015 at 8:07 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Some uses of the hwspinlock could be that one entity acquires the lock
> and the other entity releases the lock. This allows for a serialized
> traversal path from the locking entity to the other.
>
> For example, the cpuidle entry from Linux to the firmware to power down
> the core, can be serialized across the context switch by locking the
> hwspinlock in Linux and releasing it in the firmware.
>
> Do not force the caller of __hwspin_trylock() to acquire a kernel
> spinlock before acquiring the hwspinlock.

Let's discuss whether we really want to expose this functionality
under the same hwspinlock API or not.

In this new mode, unlike previously, users will now be able to sleep
after taking the lock, and others trying to take the lock might poll
the hardware for a long period of time without the ability to sleep
while waiting for the lock. It almost sounds like you were looking for
some hwmutex functionality.

What do you think about this?

Thanks,
Ohad.

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Lina Iyer]

On Sat, May 09 2015 at 03:25 -0600, Ohad Ben-Cohen wrote:
>Hi Lina,
>
>On Fri, May 1, 2015 at 8:07 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Some uses of the hwspinlock could be that one entity acquires the lock
>> and the other entity releases the lock. This allows for a serialized
>> traversal path from the locking entity to the other.
>>
>> For example, the cpuidle entry from Linux to the firmware to power down
>> the core, can be serialized across the context switch by locking the
>> hwspinlock in Linux and releasing it in the firmware.
>>
>> Do not force the caller of __hwspin_trylock() to acquire a kernel
>> spinlock before acquiring the hwspinlock.
>
>Let's discuss whether we really want to expose this functionality
>under the same hwspinlock API or not.
>
>In this new mode, unlike previously, users will now be able to sleep
>after taking the lock, and others trying to take the lock might poll
>the hardware for a long period of time without the ability to sleep
>while waiting for the lock. It almost sounds like you were looking for
>some hwmutex functionality.
>
>What do you think about this?

I agree, that it opens up a possiblity that user may sleep after holding
a hw spinlock.  But really, why should it prevents us from using it as a
hw mutex, if the need is legitimate?

We could make a check that the caller with NO_LOCK option calls only
with irq disabled, if thats required.

Thanks for the review.

-- Lina

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Ohad Ben-Cohen]

On Mon, May 11, 2015 at 5:46 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Sat, May 09 2015 at 03:25 -0600, Ohad Ben-Cohen wrote:
>> On Fri, May 1, 2015 at 8:07 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Let's discuss whether we really want to expose this functionality
>> under the same hwspinlock API or not.
>>
>> In this new mode, unlike previously, users will now be able to sleep
>> after taking the lock, and others trying to take the lock might poll
>> the hardware for a long period of time without the ability to sleep
>> while waiting for the lock. It almost sounds like you were looking for
>> some hwmutex functionality.
>
> I agree, that it opens up a possiblity that user may sleep after holding
> a hw spinlock.  But really, why should it prevents us from using it as a
> hw mutex, if the need is legitimate?

If we want hw mutex functionality, let's discuss how to expose it.
Exposing it using the existing hw spinlock API might not be ideal, as
users might get confused.

Additionally, there are hardware IP locking blocks out there which
encourage users to sleep while waiting for a lock, by providing
interrupt functionality to wake them up when the lock is freed. So if
we choose to add a hw mutex API it might be used by others in the
future too (though this reason alone is not why we would choose to add
it now of course).

API discussions aside, what do you want to happen in your scenario
while the lock is taken? are you OK with other users spinning on the
lock waiting for it to be released? IIUC that might mean processors
spinning for a non-negligible period of time?

Thanks,
Ohad.

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Lina Iyer]

On Sat, May 16 2015 at 03:03 -0600, Ohad Ben-Cohen wrote:
>On Mon, May 11, 2015 at 5:46 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Sat, May 09 2015 at 03:25 -0600, Ohad Ben-Cohen wrote:
>>> On Fri, May 1, 2015 at 8:07 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> Let's discuss whether we really want to expose this functionality
>>> under the same hwspinlock API or not.
>>>
>>> In this new mode, unlike previously, users will now be able to sleep
>>> after taking the lock, and others trying to take the lock might poll
>>> the hardware for a long period of time without the ability to sleep
>>> while waiting for the lock. It almost sounds like you were looking for
>>> some hwmutex functionality.
>>
>> I agree, that it opens up a possiblity that user may sleep after holding
>> a hw spinlock.  But really, why should it prevents us from using it as a
>> hw mutex, if the need is legitimate?
>
>If we want hw mutex functionality, let's discuss how to expose it.
>Exposing it using the existing hw spinlock API might not be ideal, as
>users might get confused.
>
>Additionally, there are hardware IP locking blocks out there which
>encourage users to sleep while waiting for a lock, by providing
>interrupt functionality to wake them up when the lock is freed. So if
>we choose to add a hw mutex API it might be used by others in the
>future too (though this reason alone is not why we would choose to add
>it now of course).
>
Okay, the API seems to want to dictate what kind of flags be specified
for __try_lock(), FLAG_NONE, in my mind, seems to fall into the same
classification. But sure, we can discuss a different form of achieving
the same thing.

Do you have any ideas?

>API discussions aside, what do you want to happen in your scenario
>while the lock is taken? are you OK with other users spinning on the
>lock waiting for it to be released? IIUC that might mean processors
>spinning for a non-negligible period of time?
>
The lock in question is used differently than traditional locks across
processors. This lock helps synchronizes context transition from
non-secure to secure on the same processor.

The usecase, goes like this. In cpuidle, any core can be the last core
to power down. The last man also holds the responsibility of shutting
down shared resources like caches etc. The way the power down of a core
works is, there are some high level decisions made in Linux and these
decisions (like to flush and invalidate caches) etc gets transferred
over to the the secure layer. The secure layer executes the ARM WFI that
powers down the cpu, but uses these decisions passed into to determine
if the cache needs to be invalidated upon wakeup etc.

There is a possible race condition between what Linux thinks is the last
core, vs what secure layer thinks is the last core. Lets say, two cores
c0, c1 are going down. c1 is the second last core to go down from Linux
as such, will not carry information about shared resources when making
the SCM call. c1  made the SCM call, but is stuck handling some FIQs. In
the meanwhile c0, goes idle and since its the last core in Linux,
figures out the state of the shared resources. c0 calls into SCM, and
ends up powering down earlier than c1. Per secure layer, the last core
to go down is c1 and the votes of the shared resources are considered
from that core. Things like cache invalidation without flush may happen
as a result of this inconsistency of last man view point.

The way we have solved it, Linux acquires a hw spinlock for each core,
when calling into SCM and the secure monitor releases the spinlock. At
any given time, only one core can switch the context from Linux to
secure for power down operations. This guarantees the last man is
synchronized between both Linux and secure. Another core may be spinning
waiting for hw mutex, but they all happen serialized. This mutex is held
in an irq disable context in cpuidle.

There may be another processor spining to wait on hw mutex, but there
isnt much to do otherwise, because the only operation at this time while
holding the lock is to call into SCM and that would unlock the mutex.

Thanks,
Lina

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Andy Gross]

On Mon, May 18, 2015 at 09:03:02AM -0600, Lina Iyer wrote:
> On Sat, May 16 2015 at 03:03 -0600, Ohad Ben-Cohen wrote:
> >On Mon, May 11, 2015 at 5:46 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> >>On Sat, May 09 2015 at 03:25 -0600, Ohad Ben-Cohen wrote:
> >>>On Fri, May 1, 2015 at 8:07 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> >>>Let's discuss whether we really want to expose this functionality
> >>>under the same hwspinlock API or not.
> >>>
> >>>In this new mode, unlike previously, users will now be able to sleep
> >>>after taking the lock, and others trying to take the lock might poll
> >>>the hardware for a long period of time without the ability to sleep
> >>>while waiting for the lock. It almost sounds like you were looking for
> >>>some hwmutex functionality.
> >>
> >>I agree, that it opens up a possiblity that user may sleep after holding
> >>a hw spinlock.  But really, why should it prevents us from using it as a
> >>hw mutex, if the need is legitimate?
> >
> >If we want hw mutex functionality, let's discuss how to expose it.
> >Exposing it using the existing hw spinlock API might not be ideal, as
> >users might get confused.
> >
> >Additionally, there are hardware IP locking blocks out there which
> >encourage users to sleep while waiting for a lock, by providing
> >interrupt functionality to wake them up when the lock is freed. So if
> >we choose to add a hw mutex API it might be used by others in the
> >future too (though this reason alone is not why we would choose to add
> >it now of course).
> >
> Okay, the API seems to want to dictate what kind of flags be specified
> for __try_lock(), FLAG_NONE, in my mind, seems to fall into the same
> classification. But sure, we can discuss a different form of achieving
> the same thing.
> 
> Do you have any ideas?

So let's say we had this hwmutex API.  Are you advocating that we separate out
the hardware spinlock into hwmutex and then make calls to acquire/release the
hwmutex in the hwspinlock?  And then we'd use the hwmutex acquire/release when
we don't want the wrapped sw spinlock.

Seems like a lot of trouble when all we want is a behavior change on the use of
the sw spinlock.

> 
> >API discussions aside, what do you want to happen in your scenario
> >while the lock is taken? are you OK with other users spinning on the
> >lock waiting for it to be released? IIUC that might mean processors
> >spinning for a non-negligible period of time?
> >
> The lock in question is used differently than traditional locks across
> processors. This lock helps synchronizes context transition from
> non-secure to secure on the same processor.
> 
> The usecase, goes like this. In cpuidle, any core can be the last core
> to power down. The last man also holds the responsibility of shutting
> down shared resources like caches etc. The way the power down of a core
> works is, there are some high level decisions made in Linux and these
> decisions (like to flush and invalidate caches) etc gets transferred
> over to the the secure layer. The secure layer executes the ARM WFI that
> powers down the cpu, but uses these decisions passed into to determine
> if the cache needs to be invalidated upon wakeup etc.
> 
> There is a possible race condition between what Linux thinks is the last
> core, vs what secure layer thinks is the last core. Lets say, two cores
> c0, c1 are going down. c1 is the second last core to go down from Linux
> as such, will not carry information about shared resources when making
> the SCM call. c1  made the SCM call, but is stuck handling some FIQs. In
> the meanwhile c0, goes idle and since its the last core in Linux,
> figures out the state of the shared resources. c0 calls into SCM, and
> ends up powering down earlier than c1. Per secure layer, the last core
> to go down is c1 and the votes of the shared resources are considered
> from that core. Things like cache invalidation without flush may happen
> as a result of this inconsistency of last man view point.
> 
> The way we have solved it, Linux acquires a hw spinlock for each core,
> when calling into SCM and the secure monitor releases the spinlock. At
> any given time, only one core can switch the context from Linux to
> secure for power down operations. This guarantees the last man is
> synchronized between both Linux and secure. Another core may be spinning
> waiting for hw mutex, but they all happen serialized. This mutex is held
> in an irq disable context in cpuidle.
> 
> There may be another processor spining to wait on hw mutex, but there
> isnt much to do otherwise, because the only operation at this time while
> holding the lock is to call into SCM and that would unlock the mutex.

In this use case you have an asymmetric use of the APIs. lock but no unlock.
And this breaks the sw spinlock usage.

-- 
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] hwspinlock: Don't take software spinlock before hwspinlock

[Lina Iyer]

On Tue, May 19 2015 at 14:13 -0600, Andy Gross wrote:
>On Mon, May 18, 2015 at 09:03:02AM -0600, Lina Iyer wrote:
>> On Sat, May 16 2015 at 03:03 -0600, Ohad Ben-Cohen wrote:
>> >On Mon, May 11, 2015 at 5:46 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> >>On Sat, May 09 2015 at 03:25 -0600, Ohad Ben-Cohen wrote:
>> >>>On Fri, May 1, 2015 at 8:07 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> >>>Let's discuss whether we really want to expose this functionality
>> >>>under the same hwspinlock API or not.
>> >>>
>> >>>In this new mode, unlike previously, users will now be able to sleep
>> >>>after taking the lock, and others trying to take the lock might poll
>> >>>the hardware for a long period of time without the ability to sleep
>> >>>while waiting for the lock. It almost sounds like you were looking for
>> >>>some hwmutex functionality.
>> >>
>> >>I agree, that it opens up a possiblity that user may sleep after holding
>> >>a hw spinlock.  But really, why should it prevents us from using it as a
>> >>hw mutex, if the need is legitimate?
>> >
>> >If we want hw mutex functionality, let's discuss how to expose it.
>> >Exposing it using the existing hw spinlock API might not be ideal, as
>> >users might get confused.
>> >
>> >Additionally, there are hardware IP locking blocks out there which
>> >encourage users to sleep while waiting for a lock, by providing
>> >interrupt functionality to wake them up when the lock is freed. So if
>> >we choose to add a hw mutex API it might be used by others in the
>> >future too (though this reason alone is not why we would choose to add
>> >it now of course).
>> >
>> Okay, the API seems to want to dictate what kind of flags be specified
>> for __try_lock(), FLAG_NONE, in my mind, seems to fall into the same
>> classification. But sure, we can discuss a different form of achieving
>> the same thing.
>>
>> Do you have any ideas?
>
>So let's say we had this hwmutex API.  Are you advocating that we separate out
>the hardware spinlock into hwmutex and then make calls to acquire/release the
>hwmutex in the hwspinlock?  And then we'd use the hwmutex acquire/release when
>we don't want the wrapped sw spinlock.
>
>Seems like a lot of trouble when all we want is a behavior change on the use of
>the sw spinlock.
>
I see the effort needed for that.
I am not advocating any thing else other than a flag to solve the
problem. I was hoping if this wasnt acceptable, somebody else has a
better idea.

>>
>> >API discussions aside, what do you want to happen in your scenario
>> >while the lock is taken? are you OK with other users spinning on the
>> >lock waiting for it to be released? IIUC that might mean processors
>> >spinning for a non-negligible period of time?
>> >
>> The lock in question is used differently than traditional locks across
>> processors. This lock helps synchronizes context transition from
>> non-secure to secure on the same processor.
>>
>> The usecase, goes like this. In cpuidle, any core can be the last core
>> to power down. The last man also holds the responsibility of shutting
>> down shared resources like caches etc. The way the power down of a core
>> works is, there are some high level decisions made in Linux and these
>> decisions (like to flush and invalidate caches) etc gets transferred
>> over to the the secure layer. The secure layer executes the ARM WFI that
>> powers down the cpu, but uses these decisions passed into to determine
>> if the cache needs to be invalidated upon wakeup etc.
>>
>> There is a possible race condition between what Linux thinks is the last
>> core, vs what secure layer thinks is the last core. Lets say, two cores
>> c0, c1 are going down. c1 is the second last core to go down from Linux
>> as such, will not carry information about shared resources when making
>> the SCM call. c1  made the SCM call, but is stuck handling some FIQs. In
>> the meanwhile c0, goes idle and since its the last core in Linux,
>> figures out the state of the shared resources. c0 calls into SCM, and
>> ends up powering down earlier than c1. Per secure layer, the last core
>> to go down is c1 and the votes of the shared resources are considered
>> from that core. Things like cache invalidation without flush may happen
>> as a result of this inconsistency of last man view point.
>>
>> The way we have solved it, Linux acquires a hw spinlock for each core,
>> when calling into SCM and the secure monitor releases the spinlock. At
>> any given time, only one core can switch the context from Linux to
>> secure for power down operations. This guarantees the last man is
>> synchronized between both Linux and secure. Another core may be spinning
>> waiting for hw mutex, but they all happen serialized. This mutex is held
>> in an irq disable context in cpuidle.
>>
>> There may be another processor spining to wait on hw mutex, but there
>> isnt much to do otherwise, because the only operation at this time while
>> holding the lock is to call into SCM and that would unlock the mutex.
>
>In this use case you have an asymmetric use of the APIs. lock but no unlock.
>And this breaks the sw spinlock usage.
>
Thats correct. Linux locks, firmware unlocks. While this is not the ideal
locking scenario, this is the only way to ensure that there are no races
during context switches.

Thanks,
Lina

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Ohad Ben-Cohen]

Hi Lina,

On Mon, May 18, 2015 at 6:03 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> The lock in question is used differently than traditional locks across
> processors. This lock helps synchronizes context transition from
> non-secure to secure on the same processor.
>
> The usecase, goes like this. In cpuidle, any core can be the last core
> to power down. The last man also holds the responsibility of shutting
> down shared resources like caches etc. The way the power down of a core
> works is, there are some high level decisions made in Linux and these
> decisions (like to flush and invalidate caches) etc gets transferred
> over to the the secure layer. The secure layer executes the ARM WFI that
> powers down the cpu, but uses these decisions passed into to determine
> if the cache needs to be invalidated upon wakeup etc.
>
> There is a possible race condition between what Linux thinks is the last
> core, vs what secure layer thinks is the last core. Lets say, two cores
> c0, c1 are going down. c1 is the second last core to go down from Linux
> as such, will not carry information about shared resources when making
> the SCM call. c1  made the SCM call, but is stuck handling some FIQs. In
> the meanwhile c0, goes idle and since its the last core in Linux,
> figures out the state of the shared resources. c0 calls into SCM, and
> ends up powering down earlier than c1. Per secure layer, the last core
> to go down is c1 and the votes of the shared resources are considered
> from that core. Things like cache invalidation without flush may happen
> as a result of this inconsistency of last man view point.
>
> The way we have solved it, Linux acquires a hw spinlock for each core,
> when calling into SCM and the secure monitor releases the spinlock. At
> any given time, only one core can switch the context from Linux to
> secure for power down operations. This guarantees the last man is
> synchronized between both Linux and secure. Another core may be spinning
> waiting for hw mutex, but they all happen serialized. This mutex is held
> in an irq disable context in cpuidle.
>
> There may be another processor spining to wait on hw mutex, but there
> isnt much to do otherwise, because the only operation at this time while
> holding the lock is to call into SCM and that would unlock the mutex.

Just to make sure I understand, is this how your scenario is solved?

- c1 goes down
- c0 goes down, carries information about shared resources
- c1 takes HWLOCK and calls into SCM, stuck handling FIQs
- c0 wants to call into SCM but is waiting spinning on HWLOCK
- c1 completes handling FIQs, goes idle, HWLOCK is released by secure monitor
- c0 takes HWLOCK, calls into SCM, shared resources handled correctly,

HWLOCK in this example is a single shared hwspinlock accessible by c0,
c1 and secure monitor.

Thanks,
Ohad.

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Lina Iyer]

On Sat, May 23 2015 at 01:36 -0600, Ohad Ben-Cohen wrote:
>Hi Lina,
>
>On Mon, May 18, 2015 at 6:03 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> The lock in question is used differently than traditional locks across
>> processors. This lock helps synchronizes context transition from
>> non-secure to secure on the same processor.
>>
>> The usecase, goes like this. In cpuidle, any core can be the last core
>> to power down. The last man also holds the responsibility of shutting
>> down shared resources like caches etc. The way the power down of a core
>> works is, there are some high level decisions made in Linux and these
>> decisions (like to flush and invalidate caches) etc gets transferred
>> over to the the secure layer. The secure layer executes the ARM WFI that
>> powers down the cpu, but uses these decisions passed into to determine
>> if the cache needs to be invalidated upon wakeup etc.
>>
>> There is a possible race condition between what Linux thinks is the last
>> core, vs what secure layer thinks is the last core. Lets say, two cores
>> c0, c1 are going down. c1 is the second last core to go down from Linux
>> as such, will not carry information about shared resources when making
>> the SCM call. c1  made the SCM call, but is stuck handling some FIQs. In
>> the meanwhile c0, goes idle and since its the last core in Linux,
>> figures out the state of the shared resources. c0 calls into SCM, and
>> ends up powering down earlier than c1. Per secure layer, the last core
>> to go down is c1 and the votes of the shared resources are considered
>> from that core. Things like cache invalidation without flush may happen
>> as a result of this inconsistency of last man view point.
>>
>> The way we have solved it, Linux acquires a hw spinlock for each core,
>> when calling into SCM and the secure monitor releases the spinlock. At
>> any given time, only one core can switch the context from Linux to
>> secure for power down operations. This guarantees the last man is
>> synchronized between both Linux and secure. Another core may be spinning
>> waiting for hw mutex, but they all happen serialized. This mutex is held
>> in an irq disable context in cpuidle.
>>
>> There may be another processor spining to wait on hw mutex, but there
>> isnt much to do otherwise, because the only operation at this time while
>> holding the lock is to call into SCM and that would unlock the mutex.
>
>Just to make sure I understand, is this how your scenario is solved?
>
>- c1 goes down
>- c0 goes down, carries information about shared resources
>- c1 takes HWLOCK and calls into SCM, stuck handling FIQs
>- c0 wants to call into SCM but is waiting spinning on HWLOCK
>- c1 completes handling FIQs, goes idle, HWLOCK is released by secure monitor
>- c0 takes HWLOCK, calls into SCM, shared resources handled correctly,
>
>HWLOCK in this example is a single shared hwspinlock accessible by c0,
>c1 and secure monitor.
>
That is correct.

-- Lina

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Ohad Ben-Cohen]

On Tue, May 26, 2015 at 11:36 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Just to make sure I understand, is this how your scenario is solved?
>>
>> - c1 goes down
>> - c0 goes down, carries information about shared resources
>> - c1 takes HWLOCK and calls into SCM, stuck handling FIQs
>> - c0 wants to call into SCM but is waiting spinning on HWLOCK
>> - c1 completes handling FIQs, goes idle, HWLOCK is released by secure monitor
>> - c0 takes HWLOCK, calls into SCM, shared resources handled correctly,
>>
>> HWLOCK in this example is a single shared hwspinlock accessible by c0,
>> c1 and secure monitor.
>>
> That is correct.

Ok, thanks.

If we adopt the proposed approach in your patch, I'm thinking maybe we
should restrict it only to hardware implementations that explicitly
allow it, using some hardware capability flag published by the
hwspinlock driver.

In OMAP, e.g., it is prohibited to spin on this hwlock for a long
period of time, so such a hw cap flag would allow you guys to enable
this behaviour specifically for your driver.

What do you think?

Thanks,
Ohad.

Re: [PATCH RFC] hwspinlock: Don't take software spinlock before hwspinlock

[Jeffrey Hugo]

On 6/4/2015 7:09 PM, Ohad Ben-Cohen wrote:
> On Tue, May 26, 2015 at 11:36 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>> Just to make sure I understand, is this how your scenario is solved?
>>>
>>> - c1 goes down
>>> - c0 goes down, carries information about shared resources
>>> - c1 takes HWLOCK and calls into SCM, stuck handling FIQs
>>> - c0 wants to call into SCM but is waiting spinning on HWLOCK
>>> - c1 completes handling FIQs, goes idle, HWLOCK is released by secure monitor
>>> - c0 takes HWLOCK, calls into SCM, shared resources handled correctly,
>>>
>>> HWLOCK in this example is a single shared hwspinlock accessible by c0,
>>> c1 and secure monitor.
>>>
>> That is correct.
>
> Ok, thanks.
>
> If we adopt the proposed approach in your patch, I'm thinking maybe we
> should restrict it only to hardware implementations that explicitly
> allow it, using some hardware capability flag published by the
> hwspinlock driver.
>
> In OMAP, e.g., it is prohibited to spin on this hwlock for a long
> period of time, so such a hw cap flag would allow you guys to enable
> this behaviour specifically for your driver.
>
> What do you think?

Lina and I talked about this today.

Lina's current approach of adding a flag (HWLOCK_NOLOCK) seems to flow 
with the current framework considering the framework already has several 
flags to control the software spinlock behavior.  The NOLOCK flag is not 
a default option, so current OMAP code is not affected.  If you like, we 
could label the option with comments in the header as an advanced 
option.  Users better be aware of what they are doing when they use it. 
  OMAP code should never use the option (based on what you appear to be 
saying above), and if someone attempts to use it in OMAP code, well 
there is plenty of rope around the kernel to get one into trouble.  This 
doesn't seem to be an exception.  The default behavior that 99% of 
clients will use remains the same simple interface as it is today, but 
"power users" who require more control are given that flexibility, along 
with additional responsibility.

In short, Lina's solution is simple and uses an existing mechanism to 
satisfy the requirement.

Your capability proposal could be made to work, but it seems to 
introduce additional questions.  Can a capability be applied to a 
specific lock?  Can a capability be applied to an entire bank of locks? 
  Can capabilities be changed during the lifetime of the system?  What 
would the API look like?  Since this mechanism appears to require a new 
API, what other things would that API cover?  Is it appropriate to put 
usecase specific logic into a driver that is responsible for managing 
the hardware? IE policy vs mechanism.  From "prototyping" an 
implementation of this in my head, it seems more complicated and heavy 
handed for a very simple need that can be easily handled in another way. 
  As far as I am aware, there is no other need at this point in time for 
such a mechanism, so it seems to be a lot of effort and work, for 
limited use.

In the interest of keeping things simple, Lina and I think Lina's 
current proposal is the preferred way forward at this point in time 
given the information available today.

If you still wish to scope out a capability based alternative, would you 
please provide some details about how you envision it working?  An 
example of the API, how it would be used, future usecases that might be 
covered by it, etc.  That would give us specifics we can discuss and 
weigh the merits of.

Thanks.

-- 
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] hwspinlock: Don't take software spinlock before hwspinlock

[Ohad Ben-Cohen]

On Sat, Jun 6, 2015 at 2:50 AM, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> If you still wish to scope out a capability based alternative, would you
> please provide some details about how you envision it working?  An example
> of the API, how it would be used, future usecases that might be covered by
> it, etc.  That would give us specifics we can discuss and weigh the merits
> of.

How about:
- add a 'hwcaps' member to hwspinlock_device, and define single cap
called HWL_CAP_ALLOW_RAW
- add new 'hwcaps' parameter to hwspin_lock_register
- omap_hwspinlock.c will pass NULL, qcom_hwspinlock will pass HWL_CAP_ALLOW_RAW
- hwspin_lock_register will set hwcaps accordingly
- before a lock is taken in RAW mode, the capabilities are checked
- document everything nicely

Unless I'm missing something, it should take 5 minutes or less. For
reference, feel free to check out mmc_host's caps member and its
usage.

Thanks,
Ohad.