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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 41CFECA0ED3 for ; Mon, 2 Sep 2024 14:40:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:Message-ID:Date:References:In-Reply-To:Subject:Cc: To:From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Ysn9klxB1AuJGKxyFma9j9cJpHGmLYYCryX+zpCeJjo=; b=lsGc/IcYQqtHhJ4e/y1Mh57G/m h4Ohlr/ldnwVOkx+4eep4d6xMZZLGWyFfIXCJCQVrNduXR3j3Vfb65c8ZmJIsS5N4d9Mo4ITvHHFb ddSl4ZLLUIMfez3VhcgEs4whKqDlysWrG7FnbXyipWe94afkT7/pRlG2ePVt3qnPMvha9MDtOWwKc cG8oMZkgGRUpN/XjhfraFurjzLVJxvUV4FLH9KReVdsWQpQywabbnnEV3dmmAjrdYxbQM4tluc3Cz Ifqy5Q3aoOPA1LZW3LHcPLd8cWi0Sf2HZyTjVRvJQ5xRFM0OfHHI8FtStbHS41/DqC7t/wwzq0ywO F9yPwaiQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sl8EP-0000000Ei21-1Tua; Mon, 02 Sep 2024 14:40:37 +0000 Received: from galois.linutronix.de ([2a0a:51c0:0:12e:550::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sl8DQ-0000000Ehrm-1wq3 for linux-arm-kernel@lists.infradead.org; Mon, 02 Sep 2024 14:39:41 +0000 From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1725287973; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ysn9klxB1AuJGKxyFma9j9cJpHGmLYYCryX+zpCeJjo=; b=rn/YDrpEf3aCWr5EKE9j+Z7JmquNw7H4Mo+BJIwwZ62YF8r2tPVN1lFCN4veKPn58MQazD YbeCcVVogyIsxDqelcbK+eMz8ZV65oqZa7i8uc1MbhAMDyM8RbAE9IK6znNqUrfom9tk/F c8MIjhP/B38vmxP/0VV8qQZXc2TNtgGViKLcqJVvPX0Zfny9NG5IPlcECtslsKpLE4k0A5 3oErctjBm7zJP1E6kBfNB4ZmZ50iTd3soNHfqk4YJKSLF9S9Jvg3rbDf8wodxot987/Rtb izXkIuMPu8t4o8P+9NSkwQ8VwE5VEmqmSSmMQYb3O1EtLJ5mVCJ8gHDvMs6osA== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1725287973; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ysn9klxB1AuJGKxyFma9j9cJpHGmLYYCryX+zpCeJjo=; b=3psHJuxLZde1dVw3bPGBTUUl/sPqIoIjC4s2RIqYe65HbnMBC+TuXkqWlfvohbc9HIED7k gzch2pz+qznvzEDw== To: richard clark Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, torvalds@linux-foundation.org, richard clark Subject: Re: [PATCH] irq: fix the interrupt trigger type override issue In-Reply-To: References: <87r0a27blv.ffs@tglx> <877cbu7596.ffs@tglx> Date: Mon, 02 Sep 2024 16:39:33 +0200 Message-ID: <87y14a5dcq.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240902_073936_993007_F4445D7B X-CRM114-Status: GOOD ( 35.53 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Richard! On Mon, Sep 02 2024 at 20:50, richard clark wrote: > On Mon, Sep 2, 2024 at 5:51=E2=80=AFPM Thomas Gleixner wrote: >> >> 2) rmmod() >> >> tears down mapping >> >> >> > This just tears down the action allocated and installed by >> > request_irq(...), but does not teardown the irq's node inserted in the >> > revmap_tree. >> >> So what creates the mapping? If the driver creates it then why doesn't >> it unmap it when it exits? >> > Kernel allocates an irq and creates the mapping during its > initialization when parsing the device's interrupt firmware, not the > driver does that. So the mapping and the interrupt allocation persist even if nothing uses them. What a waste. >> > The logic is if the trigger type specified by request_irq(...) is not >> > consistent with the firmware one, the request_irq will override the >> > FW. We need to keep this logic the same as when we insmod the same >> > kmod next time -- override the FW's too instead of returning a >> > mismatch type error. >> >> I can see how that can happen, but what's missing is the information why >> this mapping persists and why it's tried to be set up again. >> > As I mentioned, it doesn't try to set up again. It will just lookup > the mapping from the tree for the persistent reason when driver try to > request the irq... Fair enough. Though the logic in that map code is convoluted as hell and making it more convoluted does not really make it better. So let's look at how this works (or not): 1) map() allocate_irq(); set_trigger_type(FW); 2) request_irq(type =3D DRV); set_trigger_type(DRV); 3) free_irq(); // type is not reset to FW 4) map() irq_trigger_type() !=3D NONE && !=3D FW -> fail This results in a pile of questions: Why does #2 override the firmware, if the firmware defines a trigger type !=3D NONE? Isn't the whole point of firmware defining the type that the driver does not need to care? If the firmware cannot does not know what the right thing is then it should say so by using type NONE and the type is using the hardware or interrupt driver default. That aside. What you are trying to do only works when #2 and #4 are coming from the exactly same context. What it does not catch is when the interrupt line is shared and there are two drivers where the first one fiddles with type on request_irq() and the second one relies on the firmware to do the right thing. Same problem if you unload the driver and remove the type from request_irq() flags because you figured out that this was bogus. Then you end up with the old setting when you load the recompiled driver again. That's all inconsistent. The proper solution would be to restore the firmware setting in free_irq() when the last action goes away. The question is whether it's worth the trouble. If not then we can just make all of this simple, trivial and incomplete instead of making it more complex and differently incomplete. Thanks, tglx --- --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -895,32 +895,14 @@ unsigned int irq_create_fwspec_mapping(s */ virq =3D irq_find_mapping(domain, hwirq); if (virq) { - /* - * If the trigger type is not specified or matches the - * current trigger type then we are done so return the - * interrupt number. - */ - if (type =3D=3D IRQ_TYPE_NONE || type =3D=3D irq_get_trigger_type(virq)) - goto out; - - /* - * If the trigger type has not been set yet, then set - * it now and return the interrupt number. - */ - if (irq_get_trigger_type(virq) =3D=3D IRQ_TYPE_NONE) { + /* Preserve the trigger type set by request_irq() */ + if (type !=3D IRQ_TYPE_NONE && irq_get_trigger_type(virq) =3D=3D IRQ_TYP= E_NONE) { irq_data =3D irq_get_irq_data(virq); - if (!irq_data) { + if (irq_data) + irqd_set_trigger_type(irq_data, type); + else virq =3D 0; - goto out; - } - - irqd_set_trigger_type(irq_data, type); - goto out; } - - pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n", - hwirq, of_node_full_name(to_of_node(fwspec->fwnode))); - virq =3D 0; goto out; } =20 =20=20=20