linux-kernel - Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

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 for Android: free password hash cracker in your pocket

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

Message-ID: <519E3E8A.2010704@wwwdotorg.org>
Date:	Thu, 23 May 2013 10:06:34 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Lars-Peter Clausen <lars@...afoo.de>
CC:	Mark Brown <broonie@...nel.org>,
	Davide Ciminaghi <ciminaghi@...dd.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

On 05/23/2013 09:50 AM, Lars-Peter Clausen wrote:
> On 05/23/2013 05:42 PM, Stephen Warren wrote:
>> On 05/23/2013 07:06 AM, Lars-Peter Clausen wrote:
>>> regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for locking.
>>> Which means in order to avoid race conditions the lock always needs to be taken
>>> from the same context.
>>
>> I'm not really sure what this means. I assume contexts are
>> atomic-vs-nonatomic?
> 
> Yes.
> 
>> If so, spinlocks should work fine for this, right?
> 
> No, you have to take special care if you want to take the same spinlock from
> different contexts. And you have to take even more care if the code that
> takes the lock can run in different contexts.

OK, but what is that "special care" that must be taken? I assume from
this patch, you mean using spin_lock_irqsave() rather than spin_lock().
So the issue isn't so much that spin locks are used, but rather how
they're used. The code still has a spin_lock_t before and after...

>> I guess the core of the issue is that you want to replace spin_lock()
>> with spin_lock_irqsave(). I'd like to see that explicitly described in
>> the commit description, if that is the core aspect of this change.
> 
> Hm, it does.
> 
> 	regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for
> 	locking.
> 	...
> 	This patch updates the adds a flags parameter to the regmap lock
> 	and unlock callbacks and uses spin_lock_irqsave() and
> 	spin_unlock_restore() ...

To me, that doesn't make me realize that the core part of this change is
to switch between the two different APIs that operate on the spin lock.
I'd expect the commit description to say something more like:

regmap-mmio uses a spinlock for locking. In order for this to work
correctly from different contexts (atomic vs non-atomic), this spinlock
must be locked using spin_lock_irqsave() rather than spin_lock(). To
support this, a spinlock_flags field is added to struct regmap to store
the flags between regmap_{,un}lock_spinlock().
--
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/