2010-07-29 13:23:03

by Kulikov Vasiliy

Subject: [bug] Fixing mutex_lock() under held spinlock

Hi,

I've found that cfi_cmdset and lpddr_cmds call mutex_lock() under held
spinlock(). Maybe it was designed as a special locking scheme, so I
don't try to fix it as I might create new complex locking problem.


--- ./drivers/mtd/chips/cfi_cmdset_0001.c 2010-07-06 16:45:43.000000000 +0400
+++ /tmp/cocci-output-8818-2d9c62-cfi_cmdset_0001.c 2010-07-29 16:30:47.000000000 +0400
@@ -959,14 +959,12 @@ static void put_chip(struct map_info *ma

if (chip->priv) {
struct flchip_shared *shared = chip->priv;
- spin_lock(&shared->lock);
if (shared->writing == chip && chip->oldstate == FL_READY) {
/* We own the ability to write, but we're done */
shared->writing = shared->erasing;
if (shared->writing && shared->writing != chip) {
/* give back ownership to who we loaned it from */
struct flchip *loaner = shared->writing;
- mutex_lock(&loaner->mutex);
spin_unlock(&shared->lock);
mutex_unlock(&chip->mutex);
put_chip(map, loaner, loaner->start);
--- ./drivers/mtd/lpddr/lpddr_cmds.c 2010-07-06 16:45:43.000000000 +0400
+++ /tmp/cocci-output-8953-56b3dd-lpddr_cmds.c 2010-07-29 16:30:57.000000000 +0400
@@ -348,14 +348,12 @@ static void put_chip(struct map_info *ma
{
if (chip->priv) {
struct flchip_shared *shared = chip->priv;
- spin_lock(&shared->lock);
if (shared->writing == chip && chip->oldstate == FL_READY) {
/* We own the ability to write, but we're done */
shared->writing = shared->erasing;
if (shared->writing && shared->writing != chip) {
/* give back the ownership */
struct flchip *loaner = shared->writing;
- mutex_lock(&loaner->mutex);
spin_unlock(&shared->lock);
mutex_unlock(&chip->mutex);
put_chip(map, loaner);


thanks,
Vasiliy

2010-07-29 13:35:24

by Arnd Bergmann

Subject: Re: [bug] Fixing mutex_lock() under held spinlock

On Thursday 29 July 2010, Vasiliy Kulikov wrote:
> I've found that cfi_cmdset and lpddr_cmds call mutex_lock() under held
> spinlock(). Maybe it was designed as a special locking scheme, so I
> don't try to fix it as I might create new complex locking problem.

No, it certainly looks like a bug and it seems to have been introduced by
http://linux.derkeiler.com/Mailing-Lists/Kernel/2010-03/msg02798.html

Back in March, Stefani wrote:
| I have analyzed this drivers and IMHO i don't think there will be used
| from irq or atomic contexts. There is no request interrupt and there are
| a lot msleep and add_wait_queues/schedule calls during holding the
| mutex, which are not very useful in a irq or atomic context. But i don't
| know the whole mtd stack.

It seems you have missed at least two places. It should be possible to
fix this by turning shared->lock into a mutex as well.

Arnd