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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <519E3AB7.8020103@metafoo.de>
Date:	Thu, 23 May 2013 17:50:15 +0200
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Stephen Warren <swarren@...dotorg.org>
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 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.

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


> 
> Re: the other comments about the API change: I think this can be done
> non-invasively:
> 
> static void regmap_lock_spinlock(void *__map)
> {
> 	struct regmap *map = __map;
> 	unsigned long local_flags;
> 
> 	spin_lock_irqsave(&map->spinlock, local_flags);
> 	/*
> 	 * Here, we have the lock locked, so we own the flags,
> 	 * and can write to them.
> 	 */
> 	map->spinlock_flags = local_flags;
> }
> 
> static void regmap_unlock_spinlock(void *__map, unsigned long *flags)
> {
> 	struct regmap *map = __map;
> 	spin_unlock_irqrestore(&map->spinlock, map->spinlock_flags);
> }
> 
> ... and obviously add a spinlock_flags field to struct regmap (perhaps
> start unioning the mutex and spinlock data fields there if you want to
> save space).

Hm, that might work.
--
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/