[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 04/21] kernel: Add combined power-off+restart handler call chain API
09.01.2022 02:35, Michał Mirosław пишет: > On Mon, Dec 13, 2021 at 12:02:52AM +0300, Dmitry Osipenko wrote: > [...] >> +/** >> + * struct power_off_data - Power-off callback argument >> + * >> + * @cb_data: Callback data. >> + */ >> +struct power_off_data { >> + void *cb_data; >> +}; >> + >> +/** >> + * struct power_off_prep_data - Power-off preparation callback argument >> + * >> + * @cb_data: Callback data. >> + */ >> +struct power_off_prep_data { >> + void *cb_data; >> +}; > > Why two exactly same structures? Why only a single pointer instead? If > it just to enable type-checking callbacks, then thouse could be opaque > or zero-sized structs that would be embedded or casted away in > respective callbacks. Preparation and final execution are two different operations, it's much cleaner from a user's perspective to have same separated, IMO. In the future we may would want to extend one of the structs, and not the other. Type-checking is another benefit, of course. The single callback pointer is what is utilized by all current kernel users. This may change in the future and then it won't be a problem to extend the power-off API without disrupting whole kernel. >> + >> +/** >> + * struct restart_data - Restart callback argument >> + * >> + * @cb_data: Callback data. >> + * @cmd: Restart command string. >> + * @stop_chain: Further lower priority callbacks won't be executed if set to >> + * true. Can be changed within callback. Default is false. >> + * @mode: Reboot mode ID. >> + */ >> +struct restart_data { >> + void *cb_data; >> + const char *cmd; >> + bool stop_chain; >> + enum reboot_mode mode; >> +}; >> + >> +/** >> + * struct reboot_prep_data - Reboot and shutdown preparation callback >> argument >> + * >> + * @cb_data: Callback data. >> + * @cmd: Restart command string. >> + * @stop_chain: Further lower priority callbacks won't be executed if set to >> + * true. Can be changed within callback. Default is false. > > Why would we want to stop power-off or erboot chain? If the callback > succeded, then further calls won't be made. If it doesn't succeed, but > possibly breaks the system somehow, it shouldn't return. Then the only > case left would be to just try the next method of shutting down. This is what some of the API users were doing for years. I don't know for sure why they want to stop the chain, it indeed looks like an incorrect behaviour, but that's up to developers to decide. I only retained the old behaviour for those users. >> + * @mode: Preparation mode ID. >> + */ >> +struct reboot_prep_data { >> + void *cb_data; >> + const char *cmd; >> + bool stop_chain; >> + enum reboot_prepare_mode mode; >> +}; >> + >> +struct sys_off_handler_private_data { >> + struct notifier_block power_off_nb; >> + struct notifier_block restart_nb; >> + struct notifier_block reboot_nb; > > What's the difference between restart and reboot? Reboot is always executed before restart and power-off callbacks. This is explained in the doc-comment of the struct sys_off_handler. + * @reboot_prepare_cb: Reboot/shutdown preparation callback. All reboot + * preparation callbacks are invoked before @restart_cb or @power_off_cb, + * depending on the mode. It's registered with register_reboot_notifier(). + * The point is to remove boilerplate code from drivers which use this + * callback in conjunction with the restart/power-off callbacks. + * This reboot callback usually performs early preparations that are need to be done before machine is placed into reset state, please see [1] for the examples. [1] https://elixir.bootlin.com/linux/v5.16/A/ident/register_reboot_notifier I agree that "reboot" sounds like a misnomer. This name was coined long time ago, perhaps not worth to rename it at this point. I'm also not sure what could be a better name. >> + void (*platform_power_off_cb)(void); >> + void (*simple_power_off_cb)(void *data); >> + void *simple_power_off_cb_data; >> + bool registered; >> +}; > > BTW, I couldn't find a right description of my idea of unifying the > chains before, so let me sketch it now. > > The idea is to have a single system-off chain in which the callback > gets a mode ({QUERY_*, PREP_*, DO_*} for each of {*_REBOOT, *_POWEROFF, ...?). > The QUERY_* calls would be made in can_kernel_reboot/poweroff(): all > would be called, and if at least one returned true, then the shutdown > mode would continue. All of PREP_* would be called then. After that > all DO_* would be tried until one doesn't return (succeeded or broke > the system hard). Classic for(;;); could be a final fallback for the > case where arch/machine (lowest priority) call would return instead > of halting the system in machine-dependent way. The QUERY and PREP > stages could be combined, but I haven't thought about it enough to > see what conditions would need to be imposed on the callbacks in > that case (maybe it's not worth the trouble, since it isn't a fast > path anyway?). The goal here is to have less (duplicated) code in > kernel, but otherwise it seems equivalent to your API proposal. Michał, thank you for the review and suggestions! I'll take another look at yours proposal during this merge window, in a preparation to v6.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |