From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=0.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_ADSP_ALL, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 04D99C433E0 for ; Mon, 1 Jun 2020 02:41:26 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id AA59C20772 for ; Mon, 1 Jun 2020 02:41:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="HLQE5L0s"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=amazon.com header.i=@amazon.com header.b="IyLVtv2g" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AA59C20772 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=amazon.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Content-ID:In-Reply-To: References:Message-ID:Date:To:From:Subject:Reply-To:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=BObUt6o76m2ga8vcu/hpdxBVKOMKiVUsr9KBaNFF+jE=; b=HLQE5L0sXWBuTq 8O0kg+SuNBl58Gn9H8wgBgiYh+88yRvPmMcbLs80pzMQflscsdS5+veuQW1tlxSwxmNHUZVvRlFgO pR4XTShm9XLOr4x3+FMgPivWm8xTG175UodCojPD3zLCWMZtdXyND/Zb43c1H2mSW4rrg/GsKUqEZ NDEAg3drf24G0JQmuvuiLxp7NJwL0+Ch7kEsG0p4PTG2YbaL7bjc28twqebH64bwZOTAjR3lM77PA dVVcYG6ECTo0qQ1QI/aR6XnHC4wxWvPD++RBdkB7S82sB5M53ptG9wzv2aVFFN69zEdUpK56M3HJW myN/bbQSd9sxYtd9trjA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jfaO7-0001hh-Ln; Mon, 01 Jun 2020 02:41:19 +0000 Received: from smtp-fw-6002.amazon.com ([52.95.49.90]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jfaO4-0001hE-Iu for linux-arm-kernel@lists.infradead.org; Mon, 01 Jun 2020 02:41:18 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.com; i=@amazon.com; q=dns/txt; s=amazon201209; t=1590979277; x=1622515277; h=from:to:cc:date:message-id:references:in-reply-to: content-id:content-transfer-encoding:mime-version:subject; bh=TltawOQQY80cO1c2Km92kR007GNJdrez0Cda0l2On4I=; b=IyLVtv2grodTMuW51fxIfBHG3DHxpiYTvY6yNB8BS53Hc5yPjtnse9hV AnmMbBxSr2Fq5SAg9Ny6ChOccfPrsMYBiwF8QnItkfuLVGrfUgcrunm0G jqiboRUxHJupArzinq8Fke9Iy1V3/dqTSjPvTn6ySsqIAqSp3XdaH7jMv 4=; IronPort-SDR: QXzEuie2ebNPVhf0YRsfjs3PmNkASVT8hIlc7CcFYobdLQaBDOBuaceiVdRl4JrisoTlMnnqW9 d0DXCAP4KVJA== X-IronPort-AV: E=Sophos;i="5.73,459,1583193600"; d="scan'208";a="33497482" Subject: Re: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq Thread-Topic: [PATCH] irqchip/gic-v3-its: Don't try to move a disabled irq Received: from iad12-co-svc-p1-lb1-vlan3.amazon.com (HELO email-inbound-relay-1e-303d0b0e.us-east-1.amazon.com) ([10.43.8.6]) by smtp-border-fw-out-6002.iad6.amazon.com with ESMTP; 01 Jun 2020 02:40:57 +0000 Received: from EX13MTAUWB001.ant.amazon.com (iad55-ws-svc-p15-lb9-vlan3.iad.amazon.com [10.40.159.166]) by email-inbound-relay-1e-303d0b0e.us-east-1.amazon.com (Postfix) with ESMTPS id 50FE8A07B4; Mon, 1 Jun 2020 02:40:56 +0000 (UTC) Received: from EX13D02UWB003.ant.amazon.com (10.43.161.48) by EX13MTAUWB001.ant.amazon.com (10.43.161.207) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Mon, 1 Jun 2020 02:40:55 +0000 Received: from EX13D21UWB003.ant.amazon.com (10.43.161.212) by EX13D02UWB003.ant.amazon.com (10.43.161.48) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Mon, 1 Jun 2020 02:40:55 +0000 Received: from EX13D21UWB003.ant.amazon.com ([10.43.161.212]) by EX13D21UWB003.ant.amazon.com ([10.43.161.212]) with mapi id 15.00.1497.006; Mon, 1 Jun 2020 02:40:55 +0000 From: "Herrenschmidt, Benjamin" To: "maz@kernel.org" , "Saidi, Ali" Thread-Index: AQHWNbXPXXyA301h2EGDe5vwb83/i6jA2SaAgAEzUYCAAQRAgA== Date: Mon, 1 Jun 2020 02:40:55 +0000 Message-ID: <37e55e71faf76dc3db76d89c20c1bdfff942e380.camel@amazon.com> References: <20200529015501.15771-1-alisaidi@amazon.com> <8c3be990888ecfb7cca9503853dc4aac@kernel.org> <2C4F431F-8140-4C82-B4BD-E51DE618FC08@amazon.com> <20200530174929.7bf6d5d7@why> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.43.162.140] Content-ID: <579C4899EC03054D91EF4ED83B1F1AAC@amazon.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200531_194116_750624_26CBB60F X-CRM114-Status: GOOD ( 22.94 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "jason@lakedaemon.net" , "Machulsky, Zorik" , "linux-kernel@vger.kernel.org" , "Zilberman, Zeev" , "linux-arm-kernel@lists.infradead.org" , "tglx@linutronix.de" , "Woodhouse, David" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, 2020-05-31 at 12:09 +0100, Marc Zyngier wrote: > > > > Not great indeed. But this is not, as far as I can tell, a GIC > > driver problem. > > > > The semantic of activate/deactivate (which maps to started/shutdown > > in the IRQ code) is that the HW resources for a given interrupt are > > only committed when the interrupt is activated. Trying to perform > > actions involving the HW on an interrupt that isn't active cannot be > > guaranteed to take effect. > > > > I'd rather address it in the core code, by preventing set_affinity (and > > potentially others) to take place when the interrupt is not in the > > STARTED state. Userspace would get an error, which is perfectly > > legitimate, and which it already has to deal with it for plenty of > > other > > reasons. So I finally found time to dig a bit in there :) Code has changed a bit since last I looked. But I have memories of the startup code messing around with the affinity, and here it is. In irq_startup() : switch (__irq_startup_managed(desc, aff, force)) { case IRQ_STARTUP_NORMAL: ret = __irq_startup(desc); irq_setup_affinity(desc); break; case IRQ_STARTUP_MANAGED: irq_do_set_affinity(d, aff, false); ret = __irq_startup(desc); break; case IRQ_STARTUP_ABORT: irqd_set_managed_shutdown(d); return 0; So we have two cases here. Normal and managed. In the managed case, we set the affinity before startup. I feel like your patch might break that or am I missing something ? Additionally, your patch would break any userspace program that expects to be able to change the affinity on an interrupt before it's been started. I don't know if such a thing exsits but the fact that we hit that bug makes me think it might. Now most controller drivers (at least that I'm familiar with, which doesn't include GiC at this point) can deal with that just fine. Now there's also another possible issue: Your patch checks irqd_is_started(). Now I always mixup irqd vs irq_state these days so I may be wrong but irq_state_set_started() is only done in __irq_startup which will *not* be called if the interrupt has NOAUTOEN. Is that ok ? Do we intend for affinity setting not to work until the first enable_irq() for such an interrupt ? We could check activated instead of started I suppose. (again provided I didn't mixup two different things between the irqd and the irq_state stuff). For these reasons my gut feeling is we should just fix GIC as Ali wanted to do initially. The basic idea is simply to defer the HW configuration until the interrupt has been started. I don't see why that would be an issue. Have set_affinity just store the mask (and apply whatever other sanity checking it might want to do) until the itnerrupt is started and when started, apply things to HW. I might be missing a reason why it's more complicated than that :) But I do feel a bit uncomfortable with your approach. Cheers, Ben. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel