Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Fri, Aug 14, 2015 at 6:24 PM, Lina Iyer  wrote:
> Would you rather query the hwspinlock driver to see if the framework
> should take a s/w spinlock or not, IOW, raw-accessible or not?

Sorry, I'm afraid I rather not. This seems to make things even more
complicated without introducing any technical merit.

>> Let's go over your aforementioned concerns:
>>>
>>> But this could yield wrong locking scenarios. If banks are allowed RAW
>>> capability and is not enforced on a per-lock basis, a driver may lock
>>> using non-raw lock using the _raw API
>>
>>
>> If this is allowed by the hardware, then this is a valid scenario.
>> There's no such thing a non-raw lock: a lock is raw if a raw
>> functionality is required.
>>
> Agreed. I believe, we are saying the same thing.
> A raw access is a request from the calling driver. It is a request from
> the driver to directly talk to its hwspinlock driver, without any
> encumberance from the framework.
>
>>> while another driver may
>>> 'acquire' the lock (since the value written to the lock would be the
>>> same as raw api would).
>>
>>
>> Not sure I understand this one. If a lock has already been assigned to
>> a driver, it cannot be re-assigned to another driver.
>>
> Nevermind, not a good example.

Please let me know then if you have any other technical concern that
requires this capability to be maintained separately for every lock.

So far, though, it seems like maintaining this capability separately
per lock is adding complexity, without it solving any technical
problem, that the simpler proposal doesn't solve as well.

If I'm missing anything please let me know,

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Fri, Aug 14, 2015 at 6:24 PM, Lina Iyer  wrote:
> Would you rather query the hwspinlock driver to see if the framework
> should take a s/w spinlock or not, IOW, raw-accessible or not?

Sorry, I'm afraid I rather not. This seems to make things even more
complicated without introducing any technical merit.

>> Let's go over your aforementioned concerns:
>>>
>>> But this could yield wrong locking scenarios. If banks are allowed RAW
>>> capability and is not enforced on a per-lock basis, a driver may lock
>>> using non-raw lock using the _raw API
>>
>>
>> If this is allowed by the hardware, then this is a valid scenario.
>> There's no such thing a non-raw lock: a lock is raw if a raw
>> functionality is required.
>>
> Agreed. I believe, we are saying the same thing.
> A raw access is a request from the calling driver. It is a request from
> the driver to directly talk to its hwspinlock driver, without any
> encumberance from the framework.
>
>>> while another driver may
>>> 'acquire' the lock (since the value written to the lock would be the
>>> same as raw api would).
>>
>>
>> Not sure I understand this one. If a lock has already been assigned to
>> a driver, it cannot be re-assigned to another driver.
>>
> Nevermind, not a good example.

Please let me know then if you have any other technical concern that
requires this capability to be maintained separately for every lock.

So far, though, it seems like maintaining this capability separately
per lock is adding complexity, without it solving any technical
problem, that the simpler proposal doesn't solve as well.

If I'm missing anything please let me know,

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Thu, Aug 13 2015 at 00:34 -0600, Ohad Ben-Cohen wrote:

On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer  wrote:

Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.


But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API, while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would). That is why you should have the capability on
hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
capable should be used as RAW only.

QCOM platform hwlock #7 is unique that different CPUs trying to acquire
the lock would write different values and hence would be fine. But, the
same is not true for other locks in the bank.


As far as I understand, there is nothing special about QCOM's hwlock
#7 in terms of hardware. It's exactly the same lock as all the others.

The only difference in hwlock #7 is the way you use it, and that
sounds like a decision the driver should be able to make. It's a
policy, and I'm not sure we should put it in the DT. I'm also not sure
we need this hwlock-specific complexity in the hwspinlock framework.


The way I see it, we made a design assumption that all hwspinlocks would
need a s/w spinlock around it. The lock #7 here challenges that
assumption. The framework imposes a s/w lock and it is only appropriate
that the framework provide an option to overcome that.

The hwspinlock bank is just a way to initalize a set of locks in a SoC.
Nobody needs the bank after that. Drivers access locks individually. It
only seems appropriate that the raw be a property of the lock access.


The driver already makes a decision whether to disable the interrupts
or not and whether to save their state or not. So it can also make a
decision whether to take a sw spinlock at all or not --- if the
hardware allows it. and that if should be encoded in an accessible
vendor specific (not hwlock specific) struct, which is setup by the
underlying vendor specific hwspinlock driver (no DT involved).


