From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751213AbdAQIHQ (ORCPT ); Tue, 17 Jan 2017 03:07:16 -0500 Received: from david.siemens.de ([192.35.17.14]:37256 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750905AbdAQIHL (ORCPT ); Tue, 17 Jan 2017 03:07:11 -0500 Subject: Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts To: Robert Jarzmik References: <7b15a0910a3ad861fd32161c72559bafa7b71e29.1484592296.git.jan.kiszka@siemens.com> <87ziiqdstr.fsf@belgarion.home> 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 From: Jan Kiszka Message-ID: <4d97e416-4d32-3b9f-0695-de116a4b26bd@siemens.com> Date: Tue, 17 Jan 2017 09:05:14 +0100 User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 In-Reply-To: <87ziiqdstr.fsf@belgarion.home> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017-01-17 08:54, Robert Jarzmik wrote: > 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 ? Every device with a broken interrupt source can hog CPUs, nothing special with this one. If you don't close the loop in the handler itself, you close it over the hardware retriggering the interrupt over and over again. So, I don't see a point in offloading to a thread. The normal case is some TX done (FIFO available) event followed by an RX event, then the transfer is complete, isn't it? > >> + /* 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. Not really an uncommon pattern, though. > > 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. That would be a bug in transfer_handler, because we don't enter it without a reason (status != 0). > > 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); This implies code duplication in order to calculate the condition (mask...). I can do this if desired, I wouldn't do this to my own code, though. Jan > > 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. > -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux