[PATCH] tmscsim: fix spinlock usage in isr

[PATCH] tmscsim: fix spinlock usage in isr

[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.
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 <hare@suse.de>
---
 drivers/scsi/tmscsim.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index 7645757..2aecd2c 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -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));
-- 
1.8.5.2

Re: [PATCH] tmscsim: fix spinlock usage in isr

[James Bottomley]

On Fri, 2014-11-07 at 13:49 +0100, Hannes Reinecke wrote:
> 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.

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

James

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/tmscsim.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
> index 7645757..2aecd2c 100644
> --- a/drivers/scsi/tmscsim.c
> +++ b/drivers/scsi/tmscsim.c
> @@ -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));

Re: [PATCH] tmscsim: fix spinlock usage in isr

[Hannes Reinecke]

On 11/07/2014 02:09 PM, James Bottomley wrote:
> On Fri, 2014-11-07 at 13:49 +0100, Hannes Reinecke wrote:
>> 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

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

>> 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.
> 
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
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tmscsim: fix spinlock usage in isr

[James Bottomley]

On Fri, 2014-11-07 at 14:16 +0100, Hannes Reinecke wrote:
> On 11/07/2014 02:09 PM, James Bottomley wrote:
> > On Fri, 2014-11-07 at 13:49 +0100, Hannes Reinecke wrote:
> >> 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

OK, I'm losing track of how we handle interrupts, so this shows all
interrupt routines are now run with interrupts disabled.

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

That's fine ... irqsave/restore makes sure it always works.

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

As long as the assertion in the comment that the lock is locked is
correct it's OK (and if it weren't true, we'd have deadlocked).  All we
care about is enabling interrupts during the potentially long udelay so
the timer still ticks and the softlockup dosn't go off.

So I think the code you're removing here is doing what it's supposed to.

James

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