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 3A455C4167D for ; Tue, 17 May 2022 13:28:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347644AbiEQN2g (ORCPT ); Tue, 17 May 2022 09:28:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42532 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346419AbiEQN22 (ORCPT ); Tue, 17 May 2022 09:28:28 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A0519434B4; Tue, 17 May 2022 06:28:24 -0700 (PDT) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 4DBA821CC7; Tue, 17 May 2022 13:28:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1652794103; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=m8eH28qbE+2Xy0DKW3X9feLJxxFiFeYxt7IWdkwBacU=; b=D3nbGBAQ9bCKnB1WwjJRY0SPMWZeGE7s6okektsDFoMjSCg4MWkDlGu0CaIrnv72L2dSjB TH4hbCd7WNPBQBo5APMhOgzqiXxMfJKdv72h+T8GhDipQ36bscz5ULSJq0aGA9QxAaz7ZE 2DHOtfp4NZ3vqG8gLdmAnQza/SAFmg4= Received: from suse.cz (unknown [10.100.201.202]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id DE9622C141; Tue, 17 May 2022 13:28:20 +0000 (UTC) Date: Tue, 17 May 2022 15:28:20 +0200 From: Petr Mladek To: Evan Green Cc: "Guilherme G. Piccoli" , David Gow , Julius Werner , Scott Branden , bcm-kernel-feedback-list@broadcom.com, Sebastian Reichel , Linux PM , Florian Fainelli , Andrew Morton , bhe@redhat.com, kexec@lists.infradead.org, LKML , linuxppc-dev@lists.ozlabs.org, linux-alpha@vger.kernel.org, linux-arm Mailing List , linux-edac@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-leds@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-tegra@vger.kernel.org, linux-um@lists.infradead.org, linux-xtensa@linux-xtensa.org, netdev@vger.kernel.org, openipmi-developer@lists.sourceforge.net, rcu@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, halves@canonical.com, fabiomirmar@gmail.com, alejandro.j.jimenez@oracle.com, Andy Shevchenko , Arnd Bergmann , Borislav Petkov , Jonathan Corbet , d.hatayama@jp.fujitsu.com, dave.hansen@linux.intel.com, dyoung@redhat.com, feng.tang@intel.com, Greg Kroah-Hartman , mikelley@microsoft.com, hidehiro.kawai.ez@hitachi.com, jgross@suse.com, john.ogness@linutronix.de, Kees Cook , luto@kernel.org, mhiramat@kernel.org, mingo@redhat.com, paulmck@kernel.org, peterz@infradead.org, rostedt@goodmis.org, senozhatsky@chromium.org, Alan Stern , Thomas Gleixner , vgoyal@redhat.com, vkuznets@redhat.com, Will Deacon , Alexander Gordeev , Andrea Parri , Ard Biesheuvel , Benjamin Herrenschmidt , Brian Norris , Christian Borntraeger , Christophe JAILLET , "David S. Miller" , Dexuan Cui , Doug Berger , Haiyang Zhang , Hari Bathini , Heiko Carstens , Justin Chen , "K. Y. Srinivasan" , Lee Jones , Markus Mayer , Michael Ellerman , Mihai Carabas , Nicholas Piggin , Paul Mackerras , Pavel Machek , Shile Zhang , Stephen Hemminger , Sven Schnelle , Thomas Bogendoerfer , Tianyu Lan , Vasily Gorbik , Wang ShaoBo , Wei Liu , zhenwei pi , Stephen Boyd Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list Message-ID: References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-remoteproc@vger.kernel.org On Mon 2022-05-16 09:02:10, Evan Green wrote: > On Mon, May 16, 2022 at 8:07 AM Guilherme G. Piccoli > wrote: > > > > Thanks for the review! > > > > I agree with the blinking stuff, I can rework and add all LED/blinking > > stuff into the loop list, it does make sense. I'll comment a bit in the > > others below... > > > > On 16/05/2022 11:01, Petr Mladek wrote: > > > [...] > > >> --- a/arch/mips/sgi-ip22/ip22-reset.c > > >> +++ b/arch/mips/sgi-ip22/ip22-reset.c > > >> @@ -195,7 +195,7 @@ static int __init reboot_setup(void) > > >> } > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > This notifier enables blinking. It is not much safe. It calls > > > mod_timer() that takes a lock internally. > > > > > > This kind of functionality should go into the last list called > > > before panic() enters the infinite loop. IMHO, all the blinking > > > stuff should go there. > > > [...] > > >> --- a/arch/mips/sgi-ip32/ip32-reset.c > > >> +++ b/arch/mips/sgi-ip32/ip32-reset.c > > >> @@ -145,7 +144,7 @@ static __init int ip32_reboot_setup(void) > > >> pm_power_off = ip32_machine_halt; > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > Same here. Should be done only before the "loop". > > > [...] > > > > Ack. > > > > > > >> --- a/drivers/firmware/google/gsmi.c > > >> +++ b/drivers/firmware/google/gsmi.c > > >> @@ -1034,7 +1034,7 @@ static __init int gsmi_init(void) > > >> > > >> register_reboot_notifier(&gsmi_reboot_notifier); > > >> register_die_notifier(&gsmi_die_notifier); > > >> - atomic_notifier_chain_register(&panic_notifier_list, > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, > > >> &gsmi_panic_notifier); > > > > > > I am not sure about this one. It looks like some logging or > > > pre_reboot stuff. > > > > > > > Disagree here. I'm looping Google maintainers, so they can comment. > > (CCed Evan, David, Julius) > > > > This notifier is clearly a hypervisor notification mechanism. I've fixed > > a locking stuff there (in previous patch), I feel it's low-risk but even > > if it's mid-risk, the class of such callback remains a perfect fit with > > the hypervisor list IMHO. > > This logs a panic to our "eventlog", a tiny logging area in SPI flash > for critical and power-related events. In some cases this ends up > being the only clue we get in a Chromebook feedback report that a > panic occurred, so from my perspective moving it to the front of the > line seems like a good idea. IMHO, this would really better fit into the pre-reboot notifier list: + the callback stores the log so it is similar to kmsg_dump() or console_flush_on_panic() + the callback should be proceed after "info" notifiers that might add some other useful information. Honestly, I am not sure what exactly hypervisor callbacks do. But I think that they do not try to extract the kernel log because they would need to handle the internal format. Best Regards, Petr 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 lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (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 56AD3C433EF for ; Tue, 17 May 2022 23:25:03 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4L2sfP4b9Nz3cJs for ; Wed, 18 May 2022 09:25:01 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=suse.com header.i=@suse.com header.a=rsa-sha256 header.s=susede1 header.b=D3nbGBAQ; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=suse.com (client-ip=195.135.220.28; helo=smtp-out1.suse.de; envelope-from=pmladek@suse.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=suse.com header.i=@suse.com header.a=rsa-sha256 header.s=susede1 header.b=D3nbGBAQ; dkim-atps=neutral Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4L2cQ25zf2z2yMS for ; Tue, 17 May 2022 23:28:26 +1000 (AEST) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 4DBA821CC7; Tue, 17 May 2022 13:28:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1652794103; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=m8eH28qbE+2Xy0DKW3X9feLJxxFiFeYxt7IWdkwBacU=; b=D3nbGBAQ9bCKnB1WwjJRY0SPMWZeGE7s6okektsDFoMjSCg4MWkDlGu0CaIrnv72L2dSjB TH4hbCd7WNPBQBo5APMhOgzqiXxMfJKdv72h+T8GhDipQ36bscz5ULSJq0aGA9QxAaz7ZE 2DHOtfp4NZ3vqG8gLdmAnQza/SAFmg4= Received: from suse.cz (unknown [10.100.201.202]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id DE9622C141; Tue, 17 May 2022 13:28:20 +0000 (UTC) Date: Tue, 17 May 2022 15:28:20 +0200 From: Petr Mladek To: Evan Green Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list Message-ID: References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailman-Approved-At: Wed, 18 May 2022 09:23:23 +1000 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paul Mackerras , Justin Chen , Pavel Machek , Alexander Gordeev , "K. Y. Srinivasan" , Wei Liu , Alan Stern , xen-devel@lists.xenproject.org, Christian Borntraeger , Linux PM , linux-um@lists.infradead.org, Nicholas Piggin , Stephen Boyd , luto@kernel.org, Mihai Carabas , Thomas Gleixner , Greg Kroah-Hartman , LKML , senozhatsky@chromium.org, d.hatayama@jp.fujitsu.com, mhiramat@kernel.org, Andrew Morton , linux-hyperv@vger.kernel.org, dave.hansen@linux.intel.com, linux-s390@vger.kernel.org, Stephen Hemminger , Vasily Gorbik , vgoyal@redhat.com, Sven Schnelle , Andrea Parri , linux-xtensa@linux-xtensa.org, john.ogness@linutronix.de, Scott Branden , Doug Berger , Markus Mayer , hidehiro.kawai.ez@hitachi.com, linux-arm Mailing List , "Guilherme G. Piccoli" , kernel-dev@igalia.com, fabiomirmar@gmail.com, halves@canonical.com, alejandro.j.jimenez@oracle.com, feng.tang@intel.com, Will Deacon , Florian Fainelli , bhe@redhat.com, Jonathan Corbet , Dexuan Cui , zhenwei pi , bcm-kernel-feedback-list@broadcom.com, Tianyu Lan , Kees Cook , Arnd Bergmann , Haiyang Zhang , rostedt@goodmis.org, rcu@vger.kernel.org, Borislav Petkov , openipmi-developer@lists.sourceforge.net, Thomas Bogendoerfer , linux-parisc@vger.kernel.org, Sebastian Reichel , linux-alpha@vger.kernel.org, Brian Norris , "David S. Miller" , peterz@infradead.org, linux-remoteproc@vger.kernel.org, mikelley@microsoft.com, sparclinux@vger.kernel.org, Lee Jones , Ard Biesheuvel , linux-leds@vger.kernel.org, x86@kernel.org, mingo@redhat.com, dyoung@redhat.com, paulmck@kernel.org, Heiko Carstens , Shile Zhang , Wang ShaoBo , Christophe JAILLET , David Gow , linux-tegra@vger.kernel.org, Andy Shevchenko , Hari Bathini , linux-edac@vger.kernel.org, jgross@suse.com, netdev@vger.kernel.org, kernel@gpiccoli.net, kexec@lists.infradead.org, linux-mips@vger.kernel.org, Julius Werner , vkuznets@redhat.com, linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Mon 2022-05-16 09:02:10, Evan Green wrote: > On Mon, May 16, 2022 at 8:07 AM Guilherme G. Piccoli > wrote: > > > > Thanks for the review! > > > > I agree with the blinking stuff, I can rework and add all LED/blinking > > stuff into the loop list, it does make sense. I'll comment a bit in the > > others below... > > > > On 16/05/2022 11:01, Petr Mladek wrote: > > > [...] > > >> --- a/arch/mips/sgi-ip22/ip22-reset.c > > >> +++ b/arch/mips/sgi-ip22/ip22-reset.c > > >> @@ -195,7 +195,7 @@ static int __init reboot_setup(void) > > >> } > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > This notifier enables blinking. It is not much safe. It calls > > > mod_timer() that takes a lock internally. > > > > > > This kind of functionality should go into the last list called > > > before panic() enters the infinite loop. IMHO, all the blinking > > > stuff should go there. > > > [...] > > >> --- a/arch/mips/sgi-ip32/ip32-reset.c > > >> +++ b/arch/mips/sgi-ip32/ip32-reset.c > > >> @@ -145,7 +144,7 @@ static __init int ip32_reboot_setup(void) > > >> pm_power_off = ip32_machine_halt; > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > Same here. Should be done only before the "loop". > > > [...] > > > > Ack. > > > > > > >> --- a/drivers/firmware/google/gsmi.c > > >> +++ b/drivers/firmware/google/gsmi.c > > >> @@ -1034,7 +1034,7 @@ static __init int gsmi_init(void) > > >> > > >> register_reboot_notifier(&gsmi_reboot_notifier); > > >> register_die_notifier(&gsmi_die_notifier); > > >> - atomic_notifier_chain_register(&panic_notifier_list, > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, > > >> &gsmi_panic_notifier); > > > > > > I am not sure about this one. It looks like some logging or > > > pre_reboot stuff. > > > > > > > Disagree here. I'm looping Google maintainers, so they can comment. > > (CCed Evan, David, Julius) > > > > This notifier is clearly a hypervisor notification mechanism. I've fixed > > a locking stuff there (in previous patch), I feel it's low-risk but even > > if it's mid-risk, the class of such callback remains a perfect fit with > > the hypervisor list IMHO. > > This logs a panic to our "eventlog", a tiny logging area in SPI flash > for critical and power-related events. In some cases this ends up > being the only clue we get in a Chromebook feedback report that a > panic occurred, so from my perspective moving it to the front of the > line seems like a good idea. IMHO, this would really better fit into the pre-reboot notifier list: + the callback stores the log so it is similar to kmsg_dump() or console_flush_on_panic() + the callback should be proceed after "info" notifiers that might add some other useful information. Honestly, I am not sure what exactly hypervisor callbacks do. But I think that they do not try to extract the kernel log because they would need to handle the internal format. Best Regards, Petr From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Mladek Date: Tue, 17 May 2022 15:28:20 +0200 Subject: [PATCH 19/30] panic: Add the panic hypervisor notifier list In-Reply-To: References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kexec@lists.infradead.org On Mon 2022-05-16 09:02:10, Evan Green wrote: > On Mon, May 16, 2022 at 8:07 AM Guilherme G. Piccoli > wrote: > > > > Thanks for the review! > > > > I agree with the blinking stuff, I can rework and add all LED/blinking > > stuff into the loop list, it does make sense. I'll comment a bit in the > > others below... > > > > On 16/05/2022 11:01, Petr Mladek wrote: > > > [...] > > >> --- a/arch/mips/sgi-ip22/ip22-reset.c > > >> +++ b/arch/mips/sgi-ip22/ip22-reset.c > > >> @@ -195,7 +195,7 @@ static int __init reboot_setup(void) > > >> } > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > This notifier enables blinking. It is not much safe. It calls > > > mod_timer() that takes a lock internally. > > > > > > This kind of functionality should go into the last list called > > > before panic() enters the infinite loop. IMHO, all the blinking > > > stuff should go there. > > > [...] > > >> --- a/arch/mips/sgi-ip32/ip32-reset.c > > >> +++ b/arch/mips/sgi-ip32/ip32-reset.c > > >> @@ -145,7 +144,7 @@ static __init int ip32_reboot_setup(void) > > >> pm_power_off = ip32_machine_halt; > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > Same here. Should be done only before the "loop". > > > [...] > > > > Ack. > > > > > > >> --- a/drivers/firmware/google/gsmi.c > > >> +++ b/drivers/firmware/google/gsmi.c > > >> @@ -1034,7 +1034,7 @@ static __init int gsmi_init(void) > > >> > > >> register_reboot_notifier(&gsmi_reboot_notifier); > > >> register_die_notifier(&gsmi_die_notifier); > > >> - atomic_notifier_chain_register(&panic_notifier_list, > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, > > >> &gsmi_panic_notifier); > > > > > > I am not sure about this one. It looks like some logging or > > > pre_reboot stuff. > > > > > > > Disagree here. I'm looping Google maintainers, so they can comment. > > (CCed Evan, David, Julius) > > > > This notifier is clearly a hypervisor notification mechanism. I've fixed > > a locking stuff there (in previous patch), I feel it's low-risk but even > > if it's mid-risk, the class of such callback remains a perfect fit with > > the hypervisor list IMHO. > > This logs a panic to our "eventlog", a tiny logging area in SPI flash > for critical and power-related events. In some cases this ends up > being the only clue we get in a Chromebook feedback report that a > panic occurred, so from my perspective moving it to the front of the > line seems like a good idea. IMHO, this would really better fit into the pre-reboot notifier list: + the callback stores the log so it is similar to kmsg_dump() or console_flush_on_panic() + the callback should be proceed after "info" notifiers that might add some other useful information. Honestly, I am not sure what exactly hypervisor callbacks do. But I think that they do not try to extract the kernel log because they would need to handle the internal format. Best Regards, Petr From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 17 May 2022 15:28:20 +0200 From: Petr Mladek Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list Message-ID: References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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-um" Errors-To: linux-um-bounces+geert=linux-m68k.org@lists.infradead.org To: Evan Green Cc: "Guilherme G. Piccoli" , David Gow , Julius Werner , Scott Branden , bcm-kernel-feedback-list@broadcom.com, Sebastian Reichel , Linux PM , Florian Fainelli , Andrew Morton , bhe@redhat.com, kexec@lists.infradead.org, LKML , linuxppc-dev@lists.ozlabs.org, linux-alpha@vger.kernel.org, linux-arm Mailing List , linux-edac@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-leds@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-s390@vger.kernel.org, linux-tegra@vger.kernel.org, linux-um@lists.infradead.org, linux-xtensa@linux-xtensa.org, netdev@vger.kernel.org, openipmi-developer@lists.sourceforge.net, rcu@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, x86@kernel.org, kernel-dev@igalia.com, kernel@gpiccoli.net, halves@canonical.com, fabiomirmar@gmail.com, alejandro.j.jimenez@oracle.com, Andy Shevchenko , Arnd Bergmann , Borislav Petkov , Jonathan Corbet , d.hatayama@jp.fujitsu.com, dave.hansen@linux.intel.com, dyoung@redhat.com, feng.tang@intel.com, Greg Kroah-Hartman , mikelley@microsoft.com, hidehiro.kawai.ez@hitachi.com, jgross@suse.com, john.ogness@linutronix.de, Kees Cook , luto@kernel.org, mhiramat@kernel.org, mingo@redhat.com, paulmck@kernel.org, peterz@infradead.org, rostedt@goodmis.org, senozhatsky@chromium.org, Alan Stern , Thomas Gleixner , vgoyal@redhat.com, vkuznets@redhat.com, Will Deacon , Alexander Gordeev , Andrea Parri , Ard Biesheuvel , Benjamin Herrenschmidt , Brian Norris , Christian Borntraeger , Christophe JAILLET , "David S. Miller" , Dexuan Cui , Doug Berger , Haiyang Zhang , Hari Bathini , Heiko Carstens , Justin Chen , "K. Y. Srinivasan" , Lee Jones , Markus Mayer , Michael Ellerman , Mihai Carabas , Nicholas Piggin , Paul Mackerras , Pavel Machek , Shile Zhang , Stephen Hemminger , Sven Schnelle , Thomas Bogendoerfer , Tianyu Lan , Vasily Gorbik , Wang ShaoBo , Wei Liu , zhenwei pi , Stephen Boyd On Mon 2022-05-16 09:02:10, Evan Green wrote: > On Mon, May 16, 2022 at 8:07 AM Guilherme G. Piccoli > wrote: > > > > Thanks for the review! > > > > I agree with the blinking stuff, I can rework and add all LED/blinking > > stuff into the loop list, it does make sense. I'll comment a bit in the > > others below... > > > > On 16/05/2022 11:01, Petr Mladek wrote: > > > [...] > > >> --- a/arch/mips/sgi-ip22/ip22-reset.c > > >> +++ b/arch/mips/sgi-ip22/ip22-reset.c > > >> @@ -195,7 +195,7 @@ static int __init reboot_setup(void) > > >> } > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > This notifier enables blinking. It is not much safe. It calls > > > mod_timer() that takes a lock internally. > > > > > > This kind of functionality should go into the last list called > > > before panic() enters the infinite loop. IMHO, all the blinking > > > stuff should go there. > > > [...] > > >> --- a/arch/mips/sgi-ip32/ip32-reset.c > > >> +++ b/arch/mips/sgi-ip32/ip32-reset.c > > >> @@ -145,7 +144,7 @@ static __init int ip32_reboot_setup(void) > > >> pm_power_off = ip32_machine_halt; > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > Same here. Should be done only before the "loop". > > > [...] > > > > Ack. > > > > > > >> --- a/drivers/firmware/google/gsmi.c > > >> +++ b/drivers/firmware/google/gsmi.c > > >> @@ -1034,7 +1034,7 @@ static __init int gsmi_init(void) > > >> > > >> register_reboot_notifier(&gsmi_reboot_notifier); > > >> register_die_notifier(&gsmi_die_notifier); > > >> - atomic_notifier_chain_register(&panic_notifier_list, > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, > > >> &gsmi_panic_notifier); > > > > > > I am not sure about this one. It looks like some logging or > > > pre_reboot stuff. > > > > > > > Disagree here. I'm looping Google maintainers, so they can comment. > > (CCed Evan, David, Julius) > > > > This notifier is clearly a hypervisor notification mechanism. I've fixed > > a locking stuff there (in previous patch), I feel it's low-risk but even > > if it's mid-risk, the class of such callback remains a perfect fit with > > the hypervisor list IMHO. > > This logs a panic to our "eventlog", a tiny logging area in SPI flash > for critical and power-related events. In some cases this ends up > being the only clue we get in a Chromebook feedback report that a > panic occurred, so from my perspective moving it to the front of the > line seems like a good idea. IMHO, this would really better fit into the pre-reboot notifier list: + the callback stores the log so it is similar to kmsg_dump() or console_flush_on_panic() + the callback should be proceed after "info" notifiers that might add some other useful information. Honestly, I am not sure what exactly hypervisor callbacks do. But I think that they do not try to extract the kernel log because they would need to handle the internal format. Best Regards, Petr _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Mladek Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list Date: Tue, 17 May 2022 15:28:20 +0200 Message-ID: References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1652794103; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=m8eH28qbE+2Xy0DKW3X9feLJxxFiFeYxt7IWdkwBacU=; b=D3nbGBAQ9bCKnB1WwjJRY0SPMWZeGE7s6okektsDFoMjSCg4MWkDlGu0CaIrnv72L2dSjB TH4hbCd7WNPBQBo5APMhOgzqiXxMfJKdv72h+T8GhDipQ36bscz5ULSJq0aGA9QxAaz7ZE 2DHOtfp4NZ3vqG8gLdmAnQza/SAFmg4= Content-Disposition: inline In-Reply-To: List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Evan Green Cc: "Guilherme G. Piccoli" , David Gow , Julius Werner , Scott Branden , bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org, Sebastian Reichel , Linux PM , Florian Fainelli , Andrew Morton , bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, LKML , linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, linux-alpha-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm Mailing List , linux-edac-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-hyperv-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mips-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-parisc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-remoteproc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-s390-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-tegr On Mon 2022-05-16 09:02:10, Evan Green wrote: > On Mon, May 16, 2022 at 8:07 AM Guilherme G. Piccoli > wrote: > > > > Thanks for the review! > > > > I agree with the blinking stuff, I can rework and add all LED/blinking > > stuff into the loop list, it does make sense. I'll comment a bit in the > > others below... > > > > On 16/05/2022 11:01, Petr Mladek wrote: > > > [...] > > >> --- a/arch/mips/sgi-ip22/ip22-reset.c > > >> +++ b/arch/mips/sgi-ip22/ip22-reset.c > > >> @@ -195,7 +195,7 @@ static int __init reboot_setup(void) > > >> } > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > This notifier enables blinking. It is not much safe. It calls > > > mod_timer() that takes a lock internally. > > > > > > This kind of functionality should go into the last list called > > > before panic() enters the infinite loop. IMHO, all the blinking > > > stuff should go there. > > > [...] > > >> --- a/arch/mips/sgi-ip32/ip32-reset.c > > >> +++ b/arch/mips/sgi-ip32/ip32-reset.c > > >> @@ -145,7 +144,7 @@ static __init int ip32_reboot_setup(void) > > >> pm_power_off = ip32_machine_halt; > > >> > > >> timer_setup(&blink_timer, blink_timeout, 0); > > >> - atomic_notifier_chain_register(&panic_notifier_list, &panic_block); > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, &panic_block); > > > > > > Same here. Should be done only before the "loop". > > > [...] > > > > Ack. > > > > > > >> --- a/drivers/firmware/google/gsmi.c > > >> +++ b/drivers/firmware/google/gsmi.c > > >> @@ -1034,7 +1034,7 @@ static __init int gsmi_init(void) > > >> > > >> register_reboot_notifier(&gsmi_reboot_notifier); > > >> register_die_notifier(&gsmi_die_notifier); > > >> - atomic_notifier_chain_register(&panic_notifier_list, > > >> + atomic_notifier_chain_register(&panic_hypervisor_list, > > >> &gsmi_panic_notifier); > > > > > > I am not sure about this one. It looks like some logging or > > > pre_reboot stuff. > > > > > > > Disagree here. I'm looping Google maintainers, so they can comment. > > (CCed Evan, David, Julius) > > > > This notifier is clearly a hypervisor notification mechanism. I've fixed > > a locking stuff there (in previous patch), I feel it's low-risk but even > > if it's mid-risk, the class of such callback remains a perfect fit with > > the hypervisor list IMHO. > > This logs a panic to our "eventlog", a tiny logging area in SPI flash > for critical and power-related events. In some cases this ends up > being the only clue we get in a Chromebook feedback report that a > panic occurred, so from my perspective moving it to the front of the > line seems like a good idea. IMHO, this would really better fit into the pre-reboot notifier list: + the callback stores the log so it is similar to kmsg_dump() or console_flush_on_panic() + the callback should be proceed after "info" notifiers that might add some other useful information. Honestly, I am not sure what exactly hypervisor callbacks do. But I think that they do not try to extract the kernel log because they would need to handle the internal format. Best Regards, Petr