On Sat, Aug 23, 2014 at 01:24:56PM -0400, Tejun Heo wrote: > Hello, > > On Fri, Aug 22, 2014 at 05:21:30PM -0700, Bryan Wu wrote: > > On Tue, Aug 19, 2014 at 6:51 PM, Hugh Dickins <hughd@xxxxxxxxxx> wrote: > > > On Tue, 19 Aug 2014, Vincent Donnefort wrote: > > > > > >> This patch introduces a work which take care of reseting the blink workqueue and > > >> avoid calling the cancel_delayed_work_sync function which may sleep, from an IRQ > > >> context. > > >> > > > > Vincent, I'm just wandering can we use cancel_delayed_work() instead > > of sync version here. > > cancel_delayed_work() can be called from IRQ context. > > But it doesn't wait for the currently running one to finish. It > should wait, right? > It should indeed wait. Using the cancel_delayed_work() function was my first thought, however as described into the function header, when calling cancel_delayed_work(), the work may be running and since it re-arms itself, the next work may still be queued. > > > May I (most ungratefully!) say that your patch doesn't fill me with > > > confidence that it's the right solution: adding yet another work_struct > > > to get around the issue seemed dubious to me, I wonder if it might expose > > > new races. > > > > I agree with Hugh about this new cancel work_struct. But if we revert > > it back, I saw led_blink_set() will call del_timer_sync() which might > > also sleep and can't be used in IRQ context. Looks like we can't call > > led_blink_set() in any IRQ/atomic context. > > del_timer_sync() doesn't block but it does run from bh. It naturally > can't be waited from an IRQ context. It may be sitting on top of a > running instance. > > > > But rest assured that I know nothing about this, and I'm not at all > > > qualified to review your patch: I hope Bryan and others will do so. > > > > Let me invite Tejun to give some advice on how to solve this problem. > > > > Tejun, Vincent's commit 8b37e1bef5a6b60e949e28a4db3006e4b00bd758 > > convert a timer into work_struct, but Hugh found it will cause > > sleeping BUGs [1]. Could you give some opinion about that? > > Not knowing the code base, I'm not sure how helpful I can be but if > something wants to synchronize against another thing which can block, > that something needs to be able to block too. There's no way around > it and it holds the same for timers. As long as all the work items > are properly shut down at the end, there's nothing wrong with using > multiple of them even when they form a dependency chain. > > Heh, I think I need more specific questions to be actually useful. :) > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
