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/