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 36C7BC4332F for ; Mon, 18 Apr 2022 01:29:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235580AbiDRBcV (ORCPT ); Sun, 17 Apr 2022 21:32:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43450 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232905AbiDRBcR (ORCPT ); Sun, 17 Apr 2022 21:32:17 -0400 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EDFBB17068; Sun, 17 Apr 2022 18:29:38 -0700 (PDT) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: dmitry.osipenko) with ESMTPSA id 77A9B1F44320 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1650245376; bh=Ajn7d9ehxuO5NknWvmo0k/aIhgzNt/YPk9MMAC0rgK4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=H6wbrzdvBv8/q0QMxBBTynYiA8MocDKNi6pPzdbeah1TBS0sfobjoYMeVHdywDKJa YOMseWFehs7fKiS1bqLxpLaRV3E2gVJ6NGmAk/kK7Tkj7SOk+d7kIA+pVboML+0znZ oBB6ZdZaJe8q4CuDbVjC8i5gSLP4M7dhUJeJhWywSpYx0y55HsGlo0NSg/sn82x813 XAZwVlD5NoEcUOWKl8ezQsrDqxJrPlOK3xiszEQs//fITbMqaQ95IAUW03Xr4Fzay/ IWbATXa2Chl0Jy2VqKMQd994g2oQEgzNvPwVOvB9VgKGF8p+Y8PjFoazCNalTfgpXO TGCe7ZBTJXzKg== Message-ID: Date: Mon, 18 Apr 2022 04:29:30 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority Content-Language: en-US To: "Rafael J. Wysocki" Cc: Thierry Reding , Jonathan Hunter , Russell King , Catalin Marinas , Will Deacon , Guo Ren , Geert Uytterhoeven , Greg Ungerer , Joshua Thompson , Thomas Bogendoerfer , Sebastian Reichel , Linus Walleij , Philipp Zabel , Greentime Hu , Vincent Chen , "James E.J. Bottomley" , Helge Deller , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Paul Walmsley , Palmer Dabbelt , Albert Ou , Yoshinori Sato , Rich Felker , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , the arch/x86 maintainers , "H. Peter Anvin" , Boris Ostrovsky , Juergen Gross , Stefano Stabellini , Len Brown , Santosh Shilimkar , Krzysztof Kozlowski , Liam Girdwood , Mark Brown , Pavel Machek , Lee Jones , Andrew Morton , Guenter Roeck , Daniel Lezcano , Andy Shevchenko , Ulf Hansson , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Linux Kernel Mailing List , linux-csky@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org, "open list:BROADCOM NVRAM DRIVER" , linux-parisc@vger.kernel.org, linux-riscv@lists.infradead.org, Linux-sh list , xen-devel@lists.xenproject.org, ACPI Devel Maling List , Linux PM , linux-tegra References: <20220411233832.391817-1-dmitry.osipenko@collabora.com> <20220411233832.391817-4-dmitry.osipenko@collabora.com> From: Dmitry Osipenko In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org On 4/14/22 14:19, Rafael J. Wysocki wrote: > On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko > wrote: >> >> On 4/13/22 21:48, Rafael J. Wysocki wrote: >>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko >>> wrote: >>>> >>>> Add sanity check which ensures that there are no two restart handlers >>>> registered using the same priority. This requirement will become mandatory >>>> once all drivers will be converted to the new API and such errors will be >>>> fixed. >>>> >>>> Signed-off-by: Dmitry Osipenko >>> >>> The first two patches in the series are fine with me and there's only >>> one minor nit regarding this one (below). >>> >>>> --- >>>> kernel/reboot.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/kernel/reboot.c b/kernel/reboot.c >>>> index ed4e6dfb7d44..acdae4e95061 100644 >>>> --- a/kernel/reboot.c >>>> +++ b/kernel/reboot.c >>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); >>>> */ >>>> int register_restart_handler(struct notifier_block *nb) >>>> { >>>> + int ret; >>>> + >>>> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); >>>> + if (ret != -EBUSY) >>>> + return ret; >>>> + >>>> + /* >>>> + * Handler must have unique priority. Otherwise call order is >>>> + * determined by registration order, which is unreliable. >>>> + * >>>> + * This requirement will become mandatory once all drivers >>>> + * will be converted to use new sys-off API. >>>> + */ >>>> + pr_err("failed to register restart handler using unique priority\n"); >>> >>> I would use pr_info() here, because this is not a substantial error AFAICS. >> >> It's indeed not a substantial error so far, but it will become >> substantial later on once only unique priorities will be allowed. The >> pr_warn() could be a good compromise here, pr_info() is too mild, IMO. > > Well, I'm still unconvinced about requiring all of the users of this > interface to use unique priorities. > > Arguably, there are some of them who don't really care about the > ordering, so could there be an option for them to specify the lack of > care by, say, passing 0 as the priority that would be regarded as a > special case? > > IOW, if you pass 0, you'll be run along the others who've also passed > 0, but if you pass anything different from 0, it must be unique. What > do you think? There are indeed cases where ordering is unimportant. Like a case of PMIC and watchdog restart handlers for example, both handlers will produce equal effect from a user's perspective. Perhaps indeed it's more practical to have at least one shared level. In this patchset the level 0 is specified as an alias to the default level 128. If one user registers handler using unique level 128 and the other user uses non-unique level 0, then we have ambiguity. One potential option is to make the whole default level 128 non-unique. This will allow users to not care about the uniqueness by default like they always did it previously, but it will hide potential problems for users who actually need unique level and don't know about it yet due to a lucky registration ordering that they have today. Are you okay with this option? 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 1B23EC433EF for ; Mon, 18 Apr 2022 01:30:08 +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:From:References:Cc:To: Subject: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=VY49bSAI8SjUGi61ijc8ejNcK9VyZKRGQ/lGPLN+i1A=; b=qjOnJE3lL6K0/H qjPwvr26TIW4mQSzmu3iuISlLT0d/aooBw9exxVIUxuov8zfjZME1Q9YdwrUDqvX906dqIttZ/nZd aX83jmDUJ/j0us09KF7INoUTqejL3lpkwIbjSxuUdFxKxHONOQR61W9VTgZYxOBvywZ+OD7OwkP3w hBAabCcvXcijhRRgANimjz8QtzDp+VsBP+ITxlnDqlfYVxkaTEtDrDILqsP7ff3C7mQUH7aSFongF F4uP+Hsui26y6DTR54iU30vQFP7VuDJwfedtCRedzwDoZEoIl/3ny5zQiYo08tFlvovdIYN3Pk0EC TPxp1mE8fSvO8A27lS0A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ngGDC-00FDYa-Rt; Mon, 18 Apr 2022 01:29:54 +0000 Received: from bhuna.collabora.co.uk ([46.235.227.227]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ngGCy-00FDVk-At for linux-riscv@lists.infradead.org; Mon, 18 Apr 2022 01:29:42 +0000 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: dmitry.osipenko) with ESMTPSA id 77A9B1F44320 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1650245376; bh=Ajn7d9ehxuO5NknWvmo0k/aIhgzNt/YPk9MMAC0rgK4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=H6wbrzdvBv8/q0QMxBBTynYiA8MocDKNi6pPzdbeah1TBS0sfobjoYMeVHdywDKJa YOMseWFehs7fKiS1bqLxpLaRV3E2gVJ6NGmAk/kK7Tkj7SOk+d7kIA+pVboML+0znZ oBB6ZdZaJe8q4CuDbVjC8i5gSLP4M7dhUJeJhWywSpYx0y55HsGlo0NSg/sn82x813 XAZwVlD5NoEcUOWKl8ezQsrDqxJrPlOK3xiszEQs//fITbMqaQ95IAUW03Xr4Fzay/ IWbATXa2Chl0Jy2VqKMQd994g2oQEgzNvPwVOvB9VgKGF8p+Y8PjFoazCNalTfgpXO TGCe7ZBTJXzKg== Message-ID: Date: Mon, 18 Apr 2022 04:29:30 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority Content-Language: en-US To: "Rafael J. Wysocki" Cc: Thierry Reding , Jonathan Hunter , Russell King , Catalin Marinas , Will Deacon , Guo Ren , Geert Uytterhoeven , Greg Ungerer , Joshua Thompson , Thomas Bogendoerfer , Sebastian Reichel , Linus Walleij , Philipp Zabel , Greentime Hu , Vincent Chen , "James E.J. Bottomley" , Helge Deller , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Paul Walmsley , Palmer Dabbelt , Albert Ou , Yoshinori Sato , Rich Felker , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , the arch/x86 maintainers , "H. Peter Anvin" , Boris Ostrovsky , Juergen Gross , Stefano Stabellini , Len Brown , Santosh Shilimkar , Krzysztof Kozlowski , Liam Girdwood , Mark Brown , Pavel Machek , Lee Jones , Andrew Morton , Guenter Roeck , Daniel Lezcano , Andy Shevchenko , Ulf Hansson , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Linux Kernel Mailing List , linux-csky@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org, "open list:BROADCOM NVRAM DRIVER" , linux-parisc@vger.kernel.org, linux-riscv@lists.infradead.org, Linux-sh list , xen-devel@lists.xenproject.org, ACPI Devel Maling List , Linux PM , linux-tegra References: <20220411233832.391817-1-dmitry.osipenko@collabora.com> <20220411233832.391817-4-dmitry.osipenko@collabora.com> From: Dmitry Osipenko In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220417_182940_863744_CC73D34F X-CRM114-Status: GOOD ( 28.84 ) X-BeenThere: linux-riscv@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-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On 4/14/22 14:19, Rafael J. Wysocki wrote: > On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko > wrote: >> >> On 4/13/22 21:48, Rafael J. Wysocki wrote: >>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko >>> wrote: >>>> >>>> Add sanity check which ensures that there are no two restart handlers >>>> registered using the same priority. This requirement will become mandatory >>>> once all drivers will be converted to the new API and such errors will be >>>> fixed. >>>> >>>> Signed-off-by: Dmitry Osipenko >>> >>> The first two patches in the series are fine with me and there's only >>> one minor nit regarding this one (below). >>> >>>> --- >>>> kernel/reboot.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/kernel/reboot.c b/kernel/reboot.c >>>> index ed4e6dfb7d44..acdae4e95061 100644 >>>> --- a/kernel/reboot.c >>>> +++ b/kernel/reboot.c >>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); >>>> */ >>>> int register_restart_handler(struct notifier_block *nb) >>>> { >>>> + int ret; >>>> + >>>> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); >>>> + if (ret != -EBUSY) >>>> + return ret; >>>> + >>>> + /* >>>> + * Handler must have unique priority. Otherwise call order is >>>> + * determined by registration order, which is unreliable. >>>> + * >>>> + * This requirement will become mandatory once all drivers >>>> + * will be converted to use new sys-off API. >>>> + */ >>>> + pr_err("failed to register restart handler using unique priority\n"); >>> >>> I would use pr_info() here, because this is not a substantial error AFAICS. >> >> It's indeed not a substantial error so far, but it will become >> substantial later on once only unique priorities will be allowed. The >> pr_warn() could be a good compromise here, pr_info() is too mild, IMO. > > Well, I'm still unconvinced about requiring all of the users of this > interface to use unique priorities. > > Arguably, there are some of them who don't really care about the > ordering, so could there be an option for them to specify the lack of > care by, say, passing 0 as the priority that would be regarded as a > special case? > > IOW, if you pass 0, you'll be run along the others who've also passed > 0, but if you pass anything different from 0, it must be unique. What > do you think? There are indeed cases where ordering is unimportant. Like a case of PMIC and watchdog restart handlers for example, both handlers will produce equal effect from a user's perspective. Perhaps indeed it's more practical to have at least one shared level. In this patchset the level 0 is specified as an alias to the default level 128. If one user registers handler using unique level 128 and the other user uses non-unique level 0, then we have ambiguity. One potential option is to make the whole default level 128 non-unique. This will allow users to not care about the uniqueness by default like they always did it previously, but it will hide potential problems for users who actually need unique level and don't know about it yet due to a lucky registration ordering that they have today. Are you okay with this option? _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Osipenko Date: Mon, 18 Apr 2022 01:29:30 +0000 Subject: Re: [PATCH v7 03/20] reboot: Print error message if restart handler has duplicated priority Message-Id: List-Id: References: <20220411233832.391817-1-dmitry.osipenko@collabora.com> <20220411233832.391817-4-dmitry.osipenko@collabora.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Rafael J. Wysocki" Cc: Thierry Reding , Jonathan Hunter , Russell King , Catalin Marinas , Will Deacon , Guo Ren , Geert Uytterhoeven , Greg Ungerer , Joshua Thompson , Thomas Bogendoerfer , Sebastian Reichel , Linus Walleij , Philipp Zabel , Greentime Hu , Vincent Chen , "James E.J. Bottomley" , Helge Deller , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Paul Walmsley , Palmer Dabbelt , Albert Ou , Yoshinori Sato , Rich Felker , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , the arch/x86 maintainers , "H. Peter Anvin" , Boris Ostrovsky , Juergen Gross , Stefano Stabellini , Len Brown , Santosh Shilimkar , Krzysztof Kozlowski , Liam Girdwood , Mark Brown , Pavel Machek , Lee Jones , Andrew Morton , Guenter Roeck , Daniel Lezcano , Andy Shevchenko , Ulf Hansson , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Linux Kernel Mailing List , linux-csky@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org, "open list:BROADCOM NVRAM DRIVER" , linux-parisc@vger.kernel.org, linux-riscv@lists.infradead.org, Linux-sh list , xen-devel@lists.xenproject.org, ACPI Devel Maling List , Linux PM , linux-tegra On 4/14/22 14:19, Rafael J. Wysocki wrote: > On Thu, Apr 14, 2022 at 12:24 AM Dmitry Osipenko > wrote: >> >> On 4/13/22 21:48, Rafael J. Wysocki wrote: >>> On Tue, Apr 12, 2022 at 1:39 AM Dmitry Osipenko >>> wrote: >>>> >>>> Add sanity check which ensures that there are no two restart handlers >>>> registered using the same priority. This requirement will become mandatory >>>> once all drivers will be converted to the new API and such errors will be >>>> fixed. >>>> >>>> Signed-off-by: Dmitry Osipenko >>> >>> The first two patches in the series are fine with me and there's only >>> one minor nit regarding this one (below). >>> >>>> --- >>>> kernel/reboot.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>>> >>>> diff --git a/kernel/reboot.c b/kernel/reboot.c >>>> index ed4e6dfb7d44..acdae4e95061 100644 >>>> --- a/kernel/reboot.c >>>> +++ b/kernel/reboot.c >>>> @@ -182,6 +182,21 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list); >>>> */ >>>> int register_restart_handler(struct notifier_block *nb) >>>> { >>>> + int ret; >>>> + >>>> + ret = atomic_notifier_chain_register_unique_prio(&restart_handler_list, nb); >>>> + if (ret != -EBUSY) >>>> + return ret; >>>> + >>>> + /* >>>> + * Handler must have unique priority. Otherwise call order is >>>> + * determined by registration order, which is unreliable. >>>> + * >>>> + * This requirement will become mandatory once all drivers >>>> + * will be converted to use new sys-off API. >>>> + */ >>>> + pr_err("failed to register restart handler using unique priority\n"); >>> >>> I would use pr_info() here, because this is not a substantial error AFAICS. >> >> It's indeed not a substantial error so far, but it will become >> substantial later on once only unique priorities will be allowed. The >> pr_warn() could be a good compromise here, pr_info() is too mild, IMO. > > Well, I'm still unconvinced about requiring all of the users of this > interface to use unique priorities. > > Arguably, there are some of them who don't really care about the > ordering, so could there be an option for them to specify the lack of > care by, say, passing 0 as the priority that would be regarded as a > special case? > > IOW, if you pass 0, you'll be run along the others who've also passed > 0, but if you pass anything different from 0, it must be unique. What > do you think? There are indeed cases where ordering is unimportant. Like a case of PMIC and watchdog restart handlers for example, both handlers will produce equal effect from a user's perspective. Perhaps indeed it's more practical to have at least one shared level. In this patchset the level 0 is specified as an alias to the default level 128. If one user registers handler using unique level 128 and the other user uses non-unique level 0, then we have ambiguity. One potential option is to make the whole default level 128 non-unique. This will allow users to not care about the uniqueness by default like they always did it previously, but it will hide potential problems for users who actually need unique level and don't know about it yet due to a lucky registration ordering that they have today. Are you okay with this option?