linux-kernel - Re: [PATCH v8 2/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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 23 Jun 2016 19:44:11 +0800
From:	CK Hu <ck.hu@...iatek.com>
To:	Horng-Shyang Liao <hs.liao@...iatek.com>
CC:	Rob Herring <robh+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Daniel Kurtz <djkurtz@...omium.org>,
	"Sascha Hauer" <s.hauer@...gutronix.de>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-mediatek@...ts.infradead.org>,
	<srv_heupstream@...iatek.com>,
	"Sascha Hauer" <kernel@...gutronix.de>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Nicolas Boichat <drinkcat@...omium.org>,
	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>,
	Jiaguang Zhang <jiaguang.zhang@...iatek.com>,
	Dennis-YC Hsieh <dennis-yc.hsieh@...iatek.com>,
	Monica Wang <monica.wang@...iatek.com>
Subject: Re: [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver

On Thu, 2016-06-23 at 15:54 +0800, Horng-Shyang Liao wrote:
> Hi CK,
> 
> On Thu, 2016-06-23 at 14:03 +0800, CK Hu wrote:
> > Hi, HS:
> > 
> > On Mon, 2016-05-30 at 11:19 +0800, HS Liao wrote:
> > [...]
> > 
> > > +
> > > +/* events for CMDQ and display */
> > > +enum cmdq_event {
> > > +	/* Display start of frame(SOF) events */
> > > +	CMDQ_EVENT_DISP_OVL0_SOF = 11,
> > > +	CMDQ_EVENT_DISP_OVL1_SOF = 12,
> > > +	CMDQ_EVENT_DISP_RDMA0_SOF = 13,
> > > +	CMDQ_EVENT_DISP_RDMA1_SOF = 14,
> > > +	CMDQ_EVENT_DISP_RDMA2_SOF = 15,
> > > +	CMDQ_EVENT_DISP_WDMA0_SOF = 16,
> > > +	CMDQ_EVENT_DISP_WDMA1_SOF = 17,
> > > +	/* Display end of frame(EOF) events */
> > > +	CMDQ_EVENT_DISP_OVL0_EOF = 39,
> > > +	CMDQ_EVENT_DISP_OVL1_EOF = 40,
> > > +	CMDQ_EVENT_DISP_RDMA0_EOF = 41,
> > > +	CMDQ_EVENT_DISP_RDMA1_EOF = 42,
> > > +	CMDQ_EVENT_DISP_RDMA2_EOF = 43,
> > > +	CMDQ_EVENT_DISP_WDMA0_EOF = 44,
> > > +	CMDQ_EVENT_DISP_WDMA1_EOF = 45,
> > > +	/* Mutex end of frame(EOF) events */
> > > +	CMDQ_EVENT_MUTEX0_STREAM_EOF = 53,
> > > +	CMDQ_EVENT_MUTEX1_STREAM_EOF = 54,
> > > +	CMDQ_EVENT_MUTEX2_STREAM_EOF = 55,
> > > +	CMDQ_EVENT_MUTEX3_STREAM_EOF = 56,
> > > +	CMDQ_EVENT_MUTEX4_STREAM_EOF = 57,
> > > +	/* Display underrun events */
> > > +	CMDQ_EVENT_DISP_RDMA0_UNDERRUN = 63,
> > > +	CMDQ_EVENT_DISP_RDMA1_UNDERRUN = 64,
> > > +	CMDQ_EVENT_DISP_RDMA2_UNDERRUN = 65,
> > > +	/* Keep this at the end of HW events */
> > > +	CMDQ_MAX_HW_EVENT_COUNT = 260,
> > > +};
> > 
> > The value of these symbol is just the GCE-HW-defined value. I think it's
> > not appropriate to expose HW-defined value to client. For another SoC
> > GCE HW, these definition may change.
> 
> Agree.
> 
> > One way to solve this problem is to translate symbol to value
> > internally. But these events looks like interrupt and the value may vary
> > by each SoC, to prevent driver modified frequently, it's better to place
> > the value definition in device tree. It may looks like:
> > 
> > mmsys: clock-controller@...00000 {
> > 	compatible = "mediatek,mt8173-mmsys";
> > 	mediatek,gce = <&gce>;
> > 	gce-events = <53 54>;
> 
> However, client still needs to know the HW-defined value by using
> this solution.
> 
> > 	gce-event-names = "MUTEX0_EOF","MUTEX1_EOF";
> > }
> > 
> > For cmdq driver, you just need modify interface from
> > 
> > int cmdq_rec_wfe(struct cmdq_rec *rec, enum cmdq_event event)
> > int cmdq_rec_clear_event(struct cmdq_rec *rec, enum cmdq_event event)
> > 
> > to
> > 
> > int cmdq_rec_wfe(struct cmdq_rec *rec, u32 event)
> > int cmdq_rec_clear_event(struct cmdq_rec *rec, u32 event)
> 
> I think an easy way for this issue is just removing HW-defined values
> from include/soc/mediatek/cmdq.h (but keeping enum), and then mapping
> abstract values into real values in driver.
> The benefit of this solution is that we can keep interface and leave SoC
> specific code into driver.
> What do you think?

Hi, HS:

OK, that's fine.

Regards,
CK

> 
> > Regards,
> > CK
> 
> Thanks,
> HS
> 
>