linux-kernel - Re: [PATCH v7 2/4] 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: <1464327858.8578.6.camel@mtksdaap41>
Date:	Fri, 27 May 2016 13:44:18 +0800
From:	Horng-Shyang Liao <hs.liao@...iatek.com>
To:	CK Hu <ck.hu@...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>
Subject: Re: [PATCH v7 2/4] CMDQ: Mediatek CMDQ driver

Hi CK,

Reply in line.

On Thu, 2016-05-26 at 15:28 +0800, CK Hu wrote:
> Hi, HS:
> 
> Replay inline.
> 
> On Tue, 2016-05-24 at 20:27 +0800, Horng-Shyang Liao wrote:
> > Hi CK,
> > 
> > Reply in line.
> > 
> > On Tue, 2016-05-24 at 11:05 +0800, CK Hu wrote:
> > > Hi, HS:
> > > 
> > > Some comments below.
> > > 
> > ...
> > > > +static void cmdq_thread_irq_handler(struct cmdq *cmdq, int tid)
> > > > +{
> > > > +	struct cmdq_thread *thread = &cmdq->thread[tid];
> > > > +	unsigned long flags = 0L;
> > > > +	int value;
> > > > +
> > > > +	spin_lock_irqsave(&cmdq->exec_lock, flags);
> > > > +
> > > > +	/*
> > > > +	 * it is possible for another CPU core
> > > > +	 * to run "release task" right before we acquire the spin lock
> > > > +	 * and thus reset / disable this HW thread
> > > > +	 * so we check both the IRQ flag and the enable bit of this thread
> > > > +	 */
> > > > +	value = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
> > > > +	if (!(value & CMDQ_THR_IRQ_MASK)) {
> > > > +		spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> > > > +		return;
> > > > +	}
> > > 
> > > If this case happen and just return without clearing irq status, the irq
> > > would keep triggering and system hang up. So just remove this checking
> > > and go down to clear irq status.
> > 
> > This case is safe because irq status is cleared.
> > But, next if condition has the problem which you mentioned.
> > 
> > I will change it as below.
> > 
> > 	if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> > 	      CMDQ_THR_ENABLED)) {
> > 		cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);
> > 		spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> > 		return;
> > 	}
> > 
> > If thread is disabled, tasks must be all finished.
> > Therefore, just clear irq status and return.
> > 
> 
> For irq status checking part, if irq status & irq mask is zero, remove
> this checking and let it go down, it still do nothing because value &
> CMDQ_THR_IRQ_ERROR is zero and value & CMDQ_THR_IRQ_DONE is zero. So you
> can just remove this checking and get the same result.
> 
> In general HW design, once a HW is not enable, it does not trigger
> interrupt any more. Be sure that it's necessary to clear irq status even
> though thread is disable.
> 

I Will remove first if condition,
so rewrite first two if condition parts as below.

value = cmdq_thread_readl(thread, CMDQ_THR_IRQ_STATUS);
cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);

if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
			CMDQ_THR_ENABLED))
	value = 0;

> > > > +
> > > > +	if (!(cmdq_thread_readl(thread, CMDQ_THR_ENABLE_TASK) &
> > > > +	      CMDQ_THR_ENABLED)) {
> > > > +		spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	cmdq_thread_writel(thread, ~value, CMDQ_THR_IRQ_STATUS);
> > > > +
> > > > +	if (value & CMDQ_THR_IRQ_ERROR)
> > > > +		cmdq_handle_error_done(cmdq, thread, true);
> > > > +	else if (value & CMDQ_THR_IRQ_DONE)
> > > > +		cmdq_handle_error_done(cmdq, thread, false);
> > > 
> > > These irq status checking and clearing code here is the same as those in
> > > cmdq_task_handle_error_result(). To move the checking and clearing code
> > > into cmdq_handle_error_done() and here just to call
> > > cmdq_handle_error_done(cmdq, thread) would reduce duplicated code.
> > 
> > Will do.
> > 
> > > > +
> > > > +	spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> > > > +}
> > ...
> > ...
> > > 
> > > Regards,
> > > CK
> > 
> > Thanks,
> > HS
> > 
> 
> Regards,
> CK
> 
> 

Thanks,
HS