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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 634B7ECAAD2 for ; Sat, 27 Aug 2022 16:15:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232887AbiH0QPV (ORCPT ); Sat, 27 Aug 2022 12:15:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58790 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229544AbiH0QPT (ORCPT ); Sat, 27 Aug 2022 12:15:19 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3AB6E1FCF3 for ; Sat, 27 Aug 2022 09:15:18 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id C5E2960DEA for ; Sat, 27 Aug 2022 16:15:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2C8A4C433D6; Sat, 27 Aug 2022 16:15:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1661616917; bh=0xZkNc3i4ByCt1UOZ1mNZYwZb1FJx+uL7tVmiJN19Vk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=kjtuCJy+gaofiF3Se1ZPYmhmX2rJWe5r1N1zCNgLsoXbNnsjjkYBJ1CJTlt5/mO2I MxXJesSSsBG1qF3q+QHEixqMBCpdOwiRrE7VQ3wynCih++qFGESkZeVYV28C42ffbl CYe0I/KmCVdYn+kVyiY8RDvlwGf8ave88F2wRKz4e87/dWVF3lLXw1Ome6bFde/i5H EBTCjJklg7F/ups+7pu/6lh0KOVLfLl+PS+S+SB9arfwSfkZePj4fVpXNSjGWT/LTv zuyKQCI3MeBngL09Xoe8S0yieR/getHmGYehtlxBWsrtUVVdXyqwNYHhciiyBpmig0 EvZIhgv0X6Frg== Received: from [12.191.126.171] (helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oRySo-006BZG-Jk; Sat, 27 Aug 2022 17:15:15 +0100 Date: Sat, 27 Aug 2022 17:15:09 +0100 Message-ID: <87sflhr7ci.wl-maz@kernel.org> From: Marc Zyngier To: Liao Chang Cc: , , , , , , , Subject: Re: [PATCH 1/2] genirq: Record dangling hwirq number into struct irq_data In-Reply-To: <20220825060819.74303-1-liaochang1@huawei.com> References: <20220825060819.74303-1-liaochang1@huawei.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 12.191.126.171 X-SA-Exim-Rcpt-To: liaochang1@huawei.com, tglx@linutronix.de, samuel@sholland.org, brgl@bgdev.pl, andy.shevchenko@gmail.com, mikelley@microsoft.com, lvjianmin@loongson.cn, mark.rutland@arm.com, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 25 Aug 2022 07:08:18 +0100, Liao Chang wrote: > > Following interrupt allocation process lead to some interrupts are > mapped in the low-level domain(Arm ITS), but they are never been mapped > at the higher level. > > irq_domain_alloc_irqs_hierarchy(.., nr_irqs, ...) > its_irq_domain_alloc(..., nr_irqs, ...) > its_alloc_device_irq(..., nr_irqs, ...) > bitmap_find_free_region(..., get_count_order(nr_irqs)) > > Since ITS domain find a region of zero bits, the length of which must > aligned to power of two. If nr_irqs is 30, the length of zero bits is > actually 32, but only first 30 bits are really mapped. > > On teardown, low-level domain only free these interrupts that actually > mapped, and leave last interrupts dangling in the ITS domain. Thus the > ITS device resources are never freed. On device driver reload, dangling > interrupts prevent ITS domain from allocating enough resource. > > irq_domain_free_irqs_hierarchy(..., nr_irqs, ...) > its_irq_domain_free(..., irq_base + i, 1) > bitmap_release_region(..., irq_base + i, get_count_order(1)) > > John reported this problem to LKML and Marc provided a solution and fix > it in the generic code, see the discussion from Link tag. Marc's patch > fix John's problem, but does not take care of some corner case, look one > example below. > > Step1: 32 interrupts allocated in LPI domain, but return the first 30 to > higher driver. > > 111111111111111111111111111111 11 > |<------------0~29------------>|30,31| > > Step2: interrupt #16~28 are released one by one, then #0~15 and #29~31 > still be there. > > 1111111111111111 0000000000000 1 11 > |<-----0~15----->|<---16~28--->|29|30,31| > > Step#: on driver teardown, generic code will invoke ITS domain code > twice. The first time, #0~15 will be released, the second one, only #29 > will be released(1 align to power of two). > > 0000000000000000 0000000000000 0 11 > |<-----0~15----->|<---16~28--->|29|30,31| > > In short summary, the dangling problem stems from the number of released > hwirq is less than the one of allocated hwirq in ITS domain. In order to > fix this problem, make irq_data record the number of allocated but > unmapped hwirq. If hwirq followed by some unmapped bits, ITS domain > record the number of unmapped bits to the last irq_data mapped to higher > level, when the last hwirq followed by unmapped hwirq is released, some > dangling bit will be clear eventualy, look back the trivial example > above. > > Step1: record '2' into the irq_data.dangling of #29 hwirq. > > 111111111111111111111111111111 11 > |<------------0~29------------>|30,31| > dangling: 000000000000000000000000000002 > > Step2: no change > > 1111111111111111 0000000000000 1 11 > |<-----0~15----->|<---16~28--->|29|30,31| > dangling: 0000000000000000 0000000000000 2 > > Step3: ITS domain will release #30~31 since the irq_data.dangling of #29 > is '2'. > > 0000000000000000 0000000000000 0 00 > |<-----0~15----->|<---16~28--->|29|30,31| > dangling: 0000000000000000 0000000000000 2 > > Fixes: 4615fbc3788dd ("genirq/irqdomain: Don't try to free an interrupt > that has no mapping") > Reported-by: John Garry > Signed-off-by: Liao Chang > Link: https://lore.kernel.org/lkml/3d3d0155e66429968cb4f6b4feeae4b3@kernel.org/ > --- > include/linux/irq.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/irq.h b/include/linux/irq.h > index c3eb89606c2b..c48f10a0c230 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -167,6 +167,10 @@ struct irq_common_data { > * @mask: precomputed bitmask for accessing the chip registers > * @irq: interrupt number > * @hwirq: hardware interrupt number, local to the interrupt domain > + * @dangling: amount of dangling hardware interrupt, Arm ITS allocate > + * hardware interrupt more than expected, aligned to power > + * of two, so that unsued interrupt number become dangling. > + * Use this field to record dangling bits follwoing @hwirq. > * @common: point to data shared by all irqchips > * @chip: low level interrupt hardware access > * @domain: Interrupt translation domain; responsible for mapping > @@ -180,6 +184,7 @@ struct irq_data { > u32 mask; > unsigned int irq; > unsigned long hwirq; > + unsigned long dangling; > struct irq_common_data *common; > struct irq_chip *chip; > struct irq_domain *domain; There is no way I will put ITS-specific hacks in the core, and I really don't think this is the correct way to address this. Also, why track this sort of thing on a per-interrupt basis, while this is obviously a device-level allocation? The real issue is that there is currently no way for the ITS code to know when we're done with *all* the interrupts of a device. This is what needs fixing. M. -- Without deviation from the norm, progress is not possible.