[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
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. > + > +/** > + * 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. > + * @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? > + 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. Best Regards Michał Mirosław
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |