linux-kernel - Re: [PATCH] leds: make led_blink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140825085020.GA5071@ns3274321.ip-5-39-88.eu>
Date:	Mon, 25 Aug 2014 10:50:20 +0200
From:	Vincent Donnefort <vdonnefort@...il.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Bryan Wu <cooloney@...il.com>, Hugh Dickins <hughd@...gle.com>,
	Valdis.Kletnieks@...edu,
	Linux LED Subsystem <linux-leds@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>,
	linux-wireless@...r.kernel.org
Subject: Re: [PATCH] leds: make led_blink_set IRQ safe

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@...gle.com> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/