Would you rather query the hwspinlock driver to see if the framework
should take a s/w spinlock or not, IOW, raw-accessible or not?
That could work.
Every time we call into the raw access API, the framework could query
the hwspinlock driver and then bail out if the hwlock is not raw
accessible.


Let's go over your aforementioned concerns:

But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API


If this is allowed by the hardware, then this is a valid scenario.
There's no such thing a non-raw lock: a lock is raw if a raw
functionality is required.


Agreed. I believe, we are saying the same thing.
A raw access is a request from the calling driver. It is a request from
the driver to directly talk to its hwspinlock driver, without any
encumberance from the framework.


while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would).


Not sure I understand this one. If a lock has already been assigned to
a driver, it cannot be re-assigned to another driver.


Nevermind, not a good example.

Thanks,
Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Fri, Aug 14 2015 at 04:52 -0600, Ohad Ben-Cohen wrote:

On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross  wrote:

The issue in hardwiring this into the driver itself means forfeiting
extensibility.  So on one side (w/ raw support), we get the ability to deal with
the lock number changing.  On the other side (w/o raw), we'd have to probably
tie this to chip compat to figure out which lock is the 'special' if it ever
changes.


It sounds like the decision "which lock to use" is a separate problem
from "can it go raw".


Absolutely.


If the hardware doesn't prohibit raw mode, then every lock can be used
in raw mode. So you just have to pick one and make sure both sides
know which lock you use --- which is a classic multi-processor
synchronization issue.


It's arbitrary right now.  The remote processor selected a number, not the
processor running Linux.


Is the number hardcoded right now? and you're using
hwspin_lock_request_specific on the Linux side to acquire the lock?


Yes, the number is hardcoded in the SPM power controller driver. It
explicitly requests Lock #7

-- Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross  wrote:
> The issue in hardwiring this into the driver itself means forfeiting
> extensibility.  So on one side (w/ raw support), we get the ability to deal 
> with
> the lock number changing.  On the other side (w/o raw), we'd have to probably
> tie this to chip compat to figure out which lock is the 'special' if it ever
> changes.

It sounds like the decision "which lock to use" is a separate problem
from "can it go raw".

If the hardware doesn't prohibit raw mode, then every lock can be used
in raw mode. So you just have to pick one and make sure both sides
know which lock you use --- which is a classic multi-processor
synchronization issue.

> It's arbitrary right now.  The remote processor selected a number, not the
> processor running Linux.

Is the number hardcoded right now? and you're using
hwspin_lock_request_specific on the Linux side to acquire the lock?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Fri, Aug 14 2015 at 04:52 -0600, Ohad Ben-Cohen wrote:

On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross agr...@codeaurora.org wrote:

The issue in hardwiring this into the driver itself means forfeiting
extensibility.  So on one side (w/ raw support), we get the ability to deal with
the lock number changing.  On the other side (w/o raw), we'd have to probably
tie this to chip compat to figure out which lock is the 'special' if it ever
changes.


It sounds like the decision which lock to use is a separate problem
from can it go raw.


Absolutely.


If the hardware doesn't prohibit raw mode, then every lock can be used
in raw mode. So you just have to pick one and make sure both sides
know which lock you use --- which is a classic multi-processor
synchronization issue.


It's arbitrary right now.  The remote processor selected a number, not the
processor running Linux.


Is the number hardcoded right now? and you're using
hwspin_lock_request_specific on the Linux side to acquire the lock?


Yes, the number is hardcoded in the SPM power controller driver. It
explicitly requests Lock #7

-- Lina
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Thu, Aug 13 2015 at 00:34 -0600, Ohad Ben-Cohen wrote:

On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer lina.i...@linaro.org wrote:

Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.


But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API, while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would). That is why you should have the capability on
hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
capable should be used as RAW only.

QCOM platform hwlock #7 is unique that different CPUs trying to acquire
the lock would write different values and hence would be fine. But, the
same is not true for other locks in the bank.


As far as I understand, there is nothing special about QCOM's hwlock
#7 in terms of hardware. It's exactly the same lock as all the others.

The only difference in hwlock #7 is the way you use it, and that
sounds like a decision the driver should be able to make. It's a
policy, and I'm not sure we should put it in the DT. I'm also not sure
we need this hwlock-specific complexity in the hwspinlock framework.


