Hi Bjorn,


On 08/08/2019 5:37 PM, Bjorn Andersson wrote:

On Thu 08 Aug 05:52 PDT 2019, Fabien DESSENNE wrote:

quoted

On 07/08/2019 6:19 PM, Suman Anna wrote:

quoted

Hi Fabien,

On 8/7/19 3:39 AM, Fabien DESSENNE wrote:

quoted

Hi

On 06/08/2019 11:30 PM, Suman Anna wrote:

quoted

On 8/6/19 1:21 PM, Bjorn Andersson wrote:

quoted

On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:

quoted

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?

quoted

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.

quoted

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...

quoted

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

quoted

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

quoted

regards
Suman

quoted

quoted

quoted

quoted

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.

quoted

quoted

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/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel