* [PATCH] tmscsim: spin_unlock_irq in interrupt handler fix
@ 2012-07-21 6:48 Denis Efremov
2012-07-31 8:21 ` Guennadi Liakhovetski
2012-07-31 11:53 ` [PATCH v2] " Denis Efremov
0 siblings, 2 replies; 4+ messages in thread
From: Denis Efremov @ 2012-07-21 6:48 UTC (permalink / raw)
To: Kurt Garloff
Cc: Denis Efremov, Guennadi Liakhovetski, James E.J. Bottomley,
linux-scsi, linux-kernel
The replacement of spin_lock_irq/spin_unlock_irq pair in interrupt
handler by spin_lock_irqsave/spin_lock_irqrestore pair.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>
---
drivers/scsi/tmscsim.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index a1baccc..0b9d68a 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;
}
--
1.7.7
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] tmscsim: spin_unlock_irq in interrupt handler fix
2012-07-21 6:48 [PATCH] tmscsim: spin_unlock_irq in interrupt handler fix Denis Efremov
@ 2012-07-31 8:21 ` Guennadi Liakhovetski
2012-07-31 11:53 ` [PATCH v2] " Denis Efremov
1 sibling, 0 replies; 4+ messages in thread
From: Guennadi Liakhovetski @ 2012-07-31 8:21 UTC (permalink / raw)
To: Denis Efremov
Cc: Kurt Garloff, James E.J. Bottomley, linux-scsi, linux-kernel
Wow, been ages since the last patch to this driver has hit my mail:-)
On Sat, 21 Jul 2012, Denis Efremov wrote:
> The replacement of spin_lock_irq/spin_unlock_irq pair in interrupt
> handler by spin_lock_irqsave/spin_lock_irqrestore pair.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>
Right, unconditionally enabling local IRQs in an ISR seems rude. BUT:
unfortunately, it's not as easy as this. From DC390_Interrupt()
dc390_DataOut_0() and dc390_DataIn_0() can be called, which also use
spin_lock_irq() / spin_unlock_irq()... So, maybe adding a flags field to
struct dc390_acb and using it on all 3 occasions would be the easiest and
safest fix...
Thanks
Guennadi
> ---
> drivers/scsi/tmscsim.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
> index a1baccc..0b9d68a 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;
> }
>
> --
> 1.7.7
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] tmscsim: spin_unlock_irq in interrupt handler fix
2012-07-21 6:48 [PATCH] tmscsim: spin_unlock_irq in interrupt handler fix Denis Efremov
2012-07-31 8:21 ` Guennadi Liakhovetski
@ 2012-07-31 11:53 ` Denis Efremov
2012-08-03 23:23 ` Guennadi Liakhovetski
1 sibling, 1 reply; 4+ messages in thread
From: Denis Efremov @ 2012-07-31 11:53 UTC (permalink / raw)
To: Kurt Garloff
Cc: Denis Efremov, Guennadi Liakhovetski, James E.J. Bottomley,
linux-scsi, linux-kernel
The replacement of spin_lock_irq/spin_unlock_irq pairs which
can be called from interrupt handler by irqsave/irqrestore
versions.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>
---
drivers/scsi/tmscsim.c | 12 ++++++------
drivers/scsi/tmscsim.h | 1 +
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index a1baccc..e9b7148 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -665,7 +665,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, pACB->hlock_flags);
istate = DC390_read8 (Intern_State);
istatus = DC390_read8 (INT_Status); /* This clears Scsi_Status, Intern_State and INT_Status ! */
@@ -736,7 +736,7 @@ DC390_Interrupt(void *dev_id)
}
unlock:
- spin_unlock_irq(pACB->pScsiHost->host_lock);
+ spin_unlock_irqrestore(pACB->pScsiHost->host_lock, pACB->hlock_flags);
return IRQ_HANDLED;
}
@@ -771,9 +771,9 @@ 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);
+ spin_unlock_irqrestore(pACB->pScsiHost->host_lock, pACB->hlock_flags);
udelay(50);
- spin_lock_irq(pACB->pScsiHost->host_lock);
+ spin_lock_irqsave(pACB->pScsiHost->host_lock, pACB->hlock_flags);
}
if (!time_before(jiffies, timeout))
printk (KERN_CRIT "DC390: Deadlock in DataOut_0: DMA aborted unfinished: %06x bytes remain!!\n",
@@ -830,9 +830,9 @@ 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);
+ spin_unlock_irqrestore(pACB->pScsiHost->host_lock, pACB->hlock_flags);
udelay(50);
- spin_lock_irq(pACB->pScsiHost->host_lock);
+ spin_lock_irqsave(pACB->pScsiHost->host_lock, pACB->hlock_flags);
}
if (!time_before(jiffies, timeout)) {
printk (KERN_CRIT "DC390: Deadlock in DataIn_0: DMA aborted unfinished: %06x bytes remain!!\n",
diff --git a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
index 77adc54..3f9ea2b 100644
--- a/drivers/scsi/tmscsim.h
+++ b/drivers/scsi/tmscsim.h
@@ -107,6 +107,7 @@ u8 SyncOffset; /*;for reg. and nego.(low nibble) */
struct dc390_acb
{
struct Scsi_Host *pScsiHost;
+unsigned long hlock_flags;
u16 IOPortBase;
u8 IRQLevel;
u8 status;
--
1.7.7
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tmscsim: spin_unlock_irq in interrupt handler fix
2012-07-31 11:53 ` [PATCH v2] " Denis Efremov
@ 2012-08-03 23:23 ` Guennadi Liakhovetski
0 siblings, 0 replies; 4+ messages in thread
From: Guennadi Liakhovetski @ 2012-08-03 23:23 UTC (permalink / raw)
To: Denis Efremov
Cc: Kurt Garloff, James E.J. Bottomley, linux-scsi, linux-kernel
On Tue, 31 Jul 2012, Denis Efremov wrote:
> The replacement of spin_lock_irq/spin_unlock_irq pairs which
> can be called from interrupt handler by irqsave/irqrestore
> versions.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>
Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Note: I didn't test this. I still have a dc390 card in my PC, so, I could
test it, but I haven't yet got time for this and I'll be away the next
week. The patch looks correct and safe. If it does break anything, well,
we'll get to know about that sooner or later...
Thanks
Guennadi
> ---
> drivers/scsi/tmscsim.c | 12 ++++++------
> drivers/scsi/tmscsim.h | 1 +
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
> index a1baccc..e9b7148 100644
> --- a/drivers/scsi/tmscsim.c
> +++ b/drivers/scsi/tmscsim.c
> @@ -665,7 +665,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, pACB->hlock_flags);
>
> istate = DC390_read8 (Intern_State);
> istatus = DC390_read8 (INT_Status); /* This clears Scsi_Status, Intern_State and INT_Status ! */
> @@ -736,7 +736,7 @@ DC390_Interrupt(void *dev_id)
> }
>
> unlock:
> - spin_unlock_irq(pACB->pScsiHost->host_lock);
> + spin_unlock_irqrestore(pACB->pScsiHost->host_lock, pACB->hlock_flags);
> return IRQ_HANDLED;
> }
>
> @@ -771,9 +771,9 @@ 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);
> + spin_unlock_irqrestore(pACB->pScsiHost->host_lock, pACB->hlock_flags);
> udelay(50);
> - spin_lock_irq(pACB->pScsiHost->host_lock);
> + spin_lock_irqsave(pACB->pScsiHost->host_lock, pACB->hlock_flags);
> }
> if (!time_before(jiffies, timeout))
> printk (KERN_CRIT "DC390: Deadlock in DataOut_0: DMA aborted unfinished: %06x bytes remain!!\n",
> @@ -830,9 +830,9 @@ 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);
> + spin_unlock_irqrestore(pACB->pScsiHost->host_lock, pACB->hlock_flags);
> udelay(50);
> - spin_lock_irq(pACB->pScsiHost->host_lock);
> + spin_lock_irqsave(pACB->pScsiHost->host_lock, pACB->hlock_flags);
> }
> if (!time_before(jiffies, timeout)) {
> printk (KERN_CRIT "DC390: Deadlock in DataIn_0: DMA aborted unfinished: %06x bytes remain!!\n",
> diff --git a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
> index 77adc54..3f9ea2b 100644
> --- a/drivers/scsi/tmscsim.h
> +++ b/drivers/scsi/tmscsim.h
> @@ -107,6 +107,7 @@ u8 SyncOffset; /*;for reg. and nego.(low nibble) */
> struct dc390_acb
> {
> struct Scsi_Host *pScsiHost;
> +unsigned long hlock_flags;
> u16 IOPortBase;
> u8 IRQLevel;
> u8 status;
> --
> 1.7.7
>
> --
> 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
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-08-03 23:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-21 6:48 [PATCH] tmscsim: spin_unlock_irq in interrupt handler fix Denis Efremov
2012-07-31 8:21 ` Guennadi Liakhovetski
2012-07-31 11:53 ` [PATCH v2] " Denis Efremov
2012-08-03 23:23 ` Guennadi Liakhovetski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).