The way I see it, we made a design assumption that all hwspinlocks would
need a s/w spinlock around it. The lock #7 here challenges that
assumption. The framework imposes a s/w lock and it is only appropriate
that the framework provide an option to overcome that.

The hwspinlock bank is just a way to initalize a set of locks in a SoC.
Nobody needs the bank after that. Drivers access locks individually. It
only seems appropriate that the raw be a property of the lock access.


The driver already makes a decision whether to disable the interrupts
or not and whether to save their state or not. So it can also make a
decision whether to take a sw spinlock at all or not --- if the
hardware allows it. and that if should be encoded in an accessible
vendor specific (not hwlock specific) struct, which is setup by the
underlying vendor specific hwspinlock driver (no DT involved).


Would you rather query the hwspinlock driver to see if the framework
should take a s/w spinlock or not, IOW, raw-accessible or not?
That could work.
Every time we call into the raw access API, the framework could query
the hwspinlock driver and then bail out if the hwlock is not raw
accessible.


Let's go over your aforementioned concerns:

But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API


If this is allowed by the hardware, then this is a valid scenario.
There's no such thing a non-raw lock: a lock is raw if a raw
functionality is required.


Agreed. I believe, we are saying the same thing.
A raw access is a request from the calling driver. It is a request from
the driver to directly talk to its hwspinlock driver, without any
encumberance from the framework.


while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would).


Not sure I understand this one. If a lock has already been assigned to
a driver, it cannot be re-assigned to another driver.


Nevermind, not a good example.

Thanks,
Lina
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross agr...@codeaurora.org wrote:
 The issue in hardwiring this into the driver itself means forfeiting
 extensibility.  So on one side (w/ raw support), we get the ability to deal 
 with
 the lock number changing.  On the other side (w/o raw), we'd have to probably
 tie this to chip compat to figure out which lock is the 'special' if it ever
 changes.

It sounds like the decision which lock to use is a separate problem
from can it go raw.

If the hardware doesn't prohibit raw mode, then every lock can be used
in raw mode. So you just have to pick one and make sure both sides
know which lock you use --- which is a classic multi-processor
synchronization issue.

 It's arbitrary right now.  The remote processor selected a number, not the
 processor running Linux.

Is the number hardcoded right now? and you're using
hwspin_lock_request_specific on the Linux side to acquire the lock?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Andy Gross]

On Thu, Aug 13, 2015 at 09:34:13AM +0300, Ohad Ben-Cohen wrote:
> On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer  wrote:
> >> Let's not make this more complicated than needed, so please add the
> >> hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
> >> could always change this later if it proves to be insufficient.
> >>
> > But this could yield wrong locking scenarios. If banks are allowed RAW
> > capability and is not enforced on a per-lock basis, a driver may lock
> > using non-raw lock using the _raw API, while another driver may
> > 'acquire' the lock (since the value written to the lock would be the
> > same as raw api would). That is why you should have the capability on
> > hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
> > capable should be used as RAW only.
> >
> > QCOM platform hwlock #7 is unique that different CPUs trying to acquire
> > the lock would write different values and hence would be fine. But, the
> > same is not true for other locks in the bank.
> 
> As far as I understand, there is nothing special about QCOM's hwlock
> #7 in terms of hardware. It's exactly the same lock as all the others.
> 
> The only difference in hwlock #7 is the way you use it, and that
> sounds like a decision the driver should be able to make. It's a
> policy, and I'm not sure we should put it in the DT. I'm also not sure
> we need this hwlock-specific complexity in the hwspinlock framework.

The issue in hardwiring this into the driver itself means forfeiting
extensibility.  So on one side (w/ raw support), we get the ability to deal with
the lock number changing.  On the other side (w/o raw), we'd have to probably
tie this to chip compat to figure out which lock is the 'special' if it ever
changes.

> 
> The driver already makes a decision whether to disable the interrupts
> or not and whether to save their state or not. So it can also make a
> decision whether to take a sw spinlock at all or not --- if the
> hardware allows it. and that if should be encoded in an accessible
> vendor specific (not hwlock specific) struct, which is setup by the
> underlying vendor specific hwspinlock driver (no DT involved).

It's arbitrary right now.  The remote processor selected a number, not the
processor running Linux.

> 
> Let's go over your aforementioned concerns:
> > But this could yield wrong locking scenarios. If banks are allowed RAW
> > capability and is not enforced on a per-lock basis, a driver may lock
> > using non-raw lock using the _raw API
> 
> If this is allowed by the hardware, then this is a valid scenario.
> There's no such thing a non-raw lock: a lock is raw if a raw
> functionality is required.
> 
> > while another driver may
> > 'acquire' the lock (since the value written to the lock would be the
> > same as raw api would).
> 
> Not sure I understand this one. If a lock has already been assigned to
> a driver, it cannot be re-assigned to another driver.
> 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer  wrote:
>> Let's not make this more complicated than needed, so please add the
>> hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
>> could always change this later if it proves to be insufficient.
>>
> But this could yield wrong locking scenarios. If banks are allowed RAW
> capability and is not enforced on a per-lock basis, a driver may lock
> using non-raw lock using the _raw API, while another driver may
> 'acquire' the lock (since the value written to the lock would be the
> same as raw api would). That is why you should have the capability on
> hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
> capable should be used as RAW only.
>
> QCOM platform hwlock #7 is unique that different CPUs trying to acquire
> the lock would write different values and hence would be fine. But, the
> same is not true for other locks in the bank.

As far as I understand, there is nothing special about QCOM's hwlock
#7 in terms of hardware. It's exactly the same lock as all the others.

The only difference in hwlock #7 is the way you use it, and that
sounds like a decision the driver should be able to make. It's a
policy, and I'm not sure we should put it in the DT. I'm also not sure
we need this hwlock-specific complexity in the hwspinlock framework.

The driver already makes a decision whether to disable the interrupts
or not and whether to save their state or not. So it can also make a
decision whether to take a sw spinlock at all or not --- if the
hardware allows it. and that if should be encoded in an accessible
vendor specific (not hwlock specific) struct, which is setup by the
underlying vendor specific hwspinlock driver (no DT involved).

Let's go over your aforementioned concerns:
> But this could yield wrong locking scenarios. If banks are allowed RAW
> capability and is not enforced on a per-lock basis, a driver may lock
> using non-raw lock using the _raw API

If this is allowed by the hardware, then this is a valid scenario.
There's no such thing a non-raw lock: a lock is raw if a raw
functionality is required.

> while another driver may
> 'acquire' the lock (since the value written to the lock would be the
> same as raw api would).

Not sure I understand this one. If a lock has already been assigned to
a driver, it cannot be re-assigned to another driver.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer lina.i...@linaro.org wrote:
 Let's not make this more complicated than needed, so please add the
 hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
 could always change this later if it proves to be insufficient.

 But this could yield wrong locking scenarios. If banks are allowed RAW
 capability and is not enforced on a per-lock basis, a driver may lock
 using non-raw lock using the _raw API, while another driver may
 'acquire' the lock (since the value written to the lock would be the
 same as raw api would). That is why you should have the capability on
 hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
 capable should be used as RAW only.

 QCOM platform hwlock #7 is unique that different CPUs trying to acquire
 the lock would write different values and hence would be fine. But, the
 same is not true for other locks in the bank.

As far as I understand, there is nothing special about QCOM's hwlock
#7 in terms of hardware. It's exactly the same lock as all the others.

The only difference in hwlock #7 is the way you use it, and that
sounds like a decision the driver should be able to make. It's a
policy, and I'm not sure we should put it in the DT. I'm also not sure
we need this hwlock-specific complexity in the hwspinlock framework.

The driver already makes a decision whether to disable the interrupts
or not and whether to save their state or not. So it can also make a
decision whether to take a sw spinlock at all or not --- if the
hardware allows it. and that if should be encoded in an accessible
vendor specific (not hwlock specific) struct, which is setup by the
underlying vendor specific hwspinlock driver (no DT involved).

Let's go over your aforementioned concerns:
 But this could yield wrong locking scenarios. If banks are allowed RAW
 capability and is not enforced on a per-lock basis, a driver may lock
 using non-raw lock using the _raw API

If this is allowed by the hardware, then this is a valid scenario.
There's no such thing a non-raw lock: a lock is raw if a raw
functionality is required.

 while another driver may
 'acquire' the lock (since the value written to the lock would be the
 same as raw api would).

Not sure I understand this one. If a lock has already been assigned to
a driver, it cannot be re-assigned to another driver.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Andy Gross]

