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 <[email protected]> > --- > 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)); N�����r��y����b�X��ǧv�^�){.n�+����{���"�{ay� ʇڙ�,j��f���h���z� �w��� ���j:+v���w�j�m��������zZ+�����ݢj"��!�i
