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/