From patchwork Thu Jan 19 15:34:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kiszka X-Patchwork-Id: 9526231 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 62C5D60113 for ; Thu, 19 Jan 2017 15:35:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 54B14284D1 for ; Thu, 19 Jan 2017 15:35:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 480B22853E; Thu, 19 Jan 2017 15:35:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=2.0 tests=BAYES_00,RDNS_NONE autolearn=no version=3.3.1 Received: from bombadil.infradead.org (unknown [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id B3E81284D1 for ; Thu, 19 Jan 2017 15:35:15 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cUEk5-0003CN-Rc; Thu, 19 Jan 2017 15:35:13 +0000 Received: from david.siemens.de ([192.35.17.14]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cUEk0-0001y0-Pz for linux-arm-kernel@lists.infradead.org; Thu, 19 Jan 2017 15:35:11 +0000 Received: from mail3.siemens.de (mail3.siemens.de [139.25.208.14]) by david.siemens.de (8.15.2/8.15.2) with ESMTPS id v0JFYbxk027648 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 19 Jan 2017 16:34:37 +0100 Received: from md1f2u6c.ww002.siemens.net ([139.25.68.37]) by mail3.siemens.de (8.15.2/8.15.2) with ESMTP id v0JFYXbr029790; Thu, 19 Jan 2017 16:34:35 +0100 Subject: Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts To: Mark Brown References: <7b15a0910a3ad861fd32161c72559bafa7b71e29.1484592296.git.jan.kiszka@siemens.com> <87ziiqdstr.fsf@belgarion.home> <4d97e416-4d32-3b9f-0695-de116a4b26bd@siemens.com> <87r340eq28.fsf@belgarion.home> <20170118124645.6ugjwbfeq5vsh2to@sirena.org.uk> From: Jan Kiszka Message-ID: <7e5fb21d-35bd-6ac3-9e6f-cffed656997f@siemens.com> Date: Thu, 19 Jan 2017 16:34:31 +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: <20170118124645.6ugjwbfeq5vsh2to@sirena.org.uk> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170119_073509_158499_3F9CA2D6 X-CRM114-Status: GOOD ( 18.28 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mika Westerberg , linux-kernel@vger.kernel.org, Haojian Zhuang , linux-spi@vger.kernel.org, Jarkko Nikula , linux-arm-kernel@lists.infradead.org, Andy Shevchenko , Robert Jarzmik , Sascha Weisenberger , Daniel Mack Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 2017-01-18 13:46, Mark Brown wrote: > On Wed, Jan 18, 2017 at 10:33:07AM +0100, Jan Kiszka wrote: >> On 2017-01-18 09:21, Robert Jarzmik wrote: > >>>>>> + 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. > >>> I'm not speaking of a broken interrupt source, I'm speaking of a broken code, >>> such as in the handler, or broken status readback, or lack of understanding on >>> the status register which may imply the while(1) to loop forever. > >>>> 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? > >>> The point is if you stay forever in the while(1) loop, you can at least have a >>> print a backtrace (LOCKUP_DETECTOR). > >> I won't consider "debugability" as a good reason to move interrupt >> handlers into threads. There should be real workload that requires >> offloading or specific prioritization. > > It's failure mitigation - you're translating a hard lockup into > something that will potentially allow the system to soldier on which is > likely to be less severe for the user as well as making things easier to > figure out. If we're doing something like this I'd at least have a > limit on how long we allow the interrupt to scream. > OK, OK, if that is the biggest worry, I can change the pattern from loop-based to SCCR1-based, i.e. mask all interrupt sources once per interrupt so that we enforce a falling edge. Fine. But now I'm looking at the driver, wondering who all is fiddling under which conditions with SCCR1. There are a lot of RMW patterns, but I do not see the locking pattern behind that. Are all RMW accesses run only in the interrupt handler context? Unlikely, at least with the dmaengine in the loop. Closing my eyes regarding this potential issue for now, the patch could become as simple as Not efficient /wrt register accesses, but that's apparently not yet a design goal anyway (I stumbled over the SSCR1 locking while considering to introduce a cache for that reg). Jan diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index 0d10090..f9c2329 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -785,6 +785,9 @@ static irqreturn_t ssp_int(int irq, void *dev_id) if (!(status & mask)) return IRQ_NONE; + pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1); + pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg); + if (!drv_data->master->cur_msg) { handle_bad_msg(drv_data); /* Never fail */