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 959D3C001DB for ; Fri, 4 Aug 2023 16:28:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To: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=e7rS4gnZTpg7e9R7rBS0YKPpnIg0l5/ZaOJVFYIsjTA=; b=XvnGqK5tMrwR5t M+fOyWq5wqTIrHOjCrSYlzHhN1rxy9alrnIJdKI1/BuIWmqkhcFKbpsM3AyNqwC9nQNF9lGBRAypL Ijjd+aD3GhXqIMjPjDSzFcJSsNa7k/3MoZ2OpNognIadRreI44qRhRBYZ8uMoNdrrznrCsfwpXpfb Qx/1EUGdlXGp2f/fu/bSYTe0yQexLI5Sliio1LvodndTIVtecjUT8zy9FLyNyhz+XLD8pZNuJ6SwS gKSSYoVONTkpa5g8Sp4LIY6SWd+EpxsFcVVgvt0kUjAdEWszeTrViSFSrhd3K0hLmZjTe4PfBbeO/ nFO/Pr5W/Qb7Flta6LAg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qRxf3-00CnCc-1G; Fri, 04 Aug 2023 16:28:21 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qRxf0-00CnCG-2j for linux-arm-kernel@lists.infradead.org; Fri, 04 Aug 2023 16:28:20 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 27D7B6205C; Fri, 4 Aug 2023 16:28:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 855CFC433C8; Fri, 4 Aug 2023 16:28:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1691166497; bh=QbXK6cjzyzYpTexkDQnjA6HpPhZ1dR9sbMzjWsDQaz4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=J+1ISQA+ECvHq+Q4/jaw17XQbEhWuYvKjSs7I3XXafA2x75GhKAIhPa/Vdlbg1Uwq uZ/QqquK6kT7tdzYgUdgHuFi2eX8GT6xrqtxT/Hstgfycsibbf6lBXbAHre9mYZOoL KNqesgAbHMftkHbodI0lt7Vo2cAZ3dI17zvO3Ba26OlZvKMsCg+3d2FaxEk6rI1yfF xzioz4jJSI9LzIW2/FQas8iHwFY/8QhS96QuSB/1mJHUYT/RWRecO2L1D9FksNcBzd Qdvrax1lYq8q2vRPP3wKZ4fAGPzFCOO/wxlnD5phEVWlIo4yPA23Zw15E9VUlbzA7V qOHb25vwxWQWQ== Date: Fri, 4 Aug 2023 17:28:13 +0100 From: Will Deacon To: Waiman Long Cc: Mark Rutland , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Robin Murphy Subject: Re: [PATCH v3] perf/arm-dmc620: Fix dmc620_pmu_irqs_lock/cpu_hotplug_lock circular lock dependency Message-ID: <20230804162812.GC30679@willie-the-truck> References: <20230722031729.3913953-1-longman@redhat.com> <20230728150614.GF21718@willie-the-truck> <62d4b353-0237-9ec6-a63e-8a7a6764aba5@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <62d4b353-0237-9ec6-a63e-8a7a6764aba5@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230804_092818_977870_3613E1BD X-CRM114-Status: GOOD ( 34.22 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Aug 02, 2023 at 09:37:31PM -0400, Waiman Long wrote: > > On 7/28/23 11:06, Will Deacon wrote: > > On Fri, Jul 21, 2023 at 11:17:28PM -0400, Waiman Long wrote: > > > The following circular locking dependency was reported when running > > > cpus online/offline test on an arm64 system. > > > > > > [ 84.195923] Chain exists of: > > > dmc620_pmu_irqs_lock --> cpu_hotplug_lock --> cpuhp_state-down > > > > > > [ 84.207305] Possible unsafe locking scenario: > > > > > > [ 84.213212] CPU0 CPU1 > > > [ 84.217729] ---- ---- > > > [ 84.222247] lock(cpuhp_state-down); > > > [ 84.225899] lock(cpu_hotplug_lock); > > > [ 84.232068] lock(cpuhp_state-down); > > > [ 84.238237] lock(dmc620_pmu_irqs_lock); > > > [ 84.242236] > > > *** DEADLOCK *** > > > > > > The problematic locking order seems to be > > > > > > lock(dmc620_pmu_irqs_lock) --> lock(cpu_hotplug_lock) > > > > > > This locking order happens when dmc620_pmu_get_irq() is called from > > > dmc620_pmu_device_probe(). Since dmc620_pmu_irqs_lock is used for > > > protecting the dmc620_pmu_irqs structure only, we don't actually need > > > to hold the lock when adding a new instance to the CPU hotplug subsystem. > > > > > > Fix this possible deadlock scenario by releasing the lock before > > > calling cpuhp_state_add_instance_nocalls() and reacquiring it afterward. > > > To avoid the possibility of 2 racing dmc620_pmu_get_irq() calls inserting > > > duplicated dmc620_pmu_irq structures with the same irq number, a dummy > > > entry is inserted before releasing the lock which will block a competing > > > thread from inserting another irq structure of the same irq number. > > > > > > Suggested-by: Robin Murphy > > > Signed-off-by: Waiman Long > > > --- > > > drivers/perf/arm_dmc620_pmu.c | 28 ++++++++++++++++++++++------ > > > 1 file changed, 22 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/perf/arm_dmc620_pmu.c b/drivers/perf/arm_dmc620_pmu.c > > > index 9d0f01c4455a..7cafd4dd4522 100644 > > > --- a/drivers/perf/arm_dmc620_pmu.c > > > +++ b/drivers/perf/arm_dmc620_pmu.c > > > @@ -76,6 +76,7 @@ struct dmc620_pmu_irq { > > > refcount_t refcount; > > > unsigned int irq_num; > > > unsigned int cpu; > > > + unsigned int valid; > > > }; > > > struct dmc620_pmu { > > > @@ -423,9 +424,14 @@ static struct dmc620_pmu_irq *__dmc620_pmu_get_irq(int irq_num) > > > struct dmc620_pmu_irq *irq; > > > int ret; > > > - list_for_each_entry(irq, &dmc620_pmu_irqs, irqs_node) > > > - if (irq->irq_num == irq_num && refcount_inc_not_zero(&irq->refcount)) > > > + list_for_each_entry(irq, &dmc620_pmu_irqs, irqs_node) { > > > + if (irq->irq_num != irq_num) > > > + continue; > > > + if (!irq->valid) > > > + return ERR_PTR(-EAGAIN); /* Try again later */ > > It looks like this can bubble up to the probe() routine. Does the driver > > core handle -EAGAIN coming back from a probe routine? > Right, I should add code to handle this error condition. I think it can be > handled in dmc620_pmu_get_irq(). The important thing is to release the > mutex, wait a few ms and try again. What do you think? I don't really follow, but waiting a few ms and trying again sounds like a really nasty hack for something which doesn't appear to be constrained by broken hardware. In other words, we got ourselves into this mess, so we should be able to resolve it properly. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel