* [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).