tmscsim: fix spinlock usage in isr

Loading the tmscsim driver would result in kernel splat as the interrupt service routine would only use spin_lock_irq, not spin_lock_irqsave.

And while we're at it there is not need to release the spin lock when calling udelay; it's implemented via a processor counter, not via interrupts.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/tmscsim.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

drivers/scsi/tmscsim.c +5 -10

index 7645757..2aecd2c 100644
@@ -654,6 +654,7 @@ DC390_Interrupt(void *dev_id)
     u8  phase;
     void   (*stateV)( struct dc390_acb*, struct dc390_srb*, u8 *);
     u8  istate, istatus;
+    unsigned long flags;
 
     sstatus = DC390_read8 (Scsi_Status);
     if( !(sstatus & INTERRUPT) )

@@ -665,7 +666,7 @@ DC390_Interrupt(void *dev_id)
     //dstatus = DC390_read8 (DMA_Status);
     //DC390_write32 (DMA_ScsiBusCtrl, EN_INT_ON_PCI_ABORT);
 
-    spin_lock_irq(pACB->pScsiHost->host_lock);
+    spin_lock_irqsave(pACB->pScsiHost->host_lock, flags);
 
     istate = DC390_read8 (Intern_State);
     istatus = DC390_read8 (INT_Status); /* This clears Scsi_Status, Intern_State and INT_Status ! */

@@ -736,7 +737,7 @@ DC390_Interrupt(void *dev_id)
     }
 
  unlock:
-    spin_unlock_irq(pACB->pScsiHost->host_lock);
+    spin_unlock_irqrestore(pACB->pScsiHost->host_lock, flags);
     return IRQ_HANDLED;
 }
 

@@ -770,11 +771,8 @@ dc390_DataOut_0(struct dc390_acb* pACB, struct dc390_srb* pSRB, u8 *psstatus)
 
 	    /* Function called from the ISR with the host_lock held and interrupts disabled */
 	    if (pSRB->SGToBeXferLen)
-		while (time_before(jiffies, timeout) && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE)) {
-		    spin_unlock_irq(pACB->pScsiHost->host_lock);
+		while (time_before(jiffies, timeout) && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE))
 		    udelay(50);
-		    spin_lock_irq(pACB->pScsiHost->host_lock);
-		}
 	    if (!time_before(jiffies, timeout))
 		printk (KERN_CRIT "DC390: Deadlock in DataOut_0: DMA aborted unfinished: %06x bytes remain!!\n",
 			DC390_read32 (DMA_Wk_ByteCntr));

@@ -829,11 +827,8 @@ dc390_DataIn_0(struct dc390_acb* pACB, struct dc390_srb* pSRB, u8 *psstatus)
 
 	    /* Function called from the ISR with the host_lock held and interrupts disabled */
 	    if (pSRB->SGToBeXferLen)
-		while (time_before(jiffies, timeout) && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE)) {
-		    spin_unlock_irq(pACB->pScsiHost->host_lock);
+		while (time_before(jiffies, timeout) && !((dstate = DC390_read8 (DMA_Status)) & DMA_XFER_DONE))
 		    udelay(50);
-		    spin_lock_irq(pACB->pScsiHost->host_lock);
-		}
 	    if (!time_before(jiffies, timeout)) {
 		printk (KERN_CRIT "DC390: Deadlock in DataIn_0: DMA aborted unfinished: %06x bytes remain!!\n",
 			DC390_read32 (DMA_Wk_ByteCntr));

Quoted 3 lines · Hannes Reinecke

Loading the tmscsim driver would result in kernel splat as the interrupt service routine would only use spin_lock_irq, not spin_lock_irqsave.

I don't understand the reasoning ... what kernel splat?

Linux runs interrupt routines with interrupts enabled, so you know the irq state on entry; that's why you can do spin_lock_irq ... because you know it was enabled before and now it's disabled. It only saves a couple of instructions, so there's no real point using a different pattern, but there's no reason to change it either, because it's harmless when done correctly.

Quoted 3 lines

And while we're at it there is not need to release the spin lock when calling udelay; it's implemented via a processor counter, not via interrupts.

That looks a bit incorrect: The potential timeout is jiffies + HZ, i.e. up to 1 second of udelay. If it didn't drop and retake the lock (and enable interrupts), it would spin with interrupts disabled for up to a second ... with all the badness that would result from that.

Quoted 7 lines · James Bottomley

Quoted 3 lines · Hannes Reinecke

Loading the tmscsim driver would result in kernel splat as the interrupt service routine would only use spin_lock_irq, not spin_lock_irqsave.

I don't understand the reasoning ... what kernel splat?

This one:
kernel: [ 13.748186] ------------[ cut here ]------------
kernel: [ 13.748195] WARNING: CPU: 0 PID: 214 at
kernel/irq/handle.c:147 handle_irq_event_percpu+0x18b/0x1a0()
kernel: [ 13.748200] irq 20 handler do_DC390_Interrupt+0x0/0x9d0
[tmscsim] enabled interrupts
kernel: [ 13.748214] Modules linked in: crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul glue_helper bochs_drm(+) ttm ablk_helper drm_kms_helper pvpanic pcspkr cryptd drm tmscsim(+) serio_raw i2c_i801 syscopyarea sysfillrect lpc_ich mfd_core igbvf sysimgblt 8250_fintek processor thermal_sys button dm_mod efivarfs sr_mod cdrom btrfs xor raid6_pq sd_mod crc32c_intel ahci libahci libata megaraid_sas floppy sg autofs4
kernel: [ 13.748216] CPU: 0 PID: 214 Comm: kworker/0:1H Not
tainted 3.18.0-rc2-default+ #104

Quoted 7 lines

Linux runs interrupt routines with interrupts enabled, so you know the irq state on entry; that's why you can do spin_lock_irq ... because you know it was enabled before and now it's disabled. It only saves a couple of instructions, so there's no real point using a different pattern, but there's no reason to change it either, because it's harmless when done correctly.

Well, apparently it's not done correctly, then :-)
I must admit is got the 'solution' off google, so I cannot vouch for the correctness here. The only thing I know is that with switching to the irqsave/irqrestore version the warning went away.

Quoted 4 lines

Quoted 3 lines

And while we're at it there is not need to release the spin lock when calling udelay; it's implemented via a processor counter, not via interrupts.

Quoted 5 lines

That looks a bit incorrect: The potential timeout is jiffies + HZ, i.e. up to 1 second of udelay. If it didn't drop and retake the lock (and enable interrupts), it would spin with interrupts disabled for up to a second ... with all the badness that would result from that.

Hmm. I could put them back in, but for completeness one would have to pass the irq flags taken in the outer routine to avoid lock imbalancing.

Unless we manage to figure out the real reason for the above kernel splat, in which case we can leave things in.

Cheers,

Hannes