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@xxxxxxx>
> ---
> 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��������+%������w��{.n�����{������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
