Hi, [no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]
https://lkml.org/lkml/2016/9/2/335 I came up with the following somewhat random thoughts: *** this treatment is exclusive to a single use case, i.e. not covering things consistently (API-wide) > +++ b/kernel/power/qos.c > @@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request > *req, > return; > } > > - cancel_delayed_work_sync(&req->work); > + /* > + * This function may be called very early during boot, for > example, > + * from of_clk_init(), where irq needs to stay disabled. > + * cancel_delayed_work_sync() assumes that irq is enabled on > + * invocation and re-enables it on return. Avoid calling it > until > + * workqueue is initialized. > + */ > + if (keventd_up()) > + cancel_delayed_work_sync(&req->work); > + > __pm_qos_update_request(req, new_value); > } > EXPORT_SYMBOL_GPL(pm_qos_update_request); Reason: any other [early-boot] invoker of cancel_delayed_work_sync() would hit the same issue, without any fix then available locally each. This may or may not be intentional. Just wanted to point it out. The more global question here obviously is how this annoying irq handling side effect ought to be handled cleanly / symmetrically (and ideally with minimal effect to hotpaths, which probably is the usual inherent outcome of an elegant/clean solution), especially given that it is early handling vs. normal. However unfortunately I do not have any final ideas here ATM. *** try_to_grab_pending() layer violation (asymmetry) It *internally* does a local_irq_save(*flags) and then passes these flags to the *external* user, where the user after having called try_to_grab_pending() then has to invoke these same *internal layer implementation details* (local_irq_restore(flags)) That's just fugly asymmetric handling. If an API layer happens to have some certain *internal* "context"/"resource" requirements [either by having an explicit API function to establish these requirements, or by "dirtily"/"implicitly" passing these out via an out parameter], then this API layer *always* should *itself* provide correspondingly required resource handling functions *at this very layer implementation* Since the API currently is named try_to_grab_pending(), this suggests that the signature of the corresponding resource cleanup function could be something like work_grab_context_release(context_reference); Not properly providing such a public API will result in e.g. the following major disadvantages: - a flood of changes necessary *at all API users* in case use protocol of these implementation details (local_irq_restore()) happen to get changed - all users of this API layer having to #include the specific header of the inner implementation details layer (local_irq_restore()) rather than the cleanly plain API layer itself only (either within the API layer header in case of an inline helper, or within the API layer implemention internally only in case of non-inline) IOW, by getting this wrong, #include handling of this header of the internal implementation layer requirements is not properly under the control of the API layer implementer any more Also, comment "try_to_grab_pending - steal work item from worklist and disable irq" seems to have wrong order since I would think that "disable irq" is the *pre-requisite* for being able to reliably (atomically) steal work items from work list. [[hmm, and let's hope that NMIs are (known to be?) not involved]] HTH, Andreas Mohr