On Thu, Aug 13, 2015 at 09:34:13AM +0300, Ohad Ben-Cohen wrote:
 On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer lina.i...@linaro.org wrote:
  Let's not make this more complicated than needed, so please add the
  hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
  could always change this later if it proves to be insufficient.
 
  But this could yield wrong locking scenarios. If banks are allowed RAW
  capability and is not enforced on a per-lock basis, a driver may lock
  using non-raw lock using the _raw API, while another driver may
  'acquire' the lock (since the value written to the lock would be the
  same as raw api would). That is why you should have the capability on
  hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
  capable should be used as RAW only.
 
  QCOM platform hwlock #7 is unique that different CPUs trying to acquire
  the lock would write different values and hence would be fine. But, the
  same is not true for other locks in the bank.
 
 As far as I understand, there is nothing special about QCOM's hwlock
 #7 in terms of hardware. It's exactly the same lock as all the others.
 
 The only difference in hwlock #7 is the way you use it, and that
 sounds like a decision the driver should be able to make. It's a
 policy, and I'm not sure we should put it in the DT. I'm also not sure
 we need this hwlock-specific complexity in the hwspinlock framework.

The issue in hardwiring this into the driver itself means forfeiting
extensibility.  So on one side (w/ raw support), we get the ability to deal with
the lock number changing.  On the other side (w/o raw), we'd have to probably
tie this to chip compat to figure out which lock is the 'special' if it ever
changes.

 
 The driver already makes a decision whether to disable the interrupts
 or not and whether to save their state or not. So it can also make a
 decision whether to take a sw spinlock at all or not --- if the
 hardware allows it. and that if should be encoded in an accessible
 vendor specific (not hwlock specific) struct, which is setup by the
 underlying vendor specific hwspinlock driver (no DT involved).

It's arbitrary right now.  The remote processor selected a number, not the
processor running Linux.

 
 Let's go over your aforementioned concerns:
  But this could yield wrong locking scenarios. If banks are allowed RAW
  capability and is not enforced on a per-lock basis, a driver may lock
  using non-raw lock using the _raw API
 
 If this is allowed by the hardware, then this is a valid scenario.
 There's no such thing a non-raw lock: a lock is raw if a raw
 functionality is required.
 
  while another driver may
  'acquire' the lock (since the value written to the lock would be the
  same as raw api would).
 
 Not sure I understand this one. If a lock has already been assigned to
 a driver, it cannot be re-assigned to another driver.
 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Sat, Jul 18 2015 at 05:31 -0600, Ohad Ben-Cohen wrote:

Hi Lina,

On Thu, Jul 2, 2015 at 11:30 PM, Lina Iyer  wrote:

You are right, RAW capability is not lock specific. But we dont want to
impose this on every lock in the bank either.


I'm not sure I'm following your concern here: drivers still need to
explicitly indicate RAW in order for this to kick in, so this lenient
approach is not being imposed on them.


Correct.


Your original patch allowed every driver on all platforms to disable
the sw spinlock mechanism. What I'm merely suggesting is that the
underlying platform-specific driver should first allow this before it
is being used, as some vendors prohibit this completely.


Agreed. Thats why the platform driver specifies the capability in hwcaps
flag.


Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.


But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API, while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would). That is why you should have the capability on
hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
capable should be used as RAW only.

QCOM platform hwlock #7 is unique that different CPUs trying to acquire
the lock would write different values and hence would be fine. But, the
same is not true for other locks in the bank.

Please let me know if this is not clear.

Thanks,
Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Sat, Jul 18 2015 at 05:31 -0600, Ohad Ben-Cohen wrote:

Hi Lina,

On Thu, Jul 2, 2015 at 11:30 PM, Lina Iyer lina.i...@linaro.org wrote:

You are right, RAW capability is not lock specific. But we dont want to
impose this on every lock in the bank either.


I'm not sure I'm following your concern here: drivers still need to
explicitly indicate RAW in order for this to kick in, so this lenient
approach is not being imposed on them.


Correct.


Your original patch allowed every driver on all platforms to disable
the sw spinlock mechanism. What I'm merely suggesting is that the
underlying platform-specific driver should first allow this before it
is being used, as some vendors prohibit this completely.


Agreed. Thats why the platform driver specifies the capability in hwcaps
flag.


Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.


But this could yield wrong locking scenarios. If banks are allowed RAW
capability and is not enforced on a per-lock basis, a driver may lock
using non-raw lock using the _raw API, while another driver may
'acquire' the lock (since the value written to the lock would be the
same as raw api would). That is why you should have the capability on
hwspinlock and not on hwspinlock_device. Locks that are defined are RAW
capable should be used as RAW only.

QCOM platform hwlock #7 is unique that different CPUs trying to acquire
the lock would write different values and hence would be fine. But, the
same is not true for other locks in the bank.

Please let me know if this is not clear.

Thanks,
Lina
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

Hi Lina,

On Thu, Jul 2, 2015 at 11:30 PM, Lina Iyer  wrote:
> You are right, RAW capability is not lock specific. But we dont want to
> impose this on every lock in the bank either.

I'm not sure I'm following your concern here: drivers still need to
explicitly indicate RAW in order for this to kick in, so this lenient
approach is not being imposed on them.

Your original patch allowed every driver on all platforms to disable
the sw spinlock mechanism. What I'm merely suggesting is that the
underlying platform-specific driver should first allow this before it
is being used, as some vendors prohibit this completely.

Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

Hi Lina,

On Thu, Jul 2, 2015 at 11:30 PM, Lina Iyer lina.i...@linaro.org wrote:
 You are right, RAW capability is not lock specific. But we dont want to
 impose this on every lock in the bank either.

I'm not sure I'm following your concern here: drivers still need to
explicitly indicate RAW in order for this to kick in, so this lenient
approach is not being imposed on them.

Your original patch allowed every driver on all platforms to disable
the sw spinlock mechanism. What I'm merely suggesting is that the
underlying platform-specific driver should first allow this before it
is being used, as some vendors prohibit this completely.

Let's not make this more complicated than needed, so please add the
hwcaps member to hwspinlock_device instead of to hwspinlock struct. We
could always change this later if it proves to be insufficient.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Sat, Jun 27 2015 at 05:25 -0600, Ohad Ben-Cohen wrote:

Hi Lina,

On Sat, Jun 27, 2015 at 6:05 AM, Lina Iyer  wrote:

Hi Ohad,

Any comments?


Sorry, I was under the impression the discussion with Bjorn is still open.


I am of the opinion that the platform driver and the framework should
handle this request. This variation is still within the bounds of proper
usage of the hw remote lock. hwspinlock frameowkr imposes a s/w spinlock
around access to every hw remote lock and the current QCOM platform
driver assumes that the value written into the hardware, has to be a
constant.  Both of these are assumptions are the limitations in Linux
and is not a hw remote lock behavior.

I do not agree that the cpuidle driver has to memory map a hwspinlock
region and treat it as a register write, because we dont want to
complicate the hwspinlock platform driver.


Like Bjorn, I'm not so sure too we want to bind a specific lock to the
RAW capability since this is not a lock-specific hardware detail.


You are right, RAW capability is not lock specific. But we dont want to
impose this on every lock in the bank either. Drivers rely on the
framework's s/w spinlock to ensure that different processes in Linux,
trying to lock the same hwspinlock may correctly acquire. The framework
shall guarantee that the hwspinlock is correctly acquired for regular
usecases (where a constant value is written to the h/w t olock). The RAW
capability assumes that the driver acquiring the RAW lock, knows that
the platform will write a unique value to the h/w and therefore the
correctness of locking is assured by the h/w.


As far as I can see, the hardware-specific differences (if any) are at
the vendor level and not at the lock level, therefore it might make
more sense to add the caps member to hwspinlock_device rather than to
the hwspinlock struct (Jeffrey commented about this too).


Jeff's comment is about my commit text pointing to the wrong structure.
I believe he is fine with the implementation. We debated this idea,
before I came up with this patch.

Thanks,
Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

On Sat, Jun 27 2015 at 05:25 -0600, Ohad Ben-Cohen wrote:

Hi Lina,

On Sat, Jun 27, 2015 at 6:05 AM, Lina Iyer lina.i...@linaro.org wrote:

Hi Ohad,

Any comments?


Sorry, I was under the impression the discussion with Bjorn is still open.


I am of the opinion that the platform driver and the framework should
handle this request. This variation is still within the bounds of proper
usage of the hw remote lock. hwspinlock frameowkr imposes a s/w spinlock
around access to every hw remote lock and the current QCOM platform
driver assumes that the value written into the hardware, has to be a
constant.  Both of these are assumptions are the limitations in Linux
and is not a hw remote lock behavior.

I do not agree that the cpuidle driver has to memory map a hwspinlock
region and treat it as a register write, because we dont want to
complicate the hwspinlock platform driver.


Like Bjorn, I'm not so sure too we want to bind a specific lock to the
RAW capability since this is not a lock-specific hardware detail.


You are right, RAW capability is not lock specific. But we dont want to
impose this on every lock in the bank either. Drivers rely on the
framework's s/w spinlock to ensure that different processes in Linux,
trying to lock the same hwspinlock may correctly acquire. The framework
shall guarantee that the hwspinlock is correctly acquired for regular
usecases (where a constant value is written to the h/w t olock). The RAW
capability assumes that the driver acquiring the RAW lock, knows that
the platform will write a unique value to the h/w and therefore the
correctness of locking is assured by the h/w.


As far as I can see, the hardware-specific differences (if any) are at
the vendor level and not at the lock level, therefore it might make
more sense to add the caps member to hwspinlock_device rather than to
the hwspinlock struct (Jeffrey commented about this too).


Jeff's comment is about my commit text pointing to the wrong structure.
I believe he is fine with the implementation. We debated this idea,
before I came up with this patch.

Thanks,
Lina
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

Hi Lina,

On Sat, Jun 27, 2015 at 6:05 AM, Lina Iyer  wrote:
> Hi Ohad,
>
> Any comments?

Sorry, I was under the impression the discussion with Bjorn is still open.

Like Bjorn, I'm not so sure too we want to bind a specific lock to the
RAW capability since this is not a lock-specific hardware detail.

As far as I can see, the hardware-specific differences (if any) are at
the vendor level and not at the lock level, therefore it might make
more sense to add the caps member to hwspinlock_device rather than to
the hwspinlock struct (Jeffrey commented about this too).

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Ohad Ben-Cohen]

Hi Lina,

On Sat, Jun 27, 2015 at 6:05 AM, Lina Iyer lina.i...@linaro.org wrote:
 Hi Ohad,

 Any comments?

Sorry, I was under the impression the discussion with Bjorn is still open.

Like Bjorn, I'm not so sure too we want to bind a specific lock to the
RAW capability since this is not a lock-specific hardware detail.

As far as I can see, the hardware-specific differences (if any) are at
the vendor level and not at the lock level, therefore it might make
more sense to add the caps member to hwspinlock_device rather than to
the hwspinlock struct (Jeffrey commented about this too).

Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

Hi Ohad,

Any comments?

Thanks,
Lina

On Tue, Jun 09 2015 at 10:23 -0600, Lina Iyer wrote:

This patch follows the discussion based on the first RFC series posted on the
mailing list [1]. The discussion resulted in a couple of directives for
hwspinlocks that do not want the framework imposing a s/w spinlock around the
hwspinlock.

i. The default should only be a s/w spinlock around the hwspinlock to ensure
correctness of locking.

ii. Existing users may not use raw capability, unless the platform registers
support for it.

iii. Platform driver for hwspinlock should dictate which locks can be operated
in raw mode.

iv. Platform driver and the hw holds the responsibility to ensure the
correctness of acquiring the hwspinlock.

This patchset implements these directives.

Changes since RFC v1:
- Introduce 'raw' capability for hwspinlocks.
- Platform code now has to explicitly specify the raw capability of a lock.
- Check to ensure that only those locks explicitly marked as raw capable can be
 locked/unlocked through the _raw api
- QCOM patch for making lock #7 raw capable added.
- Add documentation

Thanks,
Lina

[1]. https://patches.linaro.org/47895/

Lina Iyer (2):
 hwspinlock: Introduce raw capability for hwspinlocks
 hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

Documentation/hwspinlock.txt | 16 +++
drivers/hwspinlock/hwspinlock_core.c | 75 +++-
drivers/hwspinlock/hwspinlock_internal.h |  6 +++
drivers/hwspinlock/qcom_hwspinlock.c | 22 +++---
include/linux/hwspinlock.h   | 41 +
5 files changed, 125 insertions(+), 35 deletions(-)

--
2.1.4


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

Hi Ohad,

Any comments?

Thanks,
Lina

On Tue, Jun 09 2015 at 10:23 -0600, Lina Iyer wrote:

This patch follows the discussion based on the first RFC series posted on the
mailing list [1]. The discussion resulted in a couple of directives for
hwspinlocks that do not want the framework imposing a s/w spinlock around the
hwspinlock.

