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 734FECD342A for ; Tue, 3 Sep 2024 07:56:29 +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:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=igBgchUo2vyXbADov+gENWjdwyJXCwLlZ3FYaV7kMJs=; b=s0j4PC5XUn46n6x2txncykR8wc SjrD45TmR+PsL/muKWvria6iU5tjRcD1GP0xj7E5qaoTs2POHVEKyh+sqM9clSI1bAP76yJIln5e4 LzTDOL+e69QpDFRZsDUv2O2/7XP3suNITz0AoL0Cr+sNVyW/lZO4Yb4rhS/BoaHseLdD7hTtQ7mQD MCJFJcrjgLPRALR5xtiZ81xaJHS+7mQCpSI+tKnH28Xw3ScG5DC1Eq0oUXlMxJ2Y73dLCgUmEE0nZ 0dqHp9my3A5WYztOVI/bFQ3ttkCNReQZOQIf/rBSKPTColOS4qHUgJ4QrTIIJbztaUqlB21o8eDRa Vahmtg2Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slOOi-0000000Gmrq-3boY; Tue, 03 Sep 2024 07:56:20 +0000 Received: from mail-pj1-x1030.google.com ([2607:f8b0:4864:20::1030]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1slONo-0000000GmZv-0nCv for linux-arm-kernel@lists.infradead.org; Tue, 03 Sep 2024 07:55:25 +0000 Received: by mail-pj1-x1030.google.com with SMTP id 98e67ed59e1d1-2d1daa2577bso3469026a91.2 for ; Tue, 03 Sep 2024 00:55:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1725350123; x=1725954923; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=igBgchUo2vyXbADov+gENWjdwyJXCwLlZ3FYaV7kMJs=; b=c7u9HvHQPYwnNTFV5rLcRsrrerdoDkFmyhVgJ2Hdy5+K6sD0cXiY9DzUovcASyA/l+ 0GtSzxlu/WBQQkqgz7T/1MSHi9THnB3AxXTat8pa8tTKeGaRwpE+WAn5bfVkeVxtXxUU RzXsttjpdhreil22jTr8iN0nyvz7Ldd4AqigMQkPp/RdHZNmffifB4K/Sips83VHD4Mc Gw1XeQVRX+NsBJkVlJc8D6/YPzC2+FRTy7wlpBj+z82FdTYxkS54X3T3TWv5nOLFS2uD azIBm5G0VW4Zmpw6lYMj2oJ4sf+3/M7H6PtxfsACsthNb5ect+B59ZvNkaDoPr2i0tTm 9ZEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725350123; x=1725954923; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=igBgchUo2vyXbADov+gENWjdwyJXCwLlZ3FYaV7kMJs=; b=f0rLj6yNhxd5kWumbijhNvHIX5epM90hzGHmCosY3sLkvRHr2BV1t0vvpYkysz+h3N 529VhKlxDX2fIRzaNFCoaT1hP0M/g9AUDDb+lW9BSlSlQPGvqIp/UPKaQYDe+z7pWZeK G7BHcjaeT+E9KQjMjcrx4TTwEmvt0S8RFDdbhxsZpe2jN+06DxdvJpPYgkofRVRxOyYC KkceSyOGglsfxjOKyvmFno+qP8xdw24KF+LfU4z5w86P6X0ZhV6jPYuhwDASEpYnyG6T Y5xieD0PkHne1lna3b0zmv6OvaQY4pejMJC85vTHRaO1yhwBmOQUBHTWPWFFnxww4ymq i4wg== X-Forwarded-Encrypted: i=1; AJvYcCW8COZ9Dyt0fuN/idx2nCaj4Glb5tZFU3RUqqdudXyv6DkBzRtJ2+1L2T4BenVuBEOyBFSrebB7pr/yWzpqpnlv@lists.infradead.org X-Gm-Message-State: AOJu0YzDvwJSckLpYy+hU6jL4pU26H2XDdzoTOzcNiTNBjFNYAScw3kN qbBTuxEtgZnZ/PWjL7ygLmaQriTVWsoCw6nPR6ecXxvhi8IPl6ICjReFGw== X-Google-Smtp-Source: AGHT+IEOODZ2+fsB4siQlM6mhBxUnPAFrTjs9gHzR6Bwmi8h/FnHCIbdSvuRzIJSel2zF1VVfGnE8A== X-Received: by 2002:a17:90b:3e83:b0:2d3:bd5c:4ac8 with SMTP id 98e67ed59e1d1-2d856383ff8mr18663084a91.27.1725350122463; Tue, 03 Sep 2024 00:55:22 -0700 (PDT) Received: from den-build ([116.228.68.226]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2d8f5ec4bc6sm2591066a91.57.2024.09.03.00.55.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Sep 2024 00:55:21 -0700 (PDT) Date: Tue, 3 Sep 2024 15:55:16 +0800 From: Richard Clark To: Thomas Gleixner Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, torvalds@linux-foundation.org Subject: Re: [PATCH] irq: fix the interrupt trigger type override issue Message-ID: References: <87r0a27blv.ffs@tglx> <877cbu7596.ffs@tglx> <87y14a5dcq.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87y14a5dcq.ffs@tglx> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240903_005524_263074_EC99C8A2 X-CRM114-Status: GOOD ( 58.45 ) 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 Hi Thomas, On Mon, Sep 02, 2024 at 04:39:33PM +0200, Thomas Gleixner wrote: > Richard! > > On Mon, Sep 02 2024 at 20:50, richard clark wrote: > > On Mon, Sep 2, 2024 at 5:51 PM 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. > I checked the code and found that it's not the kernel to create the mapping, it's by the driver calling platform_get_irq(...)/of_irq_get(...) to create. > > >> > 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 = DRV); > set_trigger_type(DRV); > > 3) > free_irq(); > // type is not reset to FW > > 4) > map() > irq_trigger_type() != NONE && != FW > -> fail > > This results in a pile of questions: > > Why does #2 override the firmware, if the firmware defines a trigger > type != 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. > Ah, the mapping is created from of_irq_get(...) by driver, the kernel also provides the mapping teardown interface - irq_dispose_mapping. IMO, the right way for the driver is: 1) driver calls of_irq_get() to get the irq and create the mapping 2) driver *should* call irq_dispose_mapping() as the teardown of step 1. 3) free_irq is the teardown of the request_irq to free the irq and its action. So the original issue should be the bug of the driver not calling the irq_dispose_mapping to release the mapping(and being persistent there). The error message will not show if irq_dispose_mapping(...) called by the driver. Looks like the current implementation is correct, but I don't know if it's true for the shared irq... Thanks! > > Thanks, > > tglx > --- > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -895,32 +895,14 @@ unsigned int irq_create_fwspec_mapping(s > */ > virq = 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 == IRQ_TYPE_NONE || type == 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) == IRQ_TYPE_NONE) { > + /* Preserve the trigger type set by request_irq() */ > + if (type != IRQ_TYPE_NONE && irq_get_trigger_type(virq) == IRQ_TYPE_NONE) { > irq_data = irq_get_irq_data(virq); > - if (!irq_data) { > + if (irq_data) > + irqd_set_trigger_type(irq_data, type); > + else > virq = 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 = 0; > goto out; > } > > >