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

[Stephen Warren <swarren@wwwdotorg.org>]

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().