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 5089FC433EF for ; Mon, 16 May 2022 16:10:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245718AbiEPQKO (ORCPT ); Mon, 16 May 2022 12:10:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39742 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241421AbiEPQKL (ORCPT ); Mon, 16 May 2022 12:10:11 -0400 Received: from mail-oo1-xc30.google.com (mail-oo1-xc30.google.com [IPv6:2607:f8b0:4864:20::c30]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 350542ED5C for ; Mon, 16 May 2022 09:10:11 -0700 (PDT) Received: by mail-oo1-xc30.google.com with SMTP id q73-20020a4a334c000000b0035eb110dd0dso4097377ooq.10 for ; Mon, 16 May 2022 09:10:11 -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=B2FsA0Abu0GvOFnmW2H7oo9ZdRXH22/aMpIIfNOmmBae5IkNfiTLPCr5fIEkytqqs2 wF+Gw88NBd0pkQu7rGHuP0cjyWxQPkf/gbK6cN3s0fmW4NrkHhZ2K3dSmL0nFgLZMKuy YWZQoGs638ilgFf2AUa6XRN9aHhOX7563DtViCdtpPkir+N8F0k7LYis/5UQLKgBc060 i9n+XLPpKnth1Vvi5kdqGfHT8o9MqnMN4bl6cJT+ryMehdCEGq5hLpv4bJpMR5o0kUAP knl+NnT23bp5XkQ5XCSZzAzwQPrE3kzLFzgKtvPul2UPeVrS4fsYSimLdBydAw3L3t0p am0g== X-Gm-Message-State: AOAM530fHFlQKNoqzgE6NxHYkDj7rFb9tqt8tPl110UWWoMy50BIb4Q1 2AjSRah70+rz2Fl8HD7CxckeAANubAUbKPHf X-Google-Smtp-Source: ABdhPJxyi354UiYX7Ex+oHNBewHH7NWFvYbgYKjiMJTIesCJdwCHXMO4+418vHMtCIJfeXYjBmpgZQ== X-Received: by 2002:a4a:8888:0:b0:321:7448:42df with SMTP id j8-20020a4a8888000000b00321744842dfmr6175478ooa.82.1652717409832; Mon, 16 May 2022 09:10:09 -0700 (PDT) Received: from mail-oi1-f177.google.com (mail-oi1-f177.google.com. [209.85.167.177]) by smtp.gmail.com with ESMTPSA id a18-20020a4ad1d2000000b0035eb4e5a6c2sm4387964oos.24.2022.05.16.09.10.09 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 May 2022 09:10:09 -0700 (PDT) Received: by mail-oi1-f177.google.com with SMTP id q10so19160965oia.9 for ; Mon, 16 May 2022 09:10:09 -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-kernel@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