Skip to contentSkip to conversation

staging: slicoss: restore IRQs correctly after slic_cmdq_reset()

PATCH In review Dan Carpenter 2 participants 2 messages May 19, 2015 – May 26, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging: slicoss: restore IRQs correctly after slic_cmdq_reset()

We can't save two different values in "flags" so it means that IRQs are not enabled properly at the end of this function. This isn't a problem in the current code because it's always called with IRQs disabled so we don't want to enable them at the end.

This bug is old but it's thanks to David Matlack's recent cleanups that Smatch can detect it.

---
The code prior to David Matlack's change was way worse because everyone shared the same IRQ flags. It's hard to imagine how it worked at all.

drivers/staging/slicoss/slicoss.c +2 -2
index 5f34ebbf..a609f3e 100644
@@ -1392,7 +1392,7 @@ static void slic_cmdq_reset(struct adapter *adapter)
 	unsigned long flags;
 
 	spin_lock_irqsave(&adapter->cmdq_free.lock, flags);
-	spin_lock_irqsave(&adapter->cmdq_done.lock, flags);
+	spin_lock(&adapter->cmdq_done.lock);
 	outstanding = adapter->cmdq_all.count - adapter->cmdq_done.count;
 	outstanding -= adapter->cmdq_free.count;
 	hcmd = adapter->cmdq_all.head;
@@ -1423,7 +1423,7 @@ static void slic_cmdq_reset(struct adapter *adapter)
 			"free_count %d != all count %d\n",
 			adapter->cmdq_free.count, adapter->cmdq_all.count);
 	}
-	spin_unlock_irqrestore(&adapter->cmdq_done.lock, flags);
+	spin_unlock(&adapter->cmdq_done.lock);
 	spin_unlock_irqrestore(&adapter->cmdq_free.lock, flags);
 }
 
Reviewed-by on v1 1/1
#1 lore raw
Quoted 4 lines · Dan Carpenter

We can't save two different values in "flags" so it means that IRQs are not enabled properly at the end of this function. This isn't a problem in the current code because it's always called with IRQs disabled so we don't want to enable them at the end.

Nice find. Thanks for fixing it!

Quoted 5 lines

This bug is old but it's thanks to David Matlack's recent cleanups that Smatch can detect it.

Quoted 26 lines

---
The code prior to David Matlack's change was way worse because everyone shared the same IRQ flags. It's hard to imagine how it worked at all.

drivers/staging/slicoss/slicoss.c +2 -2
index 5f34ebbf..a609f3e 100644
@@ -1392,7 +1392,7 @@ static void slic_cmdq_reset(struct adapter *adapter)
        unsigned long flags;
 
        spin_lock_irqsave(&adapter->cmdq_free.lock, flags);
-       spin_lock_irqsave(&adapter->cmdq_done.lock, flags);
+       spin_lock(&adapter->cmdq_done.lock);
        outstanding = adapter->cmdq_all.count - adapter->cmdq_done.count;
        outstanding -= adapter->cmdq_free.count;
        hcmd = adapter->cmdq_all.head;
@@ -1423,7 +1423,7 @@ static void slic_cmdq_reset(struct adapter *adapter)
                        "free_count %d != all count %d\n",
                        adapter->cmdq_free.count, adapter->cmdq_all.count);
        }
-       spin_unlock_irqrestore(&adapter->cmdq_done.lock, flags);
+       spin_unlock(&adapter->cmdq_done.lock);
        spin_unlock_irqrestore(&adapter->cmdq_free.lock, flags);
 }