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 B6433C04A68 for ; Thu, 28 Jul 2022 11:48:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236757AbiG1Lsa (ORCPT ); Thu, 28 Jul 2022 07:48:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52756 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236346AbiG1Ls2 (ORCPT ); Thu, 28 Jul 2022 07:48:28 -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 F323167CAB for ; Thu, 28 Jul 2022 04:48:27 -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 8F89A61B77 for ; Thu, 28 Jul 2022 11:48:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EB467C433C1; Thu, 28 Jul 2022 11:48:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1659008907; bh=g1ToiQ9ikGco4SSXQkZwptv+T6Xj8qP+/AAmx1sioq8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=K5qvEpGP7bx3f+MSRtC197Sy96M+qHZEKBLeRJ6yALzvwptSVGk4MhRRKTUzg61DT fhM5/DBwGp3ezfhgWTtrm2qNfYmJzR3xD393pU+RatyA5dwhPVXciYoIOHTi8Yzwaq Ebw7TIa+8sYn4y9w2oCsCTpXd2VEO2EfZMjf5JlkyCTjsiUcuRXw/fvFJbJS5fZX+Y xmihqrGCRF6C84KHLIqqW3jWBYr63ERJ8oA0Hn7VifYbOqR/WivB8RUf0w5QiK3fCi Z9dt+i8a1H1YOqY3uo9AFhppnzac7+qs2Ytz4jH5gvRRlyxStW305cW33lTYzcYkMl Lapv7GAuPoXSQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.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 1oH208-00AefZ-Bw; Thu, 28 Jul 2022 12:48:24 +0100 Date: Thu, 28 Jul 2022 12:48:23 +0100 Message-ID: <87wnbxwj94.wl-maz@kernel.org> From: Marc Zyngier To: Johan Hovold Cc: Thomas Gleixner , Greg Kroah-Hartman , Rob Herring , linux-kernel@vger.kernel.org, Dmitry Torokhov , Jon Hunter Subject: Re: [PATCH] irqdomain: Fix mapping-creation race In-Reply-To: <20220728092710.21190-1-johan+linaro@kernel.org> References: <20220728092710.21190-1-johan+linaro@kernel.org> 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: 185.219.108.64 X-SA-Exim-Rcpt-To: johan+linaro@kernel.org, tglx@linutronix.de, gregkh@linuxfoundation.org, robh@kernel.org, linux-kernel@vger.kernel.org, dtor@chromium.org, jonathanh@nvidia.com 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, 28 Jul 2022 10:27:10 +0100, Johan Hovold wrote: > > Parallel probing (e.g. due to asynchronous probing) of devices that share > interrupts can currently result in two mappings for the same hardware > interrupt to be created. And I thought nobody would be using shared interrupts anymore. Turns out people are still building braindead HW... :-/ > > Add a serialising mapping mutex so that looking for an existing mapping > before creating a new one is done atomically. > > Note that serialising the lookup and creation in > irq_create_mapping_affinity() would have been enough to prevent the > duplicate mapping, but that could instead cause > irq_create_fwspec_mapping() to fail when there is a race. > > Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for drivers") > Fixes: b62b2cf5759b ("irqdomain: Fix handling of type settings for existing mappings") > Cc: Dmitry Torokhov > Cc: Jon Hunter > Signed-off-by: Johan Hovold > --- > kernel/irq/irqdomain.c | 46 +++++++++++++++++++++++++++++++----------- > 1 file changed, 34 insertions(+), 12 deletions(-) > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index 8fe1da9614ee..d263a7dd4170 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -22,6 +22,7 @@ > > static LIST_HEAD(irq_domain_list); > static DEFINE_MUTEX(irq_domain_mutex); > +static DEFINE_MUTEX(irq_mapping_mutex); I'd really like to avoid a global mutex. At the very least this should be a per-domain mutex, otherwise this will serialise a lot more than what is needed. > > static struct irq_domain *irq_default_domain; > > @@ -669,7 +670,7 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapping); > #endif > > /** > - * irq_create_mapping_affinity() - Map a hardware interrupt into linux irq space > + * __irq_create_mapping_affinity() - Map a hardware interrupt into linux irq space > * @domain: domain owning this hardware interrupt or NULL for default domain > * @hwirq: hardware irq number in that domain space > * @affinity: irq affinity > @@ -679,9 +680,9 @@ EXPORT_SYMBOL_GPL(irq_create_direct_mapping); > * If the sense/trigger is to be specified, set_irq_type() should be called > * on the number returned from that call. > */ This comment should be moved to the exported function, instead of documenting something that nobody can call... > -unsigned int irq_create_mapping_affinity(struct irq_domain *domain, > - irq_hw_number_t hwirq, > - const struct irq_affinity_desc *affinity) > +static unsigned int __irq_create_mapping_affinity(struct irq_domain *domain, > + irq_hw_number_t hwirq, > + const struct irq_affinity_desc *affinity) > { > struct device_node *of_node; > int virq; > @@ -724,6 +725,19 @@ unsigned int irq_create_mapping_affinity(struct irq_domain *domain, > > return virq; > } > + > +unsigned int irq_create_mapping_affinity(struct irq_domain *domain, > + irq_hw_number_t hwirq, > + const struct irq_affinity_desc *affinity) > +{ > + unsigned int virq; > + > + mutex_lock(&irq_mapping_mutex); > + virq = __irq_create_mapping_affinity(domain, hwirq, affinity); > + mutex_unlock(&irq_mapping_mutex); > + > + return virq; > +} > EXPORT_SYMBOL_GPL(irq_create_mapping_affinity); > > static int irq_domain_translate(struct irq_domain *d, > @@ -789,6 +803,8 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK)) > type &= IRQ_TYPE_SENSE_MASK; > > + mutex_lock(&irq_mapping_mutex); > + > /* > * If we've already configured this interrupt, > * don't do it again, or hell will break loose. > @@ -801,7 +817,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > * interrupt number. > */ > if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq)) > - return virq; > + goto out; > > /* > * If the trigger type has not been set yet, then set > @@ -810,26 +826,26 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) { > irq_data = irq_get_irq_data(virq); > if (!irq_data) > - return 0; > + goto err; > > irqd_set_trigger_type(irq_data, type); > - return virq; > + goto out; > } > > pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n", > hwirq, of_node_full_name(to_of_node(fwspec->fwnode))); > - return 0; > + goto err; > } > > if (irq_domain_is_hierarchy(domain)) { > virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec); > if (virq <= 0) > - return 0; > + goto err; > } else { > /* Create mapping */ > - virq = irq_create_mapping(domain, hwirq); > + virq = __irq_create_mapping_affinity(domain, hwirq, NULL); This rechecks for the existence of the mapping. Surely we can do a bit better by rejigging this (admittedly bitrotting) code. Thanks, M. -- Without deviation from the norm, progress is not possible.