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 701DCC636CC for ; Wed, 15 Feb 2023 15:56: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:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:References:Cc:To:Subject:From: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=dbH0Clb/U2/G03IBB61fBUrR48JcOfhcCA3hNgh/KkU=; b=Ip55AqFUaL9vOw EHPGk/7N9ZNl0Eu/8BfrzEFJScCTb9JpNbEyJlw5nXROs7E6ub+h0QCcqYQVpoo/Ew8QS8bXW2DCa otv4kAv1m2M5FcCeJIBT8GkxclgWGVE8GyTCw9GXQ0mGBbpPOoW/9mNK8Xoexggq1R7t9OE6LPaot zfXY/rTYS3KYcoZBD8yh/O4tfj+6yGC+zkv7jCTSFlBZxnL2pM432JNOtoJHBeoKK7x71th+OmHXZ E2kdE/QmvGNILn3Fns+/EQVop9bx+aDQ+0y3GsTlYfEtd2AzSrYnacitrBEtgJf7GWqB+2kcyOMsy ePmS7CcXc3wWRoER1xpw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pSK8J-006RDt-IJ; Wed, 15 Feb 2023 15:55:47 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pSK8F-006RBi-Lk for linux-arm-kernel@lists.infradead.org; Wed, 15 Feb 2023 15:55:45 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 27A271042; Wed, 15 Feb 2023 07:56:18 -0800 (PST) Received: from [10.34.100.128] (pierre123.nice.arm.com [10.34.100.128]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7CDA53F881; Wed, 15 Feb 2023 07:55:33 -0800 (PST) Message-ID: <2af0ec8d-891c-f690-bfd6-b0e02597f391@arm.com> Date: Wed, 15 Feb 2023 16:55:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 From: Pierre Gondois Subject: Re: [PATCH v2] firmware: arm_sdei: Fix sleep from invalid context BUG To: James Morse , linux-kernel@vger.kernel.org Cc: Will Deacon , Huacai Chen , John Garry , Shaokun Zhang , WANG Xuerui , Qi Liu , Bharat Bhushan , Huang Ying , Bibo Mao , "Jason A. Donenfeld" , Daniel Lezcano , linux-arm-kernel@lists.infradead.org References: <20221018130456.1356081-1-pierre.gondois@arm.com> <5813b8c5-ae3e-87fd-fccc-94c9cd08816d@arm.com> Content-Language: en-US In-Reply-To: <5813b8c5-ae3e-87fd-fccc-94c9cd08816d@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230215_075543_856880_EB9F4C12 X-CRM114-Status: GOOD ( 26.96 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hello James, On 2/10/23 18:42, James Morse wrote: > Hi Pierre, > > Sorry its taken so long for me to catch up with this ... No worries. > > On 18/10/2022 14:04, Pierre Gondois wrote: >> On an Ampere Altra, >> Running a preemp_rt kernel based on v5.19-rc3-rt5 on an >> Ampere Altra triggers: >> [ 15.683141] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46 >> [ 15.683154] in_atomic(): 0, irqs_disabled(): 128, non_block: 0, pid: 24, name: cpuhp/0 >> [ 15.683157] preempt_count: 0, expected: 0 >> [ 15.683159] RCU nest depth: 0, expected: 0 >> [ 15.683163] 3 locks held by cpuhp/0/24: >> [ 15.683167] #0: ffffda30217c70d0 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x5c/0x248 >> [ 15.683201] #1: ffffda30217c7120 (cpuhp_state-up){+.+.}-{0:0}, at: cpuhp_thread_fun+0x5c/0x248 >> [ 15.683205] #2: ffffda3021c711f0 (sdei_list_lock){....}-{3:3}, at: sdei_cpuhp_up+0x3c/0x130 >> [ 15.683224] irq event stamp: 36 >> [ 15.683226] hardirqs last enabled at (35): [] finish_task_switch+0xb4/0x2b0 >> [ 15.683236] hardirqs last disabled at (36): [] cpuhp_thread_fun+0x21c/0x248 >> [ 15.683238] softirqs last enabled at (0): [] copy_process+0x63c/0x1ac0 >> [ 15.683245] softirqs last disabled at (0): [<0000000000000000>] 0x0 >> [ 15.683258] CPU: 0 PID: 24 Comm: cpuhp/0 Not tainted 5.19.0-rc3-rt5-[...] >> [ 15.683265] Hardware name: WIWYNN Mt.Jade Server System B81.03001.0005/Mt.Jade Motherboard, BIOS 1.08.20220218 (SCP: 1.08.20220218) 2022/02/18 >> [ 15.683268] Call trace: >> [ 15.683271] dump_backtrace+0x114/0x120 >> [ 15.683277] show_stack+0x20/0x70 >> [ 15.683279] dump_stack_lvl+0x9c/0xd8 >> [ 15.683288] dump_stack+0x18/0x34 >> [ 15.683289] __might_resched+0x188/0x228 >> [ 15.683292] rt_spin_lock+0x70/0x120 >> [ 15.683301] sdei_cpuhp_up+0x3c/0x130 >> [ 15.683303] cpuhp_invoke_callback+0x250/0xf08 >> [ 15.683305] cpuhp_thread_fun+0x120/0x248 >> [ 15.683308] smpboot_thread_fn+0x280/0x320 >> [ 15.683315] kthread+0x130/0x140 >> [ 15.683321] ret_from_fork+0x10/0x20 > >> sdei_cpuhp_up() is called in the STARTING hotplug section, >> which runs whith interrupts disabled. Move CPUHP_AP_ARM_SDEI_ >> state to the _ONLINE section to execute the cpuhp cb with >> preemption enabled. > > The background to this is SDEI got its own cpuhp slot because 'perf NMI' support > was one of the use-cases, but this got superseded by pNMI. Without an interaction with > perf, the slot doesn't need to be that early. > > >> Some SDEI calls (e.g. SDEI_1_0_FN_SDEI_PE_MASK) take actions on the >> calling CPU. It is checked that preemption is disabled for them. >> _ONLINE cpuhp cb are executed in the 'per CPU hotplug thread'. >> Preemption is enabled in those threads, but their cpumask is limited >> to 1 CPU. > >> Move 'WARN_ON_ONCE(preemptible())' statements so that SDEI cpuhp cb >> don't trigger them. This means that no check will be done for some >> cases, e.g. sdei_mask_local_cpu() invocations. > > (these things are documentation anyway) > > >> Also add a check for the SDEI_1_0_FN_SDEI_PRIVATE_RESET SDEI call >> which acts on the calling CPU. > >> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c >> index 1e1a51510e83..9b03e164a37a 100644 >> --- a/drivers/firmware/arm_sdei.c >> +++ b/drivers/firmware/arm_sdei.c > >> @@ -401,6 +399,8 @@ int sdei_event_enable(u32 event_num) >> int err = -EINVAL; >> struct sdei_event *event; >> >> + WARN_ON_ONCE(preemptible()); >> + >> mutex_lock(&sdei_events_lock); >> event = sdei_event_find(event_num); >> if (!event) { > > This doesn't look right. How can this code take a mutex if its in a non-preemptable context? > >> @@ -492,6 +490,7 @@ int sdei_event_unregister(u32 event_num) >> struct sdei_event *event; >> >> WARN_ON(in_nmi()); >> + WARN_ON_ONCE(preemptible()); >> >> mutex_lock(&sdei_events_lock); >> event = sdei_event_find(event_num); > > Same again. > >> @@ -576,6 +573,7 @@ int sdei_event_register(u32 event_num, sdei_event_callback *cb, void *arg) >> struct sdei_event *event; >> >> WARN_ON(in_nmi()); >> + WARN_ON_ONCE(preemptible()); >> >> mutex_lock(&sdei_events_lock); >> if (sdei_event_find(event_num)) { > > Same again. > > I think you've copied these here because they called _local_event_unregister(), but they > did that via on_each_cpu(), which is what made _local_event_unregister() non-preemtable. Yes right, these 3 warnings don't really make sense. They will be removed. > > You can just remove these three, the original warnings were mostly documentation, and to > catch myself out. (before RT moved the goal posts). > > With that: > Reviewed-by: James Morse > > (could you trim the [timestamps] out of the commit log?) Yes sure. > > >> @@ -765,7 +765,7 @@ static int sdei_device_freeze(struct device *dev) >> int err; >> >> /* unregister private events */ >> - cpuhp_remove_state(CPUHP_AP_ARM_SDEI_STARTING); >> + cpuhp_remove_state(CPUHP_AP_ARM_SDEI_ONLINE); > > Is there any mileage in making this CPUHP_AP_ONLINE_DYN ? > Perf really was the only reason that this needed to happen in any particular order. Ok I will do that. Thanks for the review, Regards, Pierre > > > Thanks, > > James _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel