Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1206540 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device Ohad Ben-Cohen <ohad@wizery.com> - 2015-08-13 08:40 +0200
    Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for  hwspinlock_device Andy Gross <agross@codeaurora.org> - 2015-08-13 17:30 +0200
      Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device Ohad Ben-Cohen <ohad@wizery.com> - 2015-08-14 13:00 +0200
        Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for  hwspinlock_device Lina Iyer <lina.iyer@linaro.org> - 2015-08-14 16:00 +0200
    Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for  hwspinlock_device Lina Iyer <lina.iyer@linaro.org> - 2015-08-14 17:30 +0200

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

On Wed, Jul 29, 2015 at 12:51 AM, Lina Iyer <lina.iyer@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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.iyer@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207490

On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross <agross@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Fri, Aug 14 2015 at 04:52 -0600, Ohad Ben-Cohen wrote:
>On Thu, Aug 13, 2015 at 6:25 PM, Andy Gross <agross@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.iyer@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web