Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

[Fabien DESSENNE]

Hi Bjorn,


On 08/08/2019 5:37 PM, Bjorn Andersson wrote:
> On Thu 08 Aug 05:52 PDT 2019, Fabien DESSENNE wrote:
>
>> 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:
>>>>>> 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.
>>
> Why can't you use HWLOCK_IRQSTATE in this 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.
> No it's not.
>
>> 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.
>>
> I think it's typical for hwspinlock hardware to not be able to
> distinguish between different clients within Linux, so we would need to


Agree with that, let's forget this idea.


> wrap the usage in some construct that ensures mutual exclusion in Linux
> - like a spinlock...
>
>> 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"
>>
> But having local lock protection in the hwspinlock driver would defeat
> the purpose of HWLOCK_RAW.


My understanding is that the purpose of the RAW mode is to allow the 
user to do some time-consuming or sleepable operations under the 
hardware spinlock protection.

This is probably the reason why the RAW mode does not uses any spinlock 
is used in RAW mode.

But I do not think that this is a requirement to not use any local 
protection.

So, in this mode, instead of using a spinlock, what about calling the 
atomic bitop test_and_set_bit()Â ?

This would ensure safe concurrency between the hwspinlock linux users, 
and will respect the purpose of the RAW mode.

Let me know if this is acceptable.


BR

Fabien


>
> Also this kind of warning will at best be consumed by the client driver
> authors, it will not be read by the dts authors.
>
> Regards,
> Bjorn
>
>> 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/