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 6F878C433F5 for ; Mon, 13 Dec 2021 09:23:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233683AbhLMJXl convert rfc822-to-8bit (ORCPT ); Mon, 13 Dec 2021 04:23:41 -0500 Received: from mail-vk1-f179.google.com ([209.85.221.179]:38527 "EHLO mail-vk1-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233673AbhLMJXh (ORCPT ); Mon, 13 Dec 2021 04:23:37 -0500 Received: by mail-vk1-f179.google.com with SMTP id s17so9966812vka.5; Mon, 13 Dec 2021 01:23:36 -0800 (PST) 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:content-transfer-encoding; bh=7JQ6xhDuC8UGs/jm3YR6x8QoCCs8KyehtdxluDCRZKo=; b=s1vr4fKxoqXExk3EECIfYm4XnjnYCI9FntQ4nfRSKG1qDpqNF9Ln+oyQ+8AlkOmNZY 4VKiBsJuu+BAAMm6QtPPMfTP8l403QFB9Uc1hR/hLkkeUmz4NOq9DyGh796m9mNutLIJ 9dmGJIbRxcCMcWHcqcjslDyjrICWEIGHiLxUaU8S5YF6QkMz/C+jXeqnTPmQjjJqG5dJ DmyRVHmFe1uRt/Nbgr8f/fqhsR3JckM0CbT/qJL8nxByU6P2aCObAAE9k9LrHm8V4BZI O4fstT3MvC/FSeM0K6PLkHUslGnIIUAADonm70KDKHyXKuxJLmFh91rJ3HnUgrPDo6BS 4mKQ== X-Gm-Message-State: AOAM5329nOcx31Hc48LnBiGEsDoJpYJbc+mLykjJXH6bvW5Y9lmtzdhi W+mLuBR5yt3KD2Q9Z9wXEnGVb9ZOGT4H6g== X-Google-Smtp-Source: ABdhPJy36iwzFCGQ3/LfaGF4Odlhfj2OLNrCWA4IlVb15gm8DIeNRZjtFj9sldbBl2GYTgKVUkSP6A== X-Received: by 2002:a05:6122:20a7:: with SMTP id i39mr29371339vkd.15.1639387415861; Mon, 13 Dec 2021 01:23:35 -0800 (PST) Received: from mail-ua1-f49.google.com (mail-ua1-f49.google.com. [209.85.222.49]) by smtp.gmail.com with ESMTPSA id t20sm3875484vsj.27.2021.12.13.01.23.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 13 Dec 2021 01:23:35 -0800 (PST) Received: by mail-ua1-f49.google.com with SMTP id ay21so27952890uab.12; Mon, 13 Dec 2021 01:23:34 -0800 (PST) X-Received: by 2002:a67:c106:: with SMTP id d6mr26074577vsj.77.1639387403608; Mon, 13 Dec 2021 01:23:23 -0800 (PST) MIME-Version: 1.0 References: <20211126180101.27818-1-digetx@gmail.com> <20211126180101.27818-6-digetx@gmail.com> <033ddf2a-6223-1a82-ec64-30f17c891f67@gmail.com> <091321ea-4919-0579-88a8-23d05871575d@gmail.com> <45025b2d-4be1-f694-be61-31903795cf5d@gmail.com> In-Reply-To: From: Geert Uytterhoeven Date: Mon, 13 Dec 2021 10:23:12 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority To: "Rafael J. Wysocki" Cc: Dmitry Osipenko , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Thierry Reding , Jonathan Hunter , Russell King , Catalin Marinas , Will Deacon , Guo Ren , 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 , alankao@andestech.com, "K . C . Kuen-Chern Lin" , Linux ARM , Linux Kernel Mailing List , linux-csky@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linuxppc-dev , linux-riscv@lists.infradead.org, Linux-sh list , xen-devel@lists.xenproject.org, ACPI Devel Maling List , Linux PM , linux-tegra Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 10, 2021 at 8:14 PM Rafael J. Wysocki wrote: > On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko wrote: > > 10.12.2021 21:27, Rafael J. Wysocki пишет: > > > On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko wrote: > > >> 29.11.2021 03:26, Michał Mirosław пишет: > > >>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote: > > >>>> 28.11.2021 03:28, Michał Mirosław пишет: > > >>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote: > > >>>>>> Add sanity check which ensures that there are no two restart handlers > > >>>>>> registered with the same priority. Normally it's a direct sign of a > > >>>>>> problem if two handlers use the same priority. > > >>>>> > > >>>>> The patch doesn't ensure the property that there are no duplicated-priority > > >>>>> entries on the chain. > > >>>> > > >>>> It's not the exact point of this patch. > > >>>> > > >>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns > > >>>>> -EBUSY or something istead of adding an entry with duplicate priority. > > >>>>> That way it would need only one list traversal unless you want to > > >>>>> register the duplicate anyway (then you would call the older > > >>>>> atomic_notifier_chain_register() after reporting the error). > > >>>> > > >>>> The point of this patch is to warn developers about the problem that > > >>>> needs to be fixed. We already have such troubling drivers in mainline. > > >>>> > > >>>> It's not critical to register different handlers with a duplicated > > >>>> priorities, but such cases really need to be corrected. We shouldn't > > >>>> break users' machines during transition to the new API, meanwhile > > >>>> developers should take action of fixing theirs drivers. > > >>>> > > >>>>> (Or you could return > 0 when a duplicate is registered in > > >>>>> atomic_notifier_chain_register() if the callers are prepared > > >>>>> for that. I don't really like this way, though.) > > >>>> > > >>>> I had a similar thought at some point before and decided that I'm not in > > >>>> favor of this approach. It's nicer to have a dedicated function that > > >>>> verifies the uniqueness, IMO. > > >>> > > >>> I don't like the part that it traverses the list second time to check > > >>> the uniqueness. But actually you could avoid that if > > >>> notifier_chain_register() would always add equal-priority entries in > > >>> reverse order: > > >>> > > >>> static int notifier_chain_register(struct notifier_block **nl, > > >>> struct notifier_block *n) > > >>> { > > >>> while ((*nl) != NULL) { > > >>> if (unlikely((*nl) == n)) { > > >>> WARN(1, "double register detected"); > > >>> return 0; > > >>> } > > >>> - if (n->priority > (*nl)->priority) > > >>> + if (n->priority >= (*nl)->priority) > > >>> break; > > >>> nl = &((*nl)->next); > > >>> } > > >>> n->next = *nl; > > >>> rcu_assign_pointer(*nl, n); > > >>> return 0; > > >>> } > > >>> > > >>> Then the check for uniqueness after adding would be: > > >>> > > >>> WARN(nb->next && nb->priority == nb->next->priority); > > >> > > >> We can't just change the registration order because invocation order of > > >> the call chain depends on the registration order > > > > > > It doesn't if unique priorities are required and isn't that what you want? > > > > > >> and some of current > > >> users may rely on that order. I'm pretty sure that changing the order > > >> will have unfortunate consequences. > > > > > > Well, the WARN() doesn't help much then. > > > > > > Either you can make all of the users register with unique priorities, > > > and then you can make the registration reject non-unique ones, or you > > > cannot assume them to be unique. > > > > There is no strong requirement for priorities to be unique, the reboot.c > > code will work properly. > > In which case adding the WARN() is not appropriate IMV. > > Also I've looked at the existing code and at least in some cases the > order in which the notifiers run doesn't matter. I'm not sure what > the purpose of this patch is TBH. > > > The potential problem is on the user's side and the warning is intended > > to aid the user. > > Unless somebody has the panic_on_warn mentioned previously set and > really the user need not understand what the WARN() is about. IOW, > WARN() helps developers, not users. Do panic_on_warn and reboot_on_panic play well with having a WARN() in the reboot notifier handling? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds