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
| ||
|
Date: Thu, 26 May 2016 15:28:28 +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> Subject: Re: [PATCH v7 2/4] CMDQ: Mediatek CMDQ driver 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. > > > + > > > + 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
Powered by blists - more mailing lists