From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jassi Brar Subject: Re: [PATCH v3 2/8] mailbox: arm_mhu: add driver for ARM MHU controller Date: Fri, 9 Jan 2015 20:59:20 +0530 Message-ID: References: <1420802369-3840-1-git-send-email-Vincent.Yang@tw.fujitsu.com> <1420802889-4041-1-git-send-email-Vincent.Yang@tw.fujitsu.com> <20150109125102.GL12302@n2100.arm.linux.org.uk> <20150109152402.GQ12302@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20150109152402.GQ12302-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Russell King - ARM Linux Cc: Vincent Yang , Devicetree List , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Arnd Bergmann , Olof Johansson , Rob Herring , =?UTF-8?Q?Pawe=C5=82_Moll?= , Mark Rutland , "ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org" , Kumar Gala , Sudeep Holla , Andy Green , Patch Tracking , Vincent Yang , Tetsuya Nuriya List-Id: devicetree@vger.kernel.org On 9 January 2015 at 20:54, Russell King - ARM Linux wrote: > On Fri, Jan 09, 2015 at 06:49:12PM +0530, Jassi Brar wrote: >> On 9 January 2015 at 18:21, Russell King - ARM Linux >> wrote: >> > On Fri, Jan 09, 2015 at 07:28:09PM +0800, Vincent Yang wrote: >> >> +static irqreturn_t mhu_rx_interrupt(int irq, void *p) >> >> +{ >> >> + struct mbox_chan *chan = p; >> >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >> >> + u32 val; >> >> + >> >> + pr_debug("%s:%d\n", __func__, __LINE__); >> >> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >> >> + mbox_chan_received_data(chan, (void *)val); >> >> + >> >> + writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS); >> >> + >> >> + return IRQ_HANDLED; >> > >> > What if 'val' was zero - is the interrupt still "handled" ? >> > >> This irq shouldn't fire unless RX_STAT register has some non-zero value. > > You claim this interrupt handler using IRQF_SHARED - what if another user > of this interrupt gets stuck? Your handler above will prevent the kernel > recovering as it will think that you are validly processing the stuck > interrupt each time. > > If it isn't shared, then don't use IRQF_SHARED. > > Either way, it is good practice to return IRQ_NONE if there's no work to > be done. > OK, will do. thanks. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: jaswinder.singh@linaro.org (Jassi Brar) Date: Fri, 9 Jan 2015 20:59:20 +0530 Subject: [PATCH v3 2/8] mailbox: arm_mhu: add driver for ARM MHU controller In-Reply-To: <20150109152402.GQ12302@n2100.arm.linux.org.uk> References: <1420802369-3840-1-git-send-email-Vincent.Yang@tw.fujitsu.com> <1420802889-4041-1-git-send-email-Vincent.Yang@tw.fujitsu.com> <20150109125102.GL12302@n2100.arm.linux.org.uk> <20150109152402.GQ12302@n2100.arm.linux.org.uk> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 9 January 2015 at 20:54, Russell King - ARM Linux wrote: > On Fri, Jan 09, 2015 at 06:49:12PM +0530, Jassi Brar wrote: >> On 9 January 2015 at 18:21, Russell King - ARM Linux >> wrote: >> > On Fri, Jan 09, 2015 at 07:28:09PM +0800, Vincent Yang wrote: >> >> +static irqreturn_t mhu_rx_interrupt(int irq, void *p) >> >> +{ >> >> + struct mbox_chan *chan = p; >> >> + struct mhu_link *mlink = (struct mhu_link *)chan->con_priv; >> >> + u32 val; >> >> + >> >> + pr_debug("%s:%d\n", __func__, __LINE__); >> >> + val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS); >> >> + mbox_chan_received_data(chan, (void *)val); >> >> + >> >> + writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS); >> >> + >> >> + return IRQ_HANDLED; >> > >> > What if 'val' was zero - is the interrupt still "handled" ? >> > >> This irq shouldn't fire unless RX_STAT register has some non-zero value. > > You claim this interrupt handler using IRQF_SHARED - what if another user > of this interrupt gets stuck? Your handler above will prevent the kernel > recovering as it will think that you are validly processing the stuck > interrupt each time. > > If it isn't shared, then don't use IRQF_SHARED. > > Either way, it is good practice to return IRQ_NONE if there's no work to > be done. > OK, will do. thanks.