Messages in this thread | | | Date | Wed, 28 Apr 2010 00:02:36 -0700 | Subject | Re: [PATCH 6/8] PM: Add suspend blocking work. | From | Arve Hjønnevåg <> |
| |
2010/4/27 Tejun Heo <tj@kernel.org>: > Hello, > >> +static void suspend_blocking_work_complete(struct suspend_blocking_work *work) >> +{ >> + unsigned long flags; >> + >> + WARN_ON(!work->active); >> + spin_lock_irqsave(&work->lock, flags); >> + if (!--work->active) >> + suspend_unblock(&work->suspend_blocker); >> + spin_unlock_irqrestore(&work->lock, flags); >> +} > > Maybe work->active can be an atomic_t and the lock can be removed? >
I need the spinlock to prevent the work from getting re-queued before suspend_unblock.
>> +/** >> + * suspend_blocking_work_destroy - Destroy suspend_blocking_work >> + * @work: The work item in question >> + * >> + * If the work was ever queued on more then one workqueue all but the last >> + * workqueue must be flushed before calling suspend_blocking_work_destroy. > > As it's calling cancel_work_sync(), the above is not true. As long as > no one is trying to queue it again, suspend_blocking_work_destroy() is > safe to call regardless of how the work has been used. >
I'm not sure what the best terminology is here, but cancel_work_sync() only waits for work running on all the cpu-workqueues of the last workqueue. So, if the caller queued the work on more than one workqueue, suspend_blocking_work_destroy does not ensure that the suspend_blocking_work structure is not still in use (it should trigger the WARN_ON though).
>> +void suspend_blocking_work_destroy(struct suspend_blocking_work *work) >> +{ >> + cancel_suspend_blocking_work_sync(work); >> + WARN_ON(work->active); >> + suspend_blocker_destroy(&work->suspend_blocker); >> +} >> +EXPORT_SYMBOL(suspend_blocking_work_destroy); > > Other than the above, it looks good to me. > > Thanks. > > -- > tejun >
-- Arve Hjønnevåg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |