linux-kernel - Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140903163448.GB16352@sonymobile.com>
Date:	Wed, 3 Sep 2014 09:34:55 -0700
From:	Bjorn Andersson <bjorn.andersson@...ymobile.com>
To:	Kumar Gala <galak@...eaurora.org>
CC:	Ohad Ben-Cohen <ohad@...ery.com>, Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Grant Likely <grant.likely@...aro.org>,
	Suman Anna <s-anna@...com>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-arm-msm <linux-arm-msm@...r.kernel.org>,
	Jeffrey Hugo <jhugo@...eaurora.org>,
	Eric Holmberg <eholmber@...eaurora.org>,
	"Cavin, Courtney" <Courtney.Cavin@...ymobile.com>
Subject: Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex
 block

On Wed 03 Sep 08:22 PDT 2014, Kumar Gala wrote:

> 
> On Sep 3, 2014, at 9:55 AM, Bjorn Andersson <Bjorn.Andersson@...ymobile.com> wrote:
> 
> > On Wed 03 Sep 05:49 PDT 2014, Kumar Gala wrote:
> > 
> >> 
> >> On Sep 2, 2014, at 3:04 PM, Bjorn Andersson <Bjorn.Andersson@...ymobile.com> wrote:
> >> 
> >>> Changes since v2:
> >>> - MODULE_DEVICE_TABLE
> >>> - Changed prefix to qcom
> >>> - Cleaned up includes
> >>> - Rely on reg and num-locks to figure out stride, instead of of_match data
> >> 
> >> I know Jeff prefers this method of computing stride, but I’m not a fan as
> >> there isn’t a reason one could adjust qcom,num-locks in the dt for some
> >> reason and leave regs alone.
> >> 
> > 
> > All the current platform it's 32 consecutive mutexes with either 4 or 128 byte
> > stride, so encoding it as data either way works fine. The hardware you're
> > trying to describe with your dt is the addresses that spans your mutex
> > registers and how many there are. So from the HW/dts pov I don't see why you
> > would like to do this.
> > 
> > Then looking in the caf code, there is a limit of max 8 mutexes. So apparently
> > there is some sort of usecase, I just don't know what or if it's valid from a
> > dt pov.
> 
> I believe not all the mutexes are meant for the cores running linux.
> However, I think we just expect linux to play nice and not touch anything it
> isn’t using explicitly.
> 

I would expect so too.

One problem I see is that it's not very robust relying on the relationship
between reg and num-locks. I consider this an implementation detail leaking
into the dt binding and it's not described enough currently...

> > Going to that future awesome SoCs where it's still called tcsr-mutex, but with
> > a stride of 4096 bytes makes me wonder; is that really a consecutive 128kb with
> > nothing else in-between that we can ioremap?
> 
> think 64-bit machines with more address space to burn and wanting to separate
> resources to use MMUs for protection.
> 

Makes sense, I just don't have any documentation verifying this.

> > I.e. can we really reuse this driver straight off for that SoC?
> 
> I dont see why not.
> 

As long as the space inbetween is just wasted, there is no issue.

> >>> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> >>> +- compatible:
> >>> +	Usage: required
> >>> +	Value type: <string>
> >>> +	Definition: must be one of:
> >>> +		    "qcom,sfpb-mutex",
> >>> +		    "qcom,tcsr-mutex”
> >> 
> >> I dont get the purpose of having different compatible strings if there is no
> >> difference in the code between them.
> >> 
> > 
> > The semantics are the same, but there are no mutex registers in the tcsr block
> > in e.g 8960, so the name is just missleading. I assume that's why you didn't
> > follow caf and used the compatible "sfpb" in the first place?
> 
> What do you expect the 8960 dt node to look like?  I’m not 100% against ‘sfpb’
> 
> I’m feel like we we should use compat for stride, so we’d end up with something like:
> 
> qcom,sfpb-mutex: stride 4 bytes, base: 0x01200604, reset: 0x01200600
> qcom,tcsr-mutex: stride 128 bytes, base: 0xFD484000, reset: 0xFD485380
> qcom,tcsr-4k-mutex: stride 4k bytes, base: 0x740000, reset: 0x767000
> 

Maybe these are the best names for the 3 hw blocks after all.

The alternative would be either to encode the platform name in the compatible
or adding the stride as a separate property. The first is waste and the second
doesn't describe how hw is connected. So netiher are good alternatives.


I think your suggestion is reasonable and will move the stride back into
compat. It's the most robust solution.

Is the 4k block finalized (don't see it in 8994 docs)? Should I add it to the
driver now as well?

Thanks for your input!

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