Message-ID: <1705554.pBhZQ0Icu9@diego>
Date:	Wed, 30 Jul 2014 14:16:03 +0200
From:	Heiko Stübner <heiko@...ech.de>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	linux-watchdog@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Wim Van Sebroeck <wim@...ana.be>,
	Catalin Marinas <catalin.marinas@....com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Will Deacon <will.deacon@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Russell King <linux@....linux.org.uk>,
	Jonas Jensen <jonas.jensen@...il.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...nel.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Tomasz Figa <t.figa@...sung.com>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 0/7] kernel: Add support for restart handler call chain

Hi Guenter,

Am Dienstag, 29. Juli 2014, 18:50:47 schrieb Guenter Roeck:
> On 07/18/2014 12:34 AM, Guenter Roeck wrote:
> > Various drivers implement architecture and/or device specific means
> > to restart (reset) the system. Various mechanisms have been implemented
> > to support those schemes. The best known mechanism is arm_pm_restart,
> > which is a function pointer to be set either from platform specific code
> > or from drivers. Another mechanism is to use hardware watchdogs to issue
> > a reset; this mechanism is used if there is no other method available
> > to reset a board or system. Two examples are alim7101_wdt, which currently
> > uses the reboot notifier to trigger a reset, and moxart_wdt, which
> > registers the arm_pm_restart function. Several other restart drivers for
> > arm, all directly calling arm_pm_restart, are in the process of being
> > integrated into the kernel. All those drivers would benefit from the new
> > API.
> > 
> > The existing mechanisms have a number of drawbacks. Typically only one
> > scheme to restart the system is supported (at least if arm_pm_restart is
> > used). At least in theory there can be multiple means to restart the
> > system, some of which may be less desirable (for example one mechanism
> > may only reset the CPU, while another may reset the entire system). Using
> > arm_pm_restart can also be racy if the function pointer is set from a
> > driver, as the driver may be in the process of being unloaded when
> > arm_pm_restart is called.
> > Using the reboot notifier is always racy, as it is unknown if and when
> > other functions using the reboot notifier have completed execution
> > by the time the watchdog fires.
> > 
> > Introduce a system restart handler call chain to solve the described
> > problems. This call chain is expected to be executed from the
> > architecture specific machine_restart() function. Drivers providing
> > system restart functionality (such as the watchdog drivers mentioned
> > above) are expected to register with this call chain. By using the
> > priority field in the notifier block, callers can control restart handler
> > execution sequence and thus ensure that the restart handler with the
> > optimal restart capabilities for a given system is called first.
> > 
> > Since the first revision of this patchset, a number of separate patch
> > submissions have been made which either depend on it or could make use of
> > it.
> > 
> > http://www.spinics.net/linux/lists/arm-kernel/msg344796.html
> > 
> > 	registers three notifiers.
> > 
> > https://lkml.org/lkml/2014/7/8/962
> > 
> > 	would benefit from it.
> > 
> > Patch 1 of this series implements the restart handler function. Patches 2
> > and 3 implement calling the restart handler chain from arm and arm64
> > restart code.
> > 
> > Patch 4 modifies the restart-poweroff driver to no longer call
> > arm_pm_restart directly but machine_restart. This is done to avoid
> > calling arm_pm_restart from more than one place. The change makes the
> > driver architecture independent, so it would be possible to drop the arm
> > dependency from its Kconfig entry.
> > 
> > Patch 5 and 6 convert existing restart handlers in the watchdog subsystem
> > to use the restart handler. Patch 7 unexports arm_pm_restart to ensure
> > that no one gets the idea to implement a restart handler as module.
> > 
> > ---
> > v5: Rebased series to v3.16-rc5
> > 
> >      Function renames:
> >      register_restart_notifier -> register_restart_handler
> >      unregister_restart_notifier -> unregister_restart_handler
> >      kernel_restart_notify -> do_kernel_restart
> > 
> > v4: Document restart notifier priorities
> > 
> >      Select 128 as default priority for newly introduced notifiers
> >      Fix checkpatch warning (line too long) in moxart patch
> > 
> > v3: Drop RFC.
> > 
> >      Add kernel_restart_notify wrapper function to execute notifier
> >      Improve documentation.
> >      Move restart_notifier_list into kernel/reboot.c and make it static.
> > 
> > v2: Add patch 4.
> > 
> >      Only call blocking notifier call chain if arm_pm_restart was not set.
> > 
> > --
> 
> To get more test coverage, this series plus a few add-on patches which
> depend on it are now available in the restart-staging branch of my
> repository at kernel.org [1]. The branch is currently based on 3.16-rc7.
> Extensive build test results are available at [2]; look for the column
> marked 'restart-staging' on the far right of the tables.
> 
> I would encourage everyone interested in this series to send me Reviewed-by:
> or at least Acked-by: tags. Note that I removed all of the earlier tags
> since I feel that the changes made subsequently warrant updated tags. An
> Acked-by: from affected maintainers would also be very helpful.

Thanks for adapting my Samsung restart-patches to the API changes.

The one thing I found is, in
	"clk: samsung: register restart handlers for s3c2412 and s3c2443"
you seem to have forgotten the priority in the clk-s3c2443.c part while in 
clk-s3c2412.c it is present.

And I'm not sure if there shouldn't be some sort of delay, to give the 
watchdog some time to work, as Tomasz suggested in my initial submission?


I'll update the Rockchip specific restart-patches after the merge window, when 
the core clock support has landed.

And I still like the whole concept very much, so
Acked-by: Heiko Stuebner <heiko@...ech.de>


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists