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

From: Andy Gross <agross@codeaurora.org>
To: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Lina Iyer <lina.iyer@linaro.org>,
	Bjorn Andersson <bjorn.andersson@sonymobile.com>,
	Jeffrey Hugo <jhugo@codeaurora.org>,
	"linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 0/2] hwspinlock: Introduce raw capability for hwspinlock_device
Date: Thu, 13 Aug 2015 10:25:08 -0500	[thread overview]
Message-ID: <20150813152508.GA4627@qualcomm.com> (raw)
In-Reply-To: <CAK=WgbatEexJNMipJpE7z5MkMO99dat=L=jH6uKMg+eR3x6CwA@mail.gmail.com>

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