linux-kernel - Re: [PATCH 1/2] firewire: core: no need to track irq flags in bm

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: <20100622142649.GA18061@frolo.macqel>
Date:	Tue, 22 Jun 2010 16:26:49 +0200
From:	Philippe De Muyter <phdm@...qel.be>
To:	Stefan Richter <stefanr@...6.in-berlin.de>
Cc:	linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] firewire: core: no need to track irq flags in
	bm_work

Hello Stephan,

On Tue, Jun 22, 2010 at 01:43:26PM +0200, Stefan Richter wrote:
> Philippe De Muyter wrote:
> > On Mon, Jun 21, 2010 at 11:23:52PM +0200, Stefan Richter wrote:
> >> This is a workqueue job and always entered with IRQs enabled.
> > 
> > did you mean 'disabled' ?
> 
> I meant enabled.
> 
> [...]
> >> @@ -247,10 +246,10 @@ static void fw_card_bm_work(struct work_
> >>  	bool root_device_is_cmc;
> >>  	bool irm_is_1394_1995_only;
> >>  
> >> -	spin_lock_irqsave(&card->lock, flags);
> >> +	spin_lock_irq(&card->lock);
> 
>   - spin_lock + spin_unlock don't influence whether IRQs on the current
>     CPU are on or off.
> 
>   - spin_lock_irq + spin_unlock_irq always switch IRQs on the current
>     CPU off and back on.  This is necessary if the lock could also be
>     taken by an IRQ handler.  (Well, card->lock is actually only taken
>     by process contexts and by tasklets.  Seems we could switch to
>     spin_lock_bh + spin_unlock_bh for card->lock everywhere in the
>     firewire stack.)
> 
>   - spin_lock_irqsave + spin_unlock_irqrestore switch IRQs on the
>     current CPU off and back on only if used while IRQs are enabled;
>     if used while local IRQs are already disabled they leave them
>     disabled.
> 
> http://lwn.net/images/pdf/LDD3/ch05.pdf#page=14
> 
> Therefore some people prefer to use the safer spin_lock_irqsave()/
> spin_unlock_irqrestore() everywhere.  However, their downsides are the
> need to track IRQ state flags, and --- subjectively --- that their
> appearance in the code could create an impression to a casual reader
> that this code was meant to be able to run in IRQs-on context as well as
> in IRQs-off context.  fw_card_bm_work() however definitely requires to
> be called with IRQs on, notably to be able to wait for IEEE 1394
> transactions to complete.

Thanks for the clear explanation, and sorry for your wasted time.

Philippe
--
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/