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

[Jeffrey Hugo <jhugo@codeaurora.org>]

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