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

[James Bottomley <jbottomley@parallels.com>]

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));