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 69EDFC4321E for ; Mon, 16 May 2022 16:09:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245725AbiEPQJu (ORCPT ); Mon, 16 May 2022 12:09:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39254 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234466AbiEPQJr (ORCPT ); Mon, 16 May 2022 12:09:47 -0400 Received: from mail-oa1-x33.google.com (mail-oa1-x33.google.com [IPv6:2001:4860:4864:20::33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 33A2E37BC4 for ; Mon, 16 May 2022 09:09:46 -0700 (PDT) Received: by mail-oa1-x33.google.com with SMTP id 586e51a60fabf-edf9ddb312so20721200fac.8 for ; Mon, 16 May 2022 09:09:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ScgtWm2uFkOxfulqs7520gHM/my4MYIe3yq/VcV3Ozw=; b=Ui6BaQ6lQ9IwPfOUL4TJm7cJvfYf8UFV8wI0yIE/ecSwLqobvQf4rTIikXPC8Ksnp/ JRSTr3Fgwya3hm14X44WgUx1HJGuc9H1MoeHAN4VBzxuhJEnypxN8+xF6HAWoKiAVAs3 mxYrKvjtDKZ5Z0eJvjVSmqSHIwF8R/De7GbNU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ScgtWm2uFkOxfulqs7520gHM/my4MYIe3yq/VcV3Ozw=; b=DVHXZ0pnIIC613vMBef8Rc2NOpsc+JYyPjy0jmP5Y6ZG2Pwygs9ry8FO+nnF2O+Bt/ PgfVQ/z249TjVtrhHlBbfxy1FN5C3ZW13cS86i2hrBuqJFCyra4sQvrHMSWbi0MQLMAK mBsxK4u7Se4KN33Ow++2rTs7fuzwuw9o7KMcudopdk7G6iZzObw6FZmZIjerUPvKfQ+B h/48cRDonEO8J/OR2xHuuYGvjpAZg9NzajASeiEXydtrtK+3Pjdzvi/QEEy02FrUXbhL rAH+0Hasg5F7sF0zAS1cUNgex6hO9dqR6UxYwtgZU5+tDFcc6VM0RSEsLEEkvtf4Ahbx RaNw== X-Gm-Message-State: AOAM530maljlL8ZNh27EkxBpAxy6KQ0GJrEBT8Ek1eqXDXHMOdMqvzyu bp4iCjRMK/y6oQrVKK0+YdJQRv0McneF7cri X-Google-Smtp-Source: ABdhPJzM68lEuCGiSC7Zk4n9D8Yiz6xyXwTVuo8WLH0wLIZA11gIqmNFAi+wWQAHtgVEDSVzVp1jqA== X-Received: by 2002:a05:6870:7084:b0:ed:d709:34be with SMTP id v4-20020a056870708400b000edd70934bemr14857018oae.4.1652717384840; Mon, 16 May 2022 09:09:44 -0700 (PDT) Received: from mail-oa1-f43.google.com (mail-oa1-f43.google.com. [209.85.160.43]) by smtp.gmail.com with ESMTPSA id q5-20020a4aa885000000b0035eb4e5a6b6sm4293915oom.12.2022.05.16.09.09.44 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 May 2022 09:09:44 -0700 (PDT) Received: by mail-oa1-f43.google.com with SMTP id 586e51a60fabf-f16a3e0529so10399566fac.2 for ; Mon, 16 May 2022 09:09:44 -0700 (PDT) X-Received: by 2002:a05:6870:63a0:b0:f1:8bca:8459 with SMTP id t32-20020a05687063a000b000f18bca8459mr4861359oap.174.1652716966894; Mon, 16 May 2022 09:02:46 -0700 (PDT) MIME-Version: 1.0 References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> In-Reply-To: From: Evan Green Date: Mon, 16 May 2022 09:02:10 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list To: "Guilherme G. Piccoli" Cc: Petr Mladek , 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-remoteproc@vger.kernel.org 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. -Evan 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 D5FA8C433EF for ; Mon, 16 May 2022 21:45:54 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4L2CVT2792z3f4J for ; Tue, 17 May 2022 07:45:53 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.a=rsa-sha256 header.s=google header.b=Ui6BaQ6l; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=chromium.org (client-ip=2607:f8b0:4864:20::32e; helo=mail-ot1-x32e.google.com; envelope-from=evgreen@chromium.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.a=rsa-sha256 header.s=google header.b=Ui6BaQ6l; dkim-atps=neutral Received: from mail-ot1-x32e.google.com (mail-ot1-x32e.google.com [IPv6:2607:f8b0:4864:20::32e]) (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 4L24463Hrzz3bwG for ; Tue, 17 May 2022 02:11:01 +1000 (AEST) Received: by mail-ot1-x32e.google.com with SMTP id m6-20020a05683023a600b0060612720715so10363121ots.10 for ; Mon, 16 May 2022 09:11:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ScgtWm2uFkOxfulqs7520gHM/my4MYIe3yq/VcV3Ozw=; b=Ui6BaQ6lQ9IwPfOUL4TJm7cJvfYf8UFV8wI0yIE/ecSwLqobvQf4rTIikXPC8Ksnp/ JRSTr3Fgwya3hm14X44WgUx1HJGuc9H1MoeHAN4VBzxuhJEnypxN8+xF6HAWoKiAVAs3 mxYrKvjtDKZ5Z0eJvjVSmqSHIwF8R/De7GbNU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ScgtWm2uFkOxfulqs7520gHM/my4MYIe3yq/VcV3Ozw=; b=p5rvM7NZxd7S+WEil7oBhbIIcvBl5Ev5J8/kY48cl6keafzw1ljqfGRoKllkmENfjv rQJaXTNzTYGTzdkarW/bBC4pRJeQzaWWGpfHkDQpUuzpADpdeXRQbxWlyT8f+TMxVZ91 dZ9TijDfgAlexEho0R5n3wFuktST1gQ45NnwKifcxFi88sMoIE9YBYnogV4jabBAywid TRFL5FXZqL1YFtAV/IO1R46CQ7+JrhpG71I1bkKAgXAKRVHNQ5D8takKqzUagsM/aMjm RbaMxQsh0frctGFsWoMtBXaMQ4nUhj75icXEN8U7YZ7rfmeReoFGkc0kGV4JkH8qaV6k SnXg== X-Gm-Message-State: AOAM5338Kb0whx9+kElpQDA1PioZcEWvWY9KshPrFSEBI3dOCkGI6gbn qecahENhD5pTWAPavPVCsxJ1yPuwEf+tYzN0 X-Google-Smtp-Source: ABdhPJx2uyUY9TwUw3LTZZdWN+zHU8RQO0fbqYXV3+dm8O/g6aeJf0td8t3yfW7Tc+bohqDy0zCjbA== X-Received: by 2002:a05:6830:3114:b0:606:181f:5daf with SMTP id b20-20020a056830311400b00606181f5dafmr6467011ots.335.1652717457864; Mon, 16 May 2022 09:10:57 -0700 (PDT) Received: from mail-oa1-f48.google.com (mail-oa1-f48.google.com. [209.85.160.48]) by smtp.gmail.com with ESMTPSA id a1-20020a056870374100b000edda81f868sm5373593oak.10.2022.05.16.09.10.57 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 May 2022 09:10:57 -0700 (PDT) Received: by mail-oa1-f48.google.com with SMTP id 586e51a60fabf-e656032735so20804442fac.0 for ; Mon, 16 May 2022 09:10:57 -0700 (PDT) X-Received: by 2002:a05:6870:63a0:b0:f1:8bca:8459 with SMTP id t32-20020a05687063a000b000f18bca8459mr4861359oap.174.1652716966894; Mon, 16 May 2022 09:02:46 -0700 (PDT) MIME-Version: 1.0 References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> In-Reply-To: From: Evan Green Date: Mon, 16 May 2022 09:02:10 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list To: "Guilherme G. Piccoli" Content-Type: text/plain; charset="UTF-8" X-Mailman-Approved-At: Tue, 17 May 2022 07:40:39 +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 , Petr Mladek , 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 , 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, 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. -Evan From mboxrd@z Thu Jan 1 00:00:00 1970 From: Evan Green Date: Mon, 16 May 2022 09:02:10 -0700 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, 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. -Evan From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x31.google.com ([2001:4860:4864:20::31]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nqdGh-008rYn-Dk for linux-um@lists.infradead.org; Mon, 16 May 2022 16:08:25 +0000 Received: by mail-oa1-x31.google.com with SMTP id 586e51a60fabf-e5e433d66dso20712680fac.5 for ; Mon, 16 May 2022 09:08:22 -0700 (PDT) Received: from mail-oi1-f176.google.com (mail-oi1-f176.google.com. [209.85.167.176]) by smtp.gmail.com with ESMTPSA id k19-20020a056870571300b000f169cbbb32sm4715596oap.43.2022.05.16.09.08.21 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 May 2022 09:08:21 -0700 (PDT) Received: by mail-oi1-f176.google.com with SMTP id i66so19120286oia.11 for ; Mon, 16 May 2022 09:08:21 -0700 (PDT) MIME-Version: 1.0 References: <20220427224924.592546-1-gpiccoli@igalia.com> <20220427224924.592546-20-gpiccoli@igalia.com> In-Reply-To: From: Evan Green Date: Mon, 16 May 2022 09:02:10 -0700 Message-ID: Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier 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-um" Errors-To: linux-um-bounces+geert=linux-m68k.org@lists.infradead.org To: "Guilherme G. Piccoli" Cc: Petr Mladek , 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, 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. -Evan _______________________________________________ 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: Evan Green Subject: Re: [PATCH 19/30] panic: Add the panic hypervisor notifier list Date: Mon, 16 May 2022 09:02:10 -0700 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=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ScgtWm2uFkOxfulqs7520gHM/my4MYIe3yq/VcV3Ozw=; b=Ui6BaQ6lQ9IwPfOUL4TJm7cJvfYf8UFV8wI0yIE/ecSwLqobvQf4rTIikXPC8Ksnp/ JRSTr3Fgwya3hm14X44WgUx1HJGuc9H1MoeHAN4VBzxuhJEnypxN8+xF6HAWoKiAVAs3 mxYrKvjtDKZ5Z0eJvjVSmqSHIwF8R/De7GbNU= In-Reply-To: List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Guilherme G. Piccoli" Cc: Petr Mladek , 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 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. -Evan