From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751052AbdAQH4J (ORCPT ); Tue, 17 Jan 2017 02:56:09 -0500 Received: from smtp06.smtpout.orange.fr ([80.12.242.128]:37018 "EHLO smtp.smtpout.orange.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750888AbdAQHzJ (ORCPT ); Tue, 17 Jan 2017 02:55:09 -0500 X-ME-Helo: belgarion X-ME-Auth: amFyem1pay5yb2JlcnRAb3JhbmdlLmZy X-ME-Date: Tue, 17 Jan 2017 08:54:59 +0100 X-ME-IP: 92.149.56.251 From: Robert Jarzmik To: Jan Kiszka Cc: Mark Brown , linux-spi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Daniel Mack , Haojian Zhuang , linux-kernel@vger.kernel.org, Andy Shevchenko , Mika Westerberg , Jarkko Nikula , Sascha Weisenberger Subject: Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts References: <7b15a0910a3ad861fd32161c72559bafa7b71e29.1484592296.git.jan.kiszka@siemens.com> X-URL: http://belgarath.falguerolles.org/ Date: Tue, 17 Jan 2017 08:54:56 +0100 In-Reply-To: <7b15a0910a3ad861fd32161c72559bafa7b71e29.1484592296.git.jan.kiszka@siemens.com> (Jan Kiszka's message of "Mon, 16 Jan 2017 19:44:55 +0100") Message-ID: <87ziiqdstr.fsf@belgarion.home> User-Agent: Gnus/5.130008 (Ma Gnus v0.8) Emacs/24.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Jan Kiszka writes: > When using the a device with edge-triggered interrupts, such as MSIs, > the interrupt handler has to ensure that there is a point in time during > its execution where all interrupts sources are silent so that a new > event can trigger a new interrupt again. > > This is achieved here by looping over SSSR evaluation. We need to take > into account that SSCR1 may be changed by the transfer handler, thus we > need to redo the mask calculation, at least regarding the volatile > interrupt enable bit (TIE). > > Signed-off-by: Jan Kiszka Hi Jan, > + while (1) { This bit worries me a bit, as this can be either : - hogging the SoC's CPU, endlessly running - or even worse, blocking the CPU for ever The question behind is, should this be done in a top-half, or moved to a irq thread ? > + /* Ignore possible writes if we don't need to write */ > + if (!(sccr1_reg & SSCR1_TIE)) > + mask &= ~SSSR_TFS; > > - if (!drv_data->master->cur_msg) { > - handle_bad_msg(drv_data); > - /* Never fail */ > - return IRQ_HANDLED; > - } > + if (!(status & mask)) > + return ret; > + > + if (!drv_data->master->cur_msg) { > + handle_bad_msg(drv_data); > + /* Never fail */ > + return IRQ_HANDLED; > + } > + > + ret |= drv_data->transfer_handler(drv_data); Mmm that looks weird to me, oring a irqreturn. Imagine that on first iteration the handler returns IRQ_NONE, and on second IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the handler should have exited, especially if the interrupt is shared with another driver. Another thing which is along what Andy already said : it would be better practice to have this loop in the form : do { ... } while (exit_condition_not_met); Just for maintainability, it's better, and it concentrates the test on the "exit_condition_not_met" in one place, which will enable us to review better the algorithm. Cheers. -- Robert