i. The default should only be a s/w spinlock around the hwspinlock to ensure
correctness of locking.

ii. Existing users may not use raw capability, unless the platform registers
support for it.

iii. Platform driver for hwspinlock should dictate which locks can be operated
in raw mode.

iv. Platform driver and the hw holds the responsibility to ensure the
correctness of acquiring the hwspinlock.

This patchset implements these directives.

Changes since RFC v1:
- Introduce 'raw' capability for hwspinlocks.
- Platform code now has to explicitly specify the raw capability of a lock.
- Check to ensure that only those locks explicitly marked as raw capable can be
 locked/unlocked through the _raw api
- QCOM patch for making lock #7 raw capable added.
- Add documentation

Thanks,
Lina

[1]. https://patches.linaro.org/47895/

Lina Iyer (2):
 hwspinlock: Introduce raw capability for hwspinlocks
 hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

Documentation/hwspinlock.txt | 16 +++
drivers/hwspinlock/hwspinlock_core.c | 75 +++-
drivers/hwspinlock/hwspinlock_internal.h |  6 +++
drivers/hwspinlock/qcom_hwspinlock.c | 22 +++---
include/linux/hwspinlock.h   | 41 +
5 files changed, 125 insertions(+), 35 deletions(-)

--
2.1.4


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

This patch follows the discussion based on the first RFC series posted on the
mailing list [1]. The discussion resulted in a couple of directives for
hwspinlocks that do not want the framework imposing a s/w spinlock around the
hwspinlock.

i. The default should only be a s/w spinlock around the hwspinlock to ensure
correctness of locking.

ii. Existing users may not use raw capability, unless the platform registers
support for it.

iii. Platform driver for hwspinlock should dictate which locks can be operated
in raw mode.

iv. Platform driver and the hw holds the responsibility to ensure the
correctness of acquiring the hwspinlock.

This patchset implements these directives.

Changes since RFC v1:
- Introduce 'raw' capability for hwspinlocks.
- Platform code now has to explicitly specify the raw capability of a lock.
- Check to ensure that only those locks explicitly marked as raw capable can be
  locked/unlocked through the _raw api 
- QCOM patch for making lock #7 raw capable added.
- Add documentation

Thanks,
Lina

[1]. https://patches.linaro.org/47895/

Lina Iyer (2):
  hwspinlock: Introduce raw capability for hwspinlocks
  hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

 Documentation/hwspinlock.txt | 16 +++
 drivers/hwspinlock/hwspinlock_core.c | 75 +++-
 drivers/hwspinlock/hwspinlock_internal.h |  6 +++
 drivers/hwspinlock/qcom_hwspinlock.c | 22 +++---
 include/linux/hwspinlock.h   | 41 +
 5 files changed, 125 insertions(+), 35 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device

[Lina Iyer]

This patch follows the discussion based on the first RFC series posted on the
mailing list [1]. The discussion resulted in a couple of directives for
hwspinlocks that do not want the framework imposing a s/w spinlock around the
hwspinlock.

i. The default should only be a s/w spinlock around the hwspinlock to ensure
correctness of locking.

ii. Existing users may not use raw capability, unless the platform registers
support for it.

iii. Platform driver for hwspinlock should dictate which locks can be operated
in raw mode.

iv. Platform driver and the hw holds the responsibility to ensure the
correctness of acquiring the hwspinlock.

This patchset implements these directives.

Changes since RFC v1:
- Introduce 'raw' capability for hwspinlocks.
- Platform code now has to explicitly specify the raw capability of a lock.
- Check to ensure that only those locks explicitly marked as raw capable can be
  locked/unlocked through the _raw api 
- QCOM patch for making lock #7 raw capable added.
- Add documentation

Thanks,
Lina

[1]. https://patches.linaro.org/47895/

Lina Iyer (2):
  hwspinlock: Introduce raw capability for hwspinlocks
  hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

 Documentation/hwspinlock.txt | 16 +++
 drivers/hwspinlock/hwspinlock_core.c | 75 +++-
 drivers/hwspinlock/hwspinlock_internal.h |  6 +++
 drivers/hwspinlock/qcom_hwspinlock.c | 22 +++---
 include/linux/hwspinlock.h   | 41 +
 5 files changed, 125 insertions(+), 35 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/