linux-kernel - Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver

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] [day] [month] [year] [list]

Message-ID: <CAGS+omBRhYugsiFTzfDG2jtON4rj7Y4aDY-X2afRSh09G-ZC+A@mail.gmail.com>
Date:	Sat, 27 Feb 2016 00:47:24 +0800
From:	Daniel Kurtz <djkurtz@...omium.org>
To:	Horng-Shyang Liao <hs.liao@...iatek.com>
Cc:	Sascha Hauer <s.hauer@...gutronix.de>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@...ts.infradead.org>,
	srv_heupstream <srv_heupstream@...iatek.com>,
	Sascha Hauer <kernel@...gutronix.de>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Nicolas Boichat <drinkcat@...omium.org>,
	CK HU <ck.hu@...iatek.com>,
	cawa cheng <cawa.cheng@...iatek.com>,
	Bibby Hsieh <bibby.hsieh@...iatek.com>,
	YT Shen <yt.shen@...iatek.com>,
	Daoyuan Huang <daoyuan.huang@...iatek.com>,
	Damon Chu <damon.chu@...iatek.com>,
	Josh-YC Liu <josh-yc.liu@...iatek.com>,
	Glory Hung <glory.hung@...iatek.com>,
	Yong Wu <yong.wu@...iatek.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Rob Herring <robh+dt@...nel.org>
Subject: Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver

On Fri, Feb 26, 2016 at 5:00 PM, Horng-Shyang Liao <hs.liao@...iatek.com> wrote:
>
> On Wed, 2016-02-03 at 14:40 +0800, Daniel Kurtz wrote:
> > > Hi Dan,
> > >
> > > Thanks for your comment.
> > > This solution looks good to me.
> > > I will change it as your suggestion.
> > >
> > > But, I have a question about 'mask out the provided *device virtual*
> > > address'.
> > > Are lower 16-bits (or 24-bits for JUMP op) of device virtual address the
> > > same as device physical address?
> >
> > I'm not sure.  But I doubt it we can rely on this.
> > My guess would be that the ioremap only preserves the lower 12 bits
> > (4k page size).
> >
> > > If not, we still need to pass in physical address into CMDQ driver.
> >
> > Or, instead of the iommu/slot approach, we can just provide a
> > registration function for the gce driver.
> > Each gce consumer could then have a simple gce node, with no slot/address:
> >
> >   mediatek,gce = <&gce>;
> >
> > Then on probe, the gce consumer could pass in its (struct device *) to
> > gce_register_device().  gce_register_device() could then access the
> > device's of_node to extract its physical address range, and look up
> > its physical address in its table of per-soc of
> > "device_address:gce_subsys_address" entries.  If the physical address
> > is in a valid subsys ranges, the gce_register_device would cache the
> > subsys address, and an offset in a (struct gce_consumer).
> > gce_register_device() could then add this struct to a struct list_head
> > of gce_consumers, and finally return a pointer to it back to the
> > caller.
> >
> > Later, the gce consumer could pass in ths (struct gce_consumer *) when
> > make gce calls, along with the *offset* (not the physical address or
> > virtual address) for the register that it wishes to access.  Then the
> > gce driver can simply use the gce_consumer->subsys entry to create a
> > gce address from the passed in offset.
> >
> > This will keep the binding very simple, and would remove the need to
> > convert from device virtual to physical addresses by the gce consumer,
> > but require a little more per-gce-consumer setup.
> >
> > -Dan
>
> Hi Dan,
>
> When I try to implement this comment, I realize the only benefit from
> this comment is to wrap physical address.
>
> Recall from my previous reply: gce address = subsys + valid low bits.
> So, CMDQ driver still need to do "(Base + offset) & valid mask" to get
> gce valid low bits.
> Current implementation let display driver do "base + offset".
> This comment just transfers this calculation from display driver to CMDQ
> driver.
>
> However, this comment will let CMDQ interface (behavior) become more
> complicated, e.g. gce_register_device(), struct gce_consumer, and int
> cmdq_rec_write(struct cmdq_rec *handle, u32 value, struct gce_consumer
> *consumer, u32 offset)
>
> Do you think it is worth to do this effort to wrap physical address?

Yes, I think it is worth it to unify the gce address computation and
move it into the gce driver.

-Dan