* [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. @ 2012-02-26 13:45 santosh nayak 2012-02-26 15:07 ` Dan Carpenter 2012-03-08 13:32 ` Mark Salyzyn 0 siblings, 2 replies; 11+ messages in thread From: santosh nayak @ 2012-02-26 13:45 UTC (permalink / raw) To: jack_wang Cc: lindar_liu, JBottomley, linux-scsi, linux-kernel, kernel-janitors, Santosh Nayak From: Santosh Nayak <santoshprasadnayak@gmail.com> Static checker is giving following warning: " error: calling 'spin_unlock_irqrestore()' with bogus flags" The code flow is as shown below: process_oq() --> process_one_iomb --> mpi_sata_completion In 'mpi_sata_completion' the first call for 'spin_unlock_irqrestore()' is with flags=0, which is as good as 'spin_unlock_irq()' ( unconditional interrupt enabling). So for better performance 'spin_unlock_irqrestore()' can be replaced with 'spin_unlock_irq()' and 'spin_lock_irqsave()' can be replaced by 'spin_lock_irq()'. Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com> --- drivers/scsi/pm8001/pm8001_hwi.c | 58 ++++++++++++++++++------------------- 1 files changed, 28 insertions(+), 30 deletions(-) diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c index 833e720..838e3e2 100644 --- a/drivers/scsi/pm8001/pm8001_hwi.c +++ b/drivers/scsi/pm8001/pm8001_hwi.c @@ -1877,7 +1877,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) { struct sas_task *t; struct pm8001_ccb_info *ccb; - unsigned long flags = 0; u32 param; u32 status; u32 tag; @@ -2016,9 +2015,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_QUEUE_FULL; pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*in order to force CPU ordering*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); return; } break; @@ -2036,9 +2035,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_QUEUE_FULL; pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); return; } break; @@ -2064,9 +2063,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_QUEUE_FULL; pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/* ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); return; } break; @@ -2131,9 +2130,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_QUEUE_FULL; pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); return; } break; @@ -2155,9 +2154,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_QUEUE_FULL; pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); return; } break; @@ -2175,31 +2174,31 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_DEV_NO_RESPONSE; break; } - spin_lock_irqsave(&t->task_state_lock, flags); + spin_lock_irq(&t->task_state_lock); t->task_state_flags &= ~SAS_TASK_STATE_PENDING; t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; t->task_state_flags |= SAS_TASK_STATE_DONE; if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { - spin_unlock_irqrestore(&t->task_state_lock, flags); + spin_unlock_irq(&t->task_state_lock); PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("task 0x%p done with io_status 0x%x" " resp 0x%x stat 0x%x but aborted by upper layer!\n", t, status, ts->resp, ts->stat)); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } else if (t->uldd_task) { - spin_unlock_irqrestore(&t->task_state_lock, flags); + spin_unlock_irq(&t->task_state_lock); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/* ditto */ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); } else if (!t->uldd_task) { - spin_unlock_irqrestore(&t->task_state_lock, flags); + spin_unlock_irq(&t->task_state_lock); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); } } @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) { struct sas_task *t; - unsigned long flags = 0; struct task_status_struct *ts; struct pm8001_ccb_info *ccb; struct pm8001_device *pm8001_dev; @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) ts->stat = SAS_QUEUE_FULL; pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); return; } break; @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) ts->stat = SAS_OPEN_TO; break; } - spin_lock_irqsave(&t->task_state_lock, flags); + spin_lock_irq(&t->task_state_lock); t->task_state_flags &= ~SAS_TASK_STATE_PENDING; t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; t->task_state_flags |= SAS_TASK_STATE_DONE; if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { - spin_unlock_irqrestore(&t->task_state_lock, flags); + spin_unlock_irq(&t->task_state_lock); PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("task 0x%p done with io_status 0x%x" " resp 0x%x stat 0x%x but aborted by upper layer!\n", t, event, ts->resp, ts->stat)); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } else if (t->uldd_task) { - spin_unlock_irqrestore(&t->task_state_lock, flags); + spin_unlock_irq(&t->task_state_lock); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/* ditto */ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); } else if (!t->uldd_task) { - spin_unlock_irqrestore(&t->task_state_lock, flags); + spin_unlock_irq(&t->task_state_lock); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ - spin_unlock_irqrestore(&pm8001_ha->lock, flags); + spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); + spin_lock_irq(&pm8001_ha->lock); } } -- 1.7.4.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-02-26 13:45 [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue santosh nayak @ 2012-02-26 15:07 ` Dan Carpenter 2012-02-27 4:58 ` santosh prasad nayak 2012-03-08 13:32 ` Mark Salyzyn 1 sibling, 1 reply; 11+ messages in thread From: Dan Carpenter @ 2012-02-26 15:07 UTC (permalink / raw) To: santosh nayak Cc: jack_wang, lindar_liu, JBottomley, linux-scsi, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 1295 bytes --] On Sun, Feb 26, 2012 at 07:03:30PM +0530, santosh nayak wrote: > From: Santosh Nayak <santoshprasadnayak@gmail.com> > > Static checker is giving following warning: > " error: calling 'spin_unlock_irqrestore()' with bogus flags" > > The code flow is as shown below: > process_oq() --> process_one_iomb --> mpi_sata_completion > > In 'mpi_sata_completion' > the first call for 'spin_unlock_irqrestore()' is with flags=0, > which is as good as 'spin_unlock_irq()' ( unconditional interrupt > enabling). > > So for better performance 'spin_unlock_irqrestore()' can be replaced > with 'spin_unlock_irq()' and 'spin_lock_irqsave()' can be replaced by > 'spin_lock_irq()'. > process_oq() is called from the interrupt handler pm8001_chip_isr() with interrupts disabled. drivers/scsi/pm8001/pm8001_hwi.c 4301 spin_lock_irqsave(&pm8001_ha->lock, flags); 4302 pm8001_chip_interrupt_disable(pm8001_ha); 4303 process_oq(pm8001_ha); 4304 pm8001_chip_interrupt_enable(pm8001_ha); 4305 spin_unlock_irqrestore(&pm8001_ha->lock, flags); Probably we should just be doing a spin_lock() and spin_unlock() without re-enabling the IRQs. Should we even be doing that in the irq handler anyway? regards, dan carpenter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-02-26 15:07 ` Dan Carpenter @ 2012-02-27 4:58 ` santosh prasad nayak 2012-02-27 6:55 ` Dan Carpenter 2012-02-27 8:29 ` Jack Wang 0 siblings, 2 replies; 11+ messages in thread From: santosh prasad nayak @ 2012-02-27 4:58 UTC (permalink / raw) To: Dan Carpenter Cc: jack_wang, lindar_liu, JBottomley, linux-scsi, linux-kernel, kernel-janitors In 'mpi_sata_completion' the first call for 'spin_unlock_irqrestore()' is with flags=0, which is as good as 'spin_unlock_irq()' ( unconditional interrupt enabling). If intention of the developer is to enable the interrupt during execution of ' mpi_sata_completion' , then the code changes in the patch looks ok. If interrupt should not be enabled during execution of 'mpi_sata_completion' then we can use simple spin_lock and spin_unlock. regards santosh On Sun, Feb 26, 2012 at 8:37 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Sun, Feb 26, 2012 at 07:03:30PM +0530, santosh nayak wrote: >> From: Santosh Nayak <santoshprasadnayak@gmail.com> >> >> Static checker is giving following warning: >> " error: calling 'spin_unlock_irqrestore()' with bogus flags" >> >> The code flow is as shown below: >> process_oq() --> process_one_iomb --> mpi_sata_completion >> >> In 'mpi_sata_completion' >> the first call for 'spin_unlock_irqrestore()' is with flags=0, >> which is as good as 'spin_unlock_irq()' ( unconditional interrupt >> enabling). >> >> So for better performance 'spin_unlock_irqrestore()' can be replaced >> with 'spin_unlock_irq()' and 'spin_lock_irqsave()' can be replaced by >> 'spin_lock_irq()'. >> > > process_oq() is called from the interrupt handler pm8001_chip_isr() > with interrupts disabled. > > drivers/scsi/pm8001/pm8001_hwi.c > 4301 spin_lock_irqsave(&pm8001_ha->lock, flags); > 4302 pm8001_chip_interrupt_disable(pm8001_ha); > 4303 process_oq(pm8001_ha); > 4304 pm8001_chip_interrupt_enable(pm8001_ha); > 4305 spin_unlock_irqrestore(&pm8001_ha->lock, flags); > > Probably we should just be doing a spin_lock() and spin_unlock() > without re-enabling the IRQs. Should we even be doing that in the > irq handler anyway? > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-02-27 4:58 ` santosh prasad nayak @ 2012-02-27 6:55 ` Dan Carpenter 2012-02-27 8:29 ` Jack Wang 1 sibling, 0 replies; 11+ messages in thread From: Dan Carpenter @ 2012-02-27 6:55 UTC (permalink / raw) To: santosh prasad nayak Cc: jack_wang, lindar_liu, JBottomley, linux-scsi, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 135 bytes --] Sorry, I misread the code. I thought we were trying to nest spin_lock_irq() for but we're not. My mistake. regards, dan carpenter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-02-27 4:58 ` santosh prasad nayak 2012-02-27 6:55 ` Dan Carpenter @ 2012-02-27 8:29 ` Jack Wang 1 sibling, 0 replies; 11+ messages in thread From: Jack Wang @ 2012-02-27 8:29 UTC (permalink / raw) To: 'santosh prasad nayak', 'Dan Carpenter' Cc: lindar_liu, JBottomley, linux-scsi, linux-kernel, kernel-janitors Thanks for fix. Acked-by: Jack Wang <jack_wang@usish.com> > > In 'mpi_sata_completion' > the first call for 'spin_unlock_irqrestore()' is with flags=0, > which is as good as 'spin_unlock_irq()' ( unconditional interrupt > enabling). If intention of the developer is to enable the interrupt during > execution of ' mpi_sata_completion' , then the code changes in the patch > looks ok. > > If interrupt should not be enabled during execution of > 'mpi_sata_completion' then > we can use simple spin_lock and spin_unlock. > > > regards > santosh > > > On Sun, Feb 26, 2012 at 8:37 PM, Dan Carpenter <dan.carpenter@oracle.com> > wrote: > > On Sun, Feb 26, 2012 at 07:03:30PM +0530, santosh nayak wrote: > >> From: Santosh Nayak <santoshprasadnayak@gmail.com> > >> > >> Static checker is giving following warning: > >> " error: calling 'spin_unlock_irqrestore()' with bogus flags" > >> > >> The code flow is as shown below: > >> process_oq() --> process_one_iomb --> mpi_sata_completion > >> > >> In 'mpi_sata_completion' > >> the first call for 'spin_unlock_irqrestore()' is with flags=0, > >> which is as good as 'spin_unlock_irq()' ( unconditional interrupt > >> enabling). > >> > >> So for better performance 'spin_unlock_irqrestore()' can be replaced > >> with 'spin_unlock_irq()' and 'spin_lock_irqsave()' can be replaced by > >> 'spin_lock_irq()'. > >> > > > > process_oq() is called from the interrupt handler pm8001_chip_isr() > > with interrupts disabled. > > > > drivers/scsi/pm8001/pm8001_hwi.c > > 4301 spin_lock_irqsave(&pm8001_ha->lock, flags); > > 4302 pm8001_chip_interrupt_disable(pm8001_ha); > > 4303 process_oq(pm8001_ha); > > 4304 pm8001_chip_interrupt_enable(pm8001_ha); > > 4305 spin_unlock_irqrestore(&pm8001_ha->lock, flags); > > > > Probably we should just be doing a spin_lock() and spin_unlock() > > without re-enabling the IRQs. Should we even be doing that in the > > irq handler anyway? > > > > regards, > > dan carpenter > > -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-02-26 13:45 [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue santosh nayak 2012-02-26 15:07 ` Dan Carpenter @ 2012-03-08 13:32 ` Mark Salyzyn 2012-03-08 14:15 ` jack_wang 2012-03-08 16:51 ` santosh prasad nayak 1 sibling, 2 replies; 11+ messages in thread From: Mark Salyzyn @ 2012-03-08 13:32 UTC (permalink / raw) To: santosh nayak Cc: Mark Salyzyn, Jack Wang, lindar_liu, James Bottomley, linux-scsi, linux-kernel, kernel-janitors, Dan Carpenter NAK In the fragment below, the 'spin_lock_irqsave(&t->task_state_lock, flags);' is 100% legitimate. The change you did was not inert, and result in the IRQ being disabled upon exit should a SAS_TASK_STATE_ABORTED task state condition occur. I propose (annotated below) you leave the flags automatic variable, but uninitialized. The changes surrounding spin_*lock_irq*(&pm8001_ha->lock, flags) are OK, but revert back the changes surrounding the spin_*lock_irq*(&t->task->state_lock,flags) so that lock would be properly nested. To be pedantic, and to be more correct, this code should have spawned a work queue task to perform the t->task_done(t) operation in a separate thread rather than inline and precariously unlocked; but that would be beyond the scope of these changes and should be left for future work to decide if it is even necessary. Not sure how such a change would affect performance (using the work queue) ... so I am 'afraid' of pushing such a change in any case given the relatively reliable operation of this driver (and the dynamic nature of the changes Dan is making to the SATA side of things for instance ;-} ). I am sorry for taking so long to get to the task of reviewing this (and the previous) code. I look forward to your comments. Sincerely -- Mark Salyzyn On Feb 26, 2012, at 8:33 AM, santosh nayak wrote: > @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) > static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) > { > struct sas_task *t; > - unsigned long flags = 0; MGS> unsigned long flags; > struct task_status_struct *ts; > struct pm8001_ccb_info *ccb; > struct pm8001_device *pm8001_dev; > @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) > ts->stat = SAS_QUEUE_FULL; > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/*ditto*/ > - spin_unlock_irqrestore(&pm8001_ha->lock, flags); > + spin_unlock_irq(&pm8001_ha->lock); > t->task_done(t); > - spin_lock_irqsave(&pm8001_ha->lock, flags); > + spin_lock_irq(&pm8001_ha->lock); > return; > } > break; > @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) > ts->stat = SAS_OPEN_TO; > break; > } > - spin_lock_irqsave(&t->task_state_lock, flags); > + spin_lock_irq(&t->task_state_lock); MGS> spin_lock_irqsave(&t->task_state_lock, flags); > t->task_state_flags &= ~SAS_TASK_STATE_PENDING; > t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; > t->task_state_flags |= SAS_TASK_STATE_DONE; > if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > + spin_unlock_irq(&t->task_state_lock); MGS> spin_unlock_irqrestore(&t->task_state_lock, flags); > PM8001_FAIL_DBG(pm8001_ha, > pm8001_printk("task 0x%p done with io_status 0x%x" > " resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, event, ts->resp, ts->stat)); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else if (t->uldd_task) { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > + spin_unlock_irq(&t->task_state_lock); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/* ditto */ > - spin_unlock_irqrestore(&pm8001_ha->lock, flags); > + spin_unlock_irq(&pm8001_ha->lock); > t->task_done(t); > - spin_lock_irqsave(&pm8001_ha->lock, flags); > + spin_lock_irq(&pm8001_ha->lock); > } else if (!t->uldd_task) { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > + spin_unlock_irq(&t->task_state_lock); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/*ditto*/ > - spin_unlock_irqrestore(&pm8001_ha->lock, flags); > + spin_unlock_irq(&pm8001_ha->lock); > t->task_done(t); > - spin_lock_irqsave(&pm8001_ha->lock, flags); > + spin_lock_irq(&pm8001_ha->lock); > } > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-03-08 13:32 ` Mark Salyzyn @ 2012-03-08 14:15 ` jack_wang 2012-03-08 16:51 ` santosh prasad nayak 1 sibling, 0 replies; 11+ messages in thread From: jack_wang @ 2012-03-08 14:15 UTC (permalink / raw) To: santosh nayak Cc: Mark Salyzyn, lindar_liu, James Bottomley, linux-scsi, linux-kernel, kernel-janitors, Dan Carpenter VGhhbmtzIE1hcmsgZm9yIGxvb2sgaW50byB0aGlzLllvdSdyZSByaWdodC4gVGhlIGNoYW5nZSBm b3IgdGFza19zdGF0ZV9sb2NrIGlzIG5vdCByaWdodC4NClNvcnJ5IGZvciBub3QgbG9vayBjYXJl ZnVsbHkgYWJvdXQgdGhlIGNoYW5nZS4NCg0KDQoNCi0tLS0tLS0tLS0tLS0tDQpqYWNrX3dhbmcN Cj5OQUsNCj4NCj5JbiB0aGUgZnJhZ21lbnQgYmVsb3csIHRoZSAnc3Bpbl9sb2NrX2lycXNhdmUo JnQtPnRhc2tfc3RhdGVfbG9jaywgZmxhZ3MpOycgaXMgMTAwJSBsZWdpdGltYXRlLiBUaGUgY2hh bmdlIHlvdSBkaWQgd2FzIG5vdCBpbmVydCwgYW5kIHJlc3VsdCBpbiB0aGUgSVJRIGJlaW5nIGRp c2FibGVkIHVwb24gZXhpdCBzaG91bGQgYSBTQVNfVEFTS19TVEFURV9BQk9SVEVEIHRhc2sgc3Rh dGUgY29uZGl0aW9uIG9jY3VyLg0KPg0KPkkgcHJvcG9zZSAoYW5ub3RhdGVkIGJlbG93KSB5b3Ug bGVhdmUgdGhlIGZsYWdzIGF1dG9tYXRpYyB2YXJpYWJsZSwgYnV0IHVuaW5pdGlhbGl6ZWQuIFRo ZSBjaGFuZ2VzIHN1cnJvdW5kaW5nIHNwaW5fKmxvY2tfaXJxKigmcG04MDAxX2hhLT5sb2NrLCBm bGFncykgYXJlIE9LLCBidXQgcmV2ZXJ0IGJhY2sgdGhlIGNoYW5nZXMgc3Vycm91bmRpbmcgdGhl IHNwaW5fKmxvY2tfaXJxKigmdC0+dGFzay0+c3RhdGVfbG9jayxmbGFncykgc28gdGhhdCBsb2Nr IHdvdWxkIGJlIHByb3Blcmx5IG5lc3RlZC4NCj4NCj5UbyBiZSBwZWRhbnRpYywgYW5kIHRvIGJl IG1vcmUgY29ycmVjdCwgdGhpcyBjb2RlIHNob3VsZCBoYXZlIHNwYXduZWQgYSB3b3JrIHF1ZXVl IHRhc2sgdG8gcGVyZm9ybSB0aGUgdC0+dGFza19kb25lKHQpIG9wZXJhdGlvbiBpbiBhIHNlcGFy YXRlIHRocmVhZCByYXRoZXIgdGhhbiBpbmxpbmUgYW5kIHByZWNhcmlvdXNseSB1bmxvY2tlZDsg YnV0IHRoYXQgd291bGQgYmUgYmV5b25kIHRoZSBzY29wZSBvZiB0aGVzZSBjaGFuZ2VzIGFuZCBz aG91bGQgYmUgbGVmdCBmb3IgZnV0dXJlIHdvcmsgdG8gZGVjaWRlIGlmIGl0IGlzIGV2ZW4gbmVj ZXNzYXJ5LiBOb3Qgc3VyZSBob3cgc3VjaCBhIGNoYW5nZSB3b3VsZCBhZmZlY3QgcGVyZm9ybWFu Y2UgKHVzaW5nIHRoZSB3b3JrIHF1ZXVlKSAuLi4gc28gSSBhbSAnYWZyYWlkJyBvZiBwdXNoaW5n IHN1Y2ggYSBjaGFuZ2UgaW4gYW55IGNhc2UgZ2l2ZW4gdGhlIHJlbGF0aXZlbHkgcmVsaWFibGUg b3BlcmF0aW9uIG9mIHRoaXMgZHJpdmVyIChhbmQgdGhlIGR5bmFtaWMgbmF0dXJlIG9mIHRoZSBj aGFuZ2VzIERhbiBpcyBtYWtpbmcgdG8gdGhlIFNBVEEgc2lkZSBvZiB0aGluZ3MgZm9yIGluc3Rh bmNlIDstfSApLg0KPg0KPkkgYW0gc29ycnkgZm9yIHRha2luZyBzbyBsb25nIHRvIGdldCB0byB0 aGUgdGFzayBvZiByZXZpZXdpbmcgdGhpcyAoYW5kIHRoZSBwcmV2aW91cykgY29kZS4gSSBsb29r IGZvcndhcmQgdG8geW91ciBjb21tZW50cy4NCj4NCj5TaW5jZXJlbHkgLS0gTWFyayBTYWx5enlu DQo+DQo+T24gRmViIDI2LCAyMDEyLCBhdCA4OjMzIEFNLCBzYW50b3NoIG5heWFrIHdyb3RlOg0K Pg0KPj4gQEAgLTIyMDcsNyArMjIwNiw2IEBAIG1waV9zYXRhX2NvbXBsZXRpb24oc3RydWN0IHBt ODAwMV9oYmFfaW5mbyAqcG04MDAxX2hhLCB2b2lkICpwaW9tYikNCj4+IHN0YXRpYyB2b2lkIG1w aV9zYXRhX2V2ZW50KHN0cnVjdCBwbTgwMDFfaGJhX2luZm8gKnBtODAwMV9oYSAsIHZvaWQgKnBp b21iKQ0KPj4gew0KPj4gCXN0cnVjdCBzYXNfdGFzayAqdDsNCj4+IC0JdW5zaWduZWQgbG9uZyBm bGFncyA9IDA7DQo+TUdTPgl1bnNpZ25lZCBsb25nIGZsYWdzOw0KPj4gCXN0cnVjdCB0YXNrX3N0 YXR1c19zdHJ1Y3QgKnRzOw0KPj4gCXN0cnVjdCBwbTgwMDFfY2NiX2luZm8gKmNjYjsNCj4+IAlz dHJ1Y3QgcG04MDAxX2RldmljZSAqcG04MDAxX2RldjsNCj4+IEBAIC0yMjg3LDkgKzIyODUsOSBA QCBzdGF0aWMgdm9pZCBtcGlfc2F0YV9ldmVudChzdHJ1Y3QgcG04MDAxX2hiYV9pbmZvICpwbTgw MDFfaGEgLCB2b2lkICpwaW9tYikNCj4+IAkJCXRzLT5zdGF0ID0gU0FTX1FVRVVFX0ZVTEw7DQo+ PiAJCQlwbTgwMDFfY2NiX3Rhc2tfZnJlZShwbTgwMDFfaGEsIHQsIGNjYiwgdGFnKTsNCj4+IAkJ CW1iKCk7LypkaXR0byovDQo+PiAtCQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmcG04MDAxX2hh LT5sb2NrLCBmbGFncyk7DQo+PiArCQkJc3Bpbl91bmxvY2tfaXJxKCZwbTgwMDFfaGEtPmxvY2sp Ow0KPj4gCQkJdC0+dGFza19kb25lKHQpOw0KPj4gLQkJCXNwaW5fbG9ja19pcnFzYXZlKCZwbTgw MDFfaGEtPmxvY2ssIGZsYWdzKTsNCj4+ICsJCQlzcGluX2xvY2tfaXJxKCZwbTgwMDFfaGEtPmxv Y2spOw0KPj4gCQkJcmV0dXJuOw0KPj4gCQl9DQo+PiAJCWJyZWFrOw0KPj4gQEAgLTIzODcsMzEg KzIzODUsMzEgQEAgc3RhdGljIHZvaWQgbXBpX3NhdGFfZXZlbnQoc3RydWN0IHBtODAwMV9oYmFf aW5mbyAqcG04MDAxX2hhICwgdm9pZCAqcGlvbWIpDQo+PiAJCXRzLT5zdGF0ID0gU0FTX09QRU5f VE87DQo+PiAJCWJyZWFrOw0KPj4gCX0NCj4+IC0Jc3Bpbl9sb2NrX2lycXNhdmUoJnQtPnRhc2tf c3RhdGVfbG9jaywgZmxhZ3MpOw0KPj4gKwlzcGluX2xvY2tfaXJxKCZ0LT50YXNrX3N0YXRlX2xv Y2spOw0KPk1HUz4Jc3Bpbl9sb2NrX2lycXNhdmUoJnQtPnRhc2tfc3RhdGVfbG9jaywgZmxhZ3Mp Ow0KPj4gCXQtPnRhc2tfc3RhdGVfZmxhZ3MgJj0gflNBU19UQVNLX1NUQVRFX1BFTkRJTkc7DQo+ PiAJdC0+dGFza19zdGF0ZV9mbGFncyAmPSB+U0FTX1RBU0tfQVRfSU5JVElBVE9SOw0KPj4gCXQt PnRhc2tfc3RhdGVfZmxhZ3MgfD0gU0FTX1RBU0tfU1RBVEVfRE9ORTsNCj4+IAlpZiAodW5saWtl bHkoKHQtPnRhc2tfc3RhdGVfZmxhZ3MgJiBTQVNfVEFTS19TVEFURV9BQk9SVEVEKSkpIHsNCj4+ IC0JCXNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJnQtPnRhc2tfc3RhdGVfbG9jaywgZmxhZ3MpOw0K Pj4gKwkJc3Bpbl91bmxvY2tfaXJxKCZ0LT50YXNrX3N0YXRlX2xvY2spOw0KPk1HUz4JCXNwaW5f dW5sb2NrX2lycXJlc3RvcmUoJnQtPnRhc2tfc3RhdGVfbG9jaywgZmxhZ3MpOw0KPj4gCQlQTTgw MDFfRkFJTF9EQkcocG04MDAxX2hhLA0KPj4gCQkJcG04MDAxX3ByaW50aygidGFzayAweCVwIGRv bmUgd2l0aCBpb19zdGF0dXMgMHgleCINCj4+IAkJCSIgcmVzcCAweCV4IHN0YXQgMHgleCBidXQg YWJvcnRlZCBieSB1cHBlciBsYXllciFcbiIsDQo+PiAJCQl0LCBldmVudCwgdHMtPnJlc3AsIHRz LT5zdGF0KSk7DQo+PiAJCXBtODAwMV9jY2JfdGFza19mcmVlKHBtODAwMV9oYSwgdCwgY2NiLCB0 YWcpOw0KPj4gCX0gZWxzZSBpZiAodC0+dWxkZF90YXNrKSB7DQo+PiAtCQlzcGluX3VubG9ja19p cnFyZXN0b3JlKCZ0LT50YXNrX3N0YXRlX2xvY2ssIGZsYWdzKTsNCj4+ICsJCXNwaW5fdW5sb2Nr X2lycSgmdC0+dGFza19zdGF0ZV9sb2NrKTsNCj4+IAkJcG04MDAxX2NjYl90YXNrX2ZyZWUocG04 MDAxX2hhLCB0LCBjY2IsIHRhZyk7DQo+PiAJCW1iKCk7LyogZGl0dG8gKi8NCj4+IC0JCXNwaW5f dW5sb2NrX2lycXJlc3RvcmUoJnBtODAwMV9oYS0+bG9jaywgZmxhZ3MpOw0KPj4gKwkJc3Bpbl91 bmxvY2tfaXJxKCZwbTgwMDFfaGEtPmxvY2spOw0KPj4gCQl0LT50YXNrX2RvbmUodCk7DQo+PiAt CQlzcGluX2xvY2tfaXJxc2F2ZSgmcG04MDAxX2hhLT5sb2NrLCBmbGFncyk7DQo+PiArCQlzcGlu X2xvY2tfaXJxKCZwbTgwMDFfaGEtPmxvY2spOw0KPj4gCX0gZWxzZSBpZiAoIXQtPnVsZGRfdGFz aykgew0KPj4gLQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmdC0+dGFza19zdGF0ZV9sb2NrLCBm bGFncyk7DQo+PiArCQlzcGluX3VubG9ja19pcnEoJnQtPnRhc2tfc3RhdGVfbG9jayk7DQo+PiAJ CXBtODAwMV9jY2JfdGFza19mcmVlKHBtODAwMV9oYSwgdCwgY2NiLCB0YWcpOw0KPj4gCQltYigp Oy8qZGl0dG8qLw0KPj4gLQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmcG04MDAxX2hhLT5sb2Nr LCBmbGFncyk7DQo+PiArCQlzcGluX3VubG9ja19pcnEoJnBtODAwMV9oYS0+bG9jayk7DQo+PiAJ CXQtPnRhc2tfZG9uZSh0KTsNCj4+IC0JCXNwaW5fbG9ja19pcnFzYXZlKCZwbTgwMDFfaGEtPmxv Y2ssIGZsYWdzKTsNCj4+ICsJCXNwaW5fbG9ja19pcnEoJnBtODAwMV9oYS0+bG9jayk7DQo+PiAJ fQ0KPj4gfQ0KPg0KPi0tDQo+VG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhl IGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LXNjc2kiIGluDQo+dGhlIGJvZHkgb2YgYSBtZXNzYWdl IHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcNCj5Nb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBo dHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCj4NCj5fX19fX19fX19f IEluZm9ybWF0aW9uIGZyb20gRVNFVCBOT0QzMiBBbnRpdmlydXMsIHZlcnNpb24gb2YgdmlydXMg c2lnbmF0dXJlIGRhdGFiYXNlIDU2NTkgKDIwMTAxMTI5KSBfX19fX19fX19fDQo+DQo+VGhlIG1l c3NhZ2Ugd2FzIGNoZWNrZWQgYnkgRVNFVCBOT0QzMiBBbnRpdmlydXMuDQo+DQo+aHR0cDovL3d3 dy5lc2V0LmNvbQ0KPg0KPg0KPg= ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-03-08 13:32 ` Mark Salyzyn 2012-03-08 14:15 ` jack_wang @ 2012-03-08 16:51 ` santosh prasad nayak 2012-03-08 17:11 ` Mark Salyzyn 1 sibling, 1 reply; 11+ messages in thread From: santosh prasad nayak @ 2012-03-08 16:51 UTC (permalink / raw) To: Mark Salyzyn Cc: Jack Wang, lindar_liu, James Bottomley, linux-scsi, linux-kernel, kernel-janitors, Dan Carpenter Hi Mark, Thanks for your review. Few queries: 1. Similar changes have been made in mpi_sata_completion() surrounding spin_*lock_irq*(&t->task->state_lock) Should those changes also need to revert back ? > > The change you did was not inert, and result in the IRQ being disabled upon exit should a > SAS_TASK_STATE_ABORTED task state condition occur then following block will enable IRQ. 2. I could not get this point. If "SAS_TASK_STATE_ABORTED" task state condition occurs then following block will enable IRQ. if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { spin_unlock_irq(&t->task_state_lock); // HERE IRQ will be enabled. ....... pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } 3. How bad will be "spin_lock_irq / spin_unlock_irq" in this case compared to "spin_lock_irqsave / spin_unlock_irqrestore " Regards Santosh > > I propose (annotated below) you leave the flags automatic variable, but uninitialized. The changes surrounding spin_*lock_irq*(&pm8001_ha->lock, flags) are OK, but revert back the changes surrounding the spin_*lock_irq*(&t->task->state_lock,flags) so that lock would be properly nested. > > To be pedantic, and to be more correct, this code should have spawned a work queue task to perform the t->task_done(t) operation in a separate thread rather than inline and precariously unlocked; but that would be beyond the scope of these changes and should be left for future work to decide if it is even necessary. Not sure how such a change would affect performance (using the work queue) ... so I am 'afraid' of pushing such a change in any case given the relatively reliable operation of this driver (and the dynamic nature of the changes Dan is making to the SATA side of things for instance ;-} ). > > I am sorry for taking so long to get to the task of reviewing this (and the previous) code. I look forward to your comments. > > Sincerely -- Mark Salyzyn > > On Feb 26, 2012, at 8:33 AM, santosh nayak wrote: > >> @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) >> static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >> { >> struct sas_task *t; >> - unsigned long flags = 0; > MGS> unsigned long flags; >> struct task_status_struct *ts; >> struct pm8001_ccb_info *ccb; >> struct pm8001_device *pm8001_dev; >> @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >> ts->stat = SAS_QUEUE_FULL; >> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >> mb();/*ditto*/ >> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >> + spin_unlock_irq(&pm8001_ha->lock); >> t->task_done(t); >> - spin_lock_irqsave(&pm8001_ha->lock, flags); >> + spin_lock_irq(&pm8001_ha->lock); >> return; >> } >> break; >> @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >> ts->stat = SAS_OPEN_TO; >> break; >> } >> - spin_lock_irqsave(&t->task_state_lock, flags); >> + spin_lock_irq(&t->task_state_lock); > MGS> spin_lock_irqsave(&t->task_state_lock, flags); >> t->task_state_flags &= ~SAS_TASK_STATE_PENDING; >> t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; >> t->task_state_flags |= SAS_TASK_STATE_DONE; >> if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { >> - spin_unlock_irqrestore(&t->task_state_lock, flags); >> + spin_unlock_irq(&t->task_state_lock); > MGS> spin_unlock_irqrestore(&t->task_state_lock, flags); >> PM8001_FAIL_DBG(pm8001_ha, >> pm8001_printk("task 0x%p done with io_status 0x%x" >> " resp 0x%x stat 0x%x but aborted by upper layer!\n", >> t, event, ts->resp, ts->stat)); >> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >> } else if (t->uldd_task) { >> - spin_unlock_irqrestore(&t->task_state_lock, flags); >> + spin_unlock_irq(&t->task_state_lock); >> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >> mb();/* ditto */ >> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >> + spin_unlock_irq(&pm8001_ha->lock); >> t->task_done(t); >> - spin_lock_irqsave(&pm8001_ha->lock, flags); >> + spin_lock_irq(&pm8001_ha->lock); >> } else if (!t->uldd_task) { >> - spin_unlock_irqrestore(&t->task_state_lock, flags); >> + spin_unlock_irq(&t->task_state_lock); >> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >> mb();/*ditto*/ >> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >> + spin_unlock_irq(&pm8001_ha->lock); >> t->task_done(t); >> - spin_lock_irqsave(&pm8001_ha->lock, flags); >> + spin_lock_irq(&pm8001_ha->lock); >> } >> } > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-03-08 16:51 ` santosh prasad nayak @ 2012-03-08 17:11 ` Mark Salyzyn 2012-03-08 20:49 ` santosh prasad nayak 0 siblings, 1 reply; 11+ messages in thread From: Mark Salyzyn @ 2012-03-08 17:11 UTC (permalink / raw) To: santosh prasad nayak Cc: Mark Salyzyn, Jack Wang, lindar_liu, James Bottomley, linux-scsi, linux-kernel, kernel-janitors, Dan Carpenter Comments embedded On Mar 8, 2012, at 11:50 AM, santosh prasad nayak wrote: > Hi Mark, > > Thanks for your review. > > Few queries: > > 1. Similar changes have been made in mpi_sata_completion() surrounding > spin_*lock_irq*(&t->task->state_lock) > Should those changes also need to revert back ? I was talking generically, yes, in both set of functions, everything associated with the task_state_lock. The change as described needs to be applied to both sets of functions. >> The change you did was not inert, and result in the IRQ being disabled upon exit should a >> SAS_TASK_STATE_ABORTED task state condition occur then following block will enable IRQ. > > 2. I could not get this point. > If "SAS_TASK_STATE_ABORTED" task state condition occurs then following > block will enable IRQ. > > if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { > spin_unlock_irq(&t->task_state_lock); > // HERE IRQ will be enabled. > ....... > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } Exactly, the IRQ is enabled, then the routine exits. All other paths leave the IRQ disabled (with a spin_lock_irq()). > 3. How bad will be "spin_lock_irq / spin_unlock_irq" in this case compared to > "spin_lock_irqsave / spin_unlock_irqrestore " Efficiency is not so much the requirement, but correctness. Yes, a spin_unlock(&t->task_state_lock) will do, but the 'world' likes to see the balance of flag save/restore, one should NOT make the assumption the interrupt is enabled or disabled upon entry, and should preserve such upon exit. Given this statement, perhaps for the case of the hacks surrounding the completion handler calls and host locks, one would do a save of the irq, unlock with irq enabled, lock, and then restore it back to the value upon entry. This is balanced. Your changes make the (correct) assumption that the irq is disabled upon entry. But pedantically incorrect, I'd take the extra steps, but I do not view that as a critical deficiency in your patch. > Regards > Santosh > >> >> I propose (annotated below) you leave the flags automatic variable, but uninitialized. The changes surrounding spin_*lock_irq*(&pm8001_ha->lock, flags) are OK, but revert back the changes surrounding the spin_*lock_irq*(&t->task->state_lock,flags) so that lock would be properly nested. >> >> To be pedantic, and to be more correct, this code should have spawned a work queue task to perform the t->task_done(t) operation in a separate thread rather than inline and precariously unlocked; but that would be beyond the scope of these changes and should be left for future work to decide if it is even necessary. Not sure how such a change would affect performance (using the work queue) ... so I am 'afraid' of pushing such a change in any case given the relatively reliable operation of this driver (and the dynamic nature of the changes Dan is making to the SATA side of things for instance ;-} ). >> >> I am sorry for taking so long to get to the task of reviewing this (and the previous) code. I look forward to your comments. >> >> Sincerely -- Mark Salyzyn >> >> On Feb 26, 2012, at 8:33 AM, santosh nayak wrote: >> >>> @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) >>> static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>> { >>> struct sas_task *t; >>> - unsigned long flags = 0; >> MGS> unsigned long flags; >>> struct task_status_struct *ts; >>> struct pm8001_ccb_info *ccb; >>> struct pm8001_device *pm8001_dev; >>> @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>> ts->stat = SAS_QUEUE_FULL; >>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>> mb();/*ditto*/ >>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>> + spin_unlock_irq(&pm8001_ha->lock); >>> t->task_done(t); >>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>> + spin_lock_irq(&pm8001_ha->lock); >>> return; >>> } >>> break; >>> @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>> ts->stat = SAS_OPEN_TO; >>> break; >>> } >>> - spin_lock_irqsave(&t->task_state_lock, flags); >>> + spin_lock_irq(&t->task_state_lock); >> MGS> spin_lock_irqsave(&t->task_state_lock, flags); >>> t->task_state_flags &= ~SAS_TASK_STATE_PENDING; >>> t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; >>> t->task_state_flags |= SAS_TASK_STATE_DONE; >>> if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { >>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>> + spin_unlock_irq(&t->task_state_lock); >> MGS> spin_unlock_irqrestore(&t->task_state_lock, flags); >>> PM8001_FAIL_DBG(pm8001_ha, >>> pm8001_printk("task 0x%p done with io_status 0x%x" >>> " resp 0x%x stat 0x%x but aborted by upper layer!\n", >>> t, event, ts->resp, ts->stat)); >>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>> } else if (t->uldd_task) { >>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>> + spin_unlock_irq(&t->task_state_lock); >>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>> mb();/* ditto */ >>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>> + spin_unlock_irq(&pm8001_ha->lock); >>> t->task_done(t); >>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>> + spin_lock_irq(&pm8001_ha->lock); >>> } else if (!t->uldd_task) { >>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>> + spin_unlock_irq(&t->task_state_lock); >>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>> mb();/*ditto*/ >>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>> + spin_unlock_irq(&pm8001_ha->lock); >>> t->task_done(t); >>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>> + spin_lock_irq(&pm8001_ha->lock); >>> } >>> } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-03-08 17:11 ` Mark Salyzyn @ 2012-03-08 20:49 ` santosh prasad nayak 2012-03-08 21:16 ` Mark Salyzyn 0 siblings, 1 reply; 11+ messages in thread From: santosh prasad nayak @ 2012-03-08 20:49 UTC (permalink / raw) To: Mark Salyzyn Cc: Jack Wang, lindar_liu, James Bottomley, linux-scsi, linux-kernel, kernel-janitors, Dan Carpenter I did changes as per Mark's suggestion. The following patch is only for review not a formal one. After getting reviewed, I will send a formal patch diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c index 3619f6e..9d82ee5 100644 --- a/drivers/scsi/pm8001/pm8001_hwi.c +++ b/drivers/scsi/pm8001/pm8001_hwi.c @@ -2093,6 +2093,7 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) struct ata_task_resp *resp ; u32 *sata_resp; struct pm8001_device *pm8001_dev; + unsigned long flags; psataPayload = (struct sata_completion_resp *)(piomb + 4); status = le32_to_cpu(psataPayload->status); @@ -2382,26 +2383,26 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_DEV_NO_RESPONSE; break; } - spin_lock_irq(&t->task_state_lock); + spin_lock_irqsave(&t->task_state_lock, flags); t->task_state_flags &= ~SAS_TASK_STATE_PENDING; t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; t->task_state_flags |= SAS_TASK_STATE_DONE; if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { - spin_unlock_irq(&t->task_state_lock); + spin_unlock_irqrestore(&t->task_state_lock, flags); PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("task 0x%p done with io_status 0x%x" " resp 0x%x stat 0x%x but aborted by upper layer!\n", t, status, ts->resp, ts->stat)); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } else if (t->uldd_task) { - spin_unlock_irq(&t->task_state_lock); + spin_unlock_irqrestore(&t->task_state_lock, flags); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/* ditto */ spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); spin_lock_irq(&pm8001_ha->lock); } else if (!t->uldd_task) { - spin_unlock_irq(&t->task_state_lock); + spin_unlock_irqrestore(&t->task_state_lock, flags); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ spin_unlock_irq(&pm8001_ha->lock); @@ -2423,6 +2424,7 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) u32 tag = le32_to_cpu(psataPayload->tag); u32 port_id = le32_to_cpu(psataPayload->port_id); u32 dev_id = le32_to_cpu(psataPayload->device_id); + unsigned long flags; ccb = &pm8001_ha->ccb_info[tag]; t = ccb->task; @@ -2593,26 +2595,26 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) ts->stat = SAS_OPEN_TO; break; } - spin_lock_irq(&t->task_state_lock); + spin_lock_irqsave(&t->task_state_lock, flags); t->task_state_flags &= ~SAS_TASK_STATE_PENDING; t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; t->task_state_flags |= SAS_TASK_STATE_DONE; if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { - spin_unlock_irq(&t->task_state_lock); + spin_unlock_irqrestore(&t->task_state_lock, flags); PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("task 0x%p done with io_status 0x%x" " resp 0x%x stat 0x%x but aborted by upper layer!\n", t, event, ts->resp, ts->stat)); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } else if (t->uldd_task) { - spin_unlock_irq(&t->task_state_lock); + spin_unlock_irqrestore(&t->task_state_lock, flags); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/* ditto */ spin_unlock_irq(&pm8001_ha->lock); t->task_done(t); spin_lock_irq(&pm8001_ha->lock); } else if (!t->uldd_task) { - spin_unlock_irq(&t->task_state_lock); + spin_unlock_irqrestore(&t->task_state_lock, flags); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); mb();/*ditto*/ spin_unlock_irq(&pm8001_ha->lock); Regards Santosh On Thu, Mar 8, 2012 at 10:41 PM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote: > Comments embedded > > On Mar 8, 2012, at 11:50 AM, santosh prasad nayak wrote: > >> Hi Mark, >> >> Thanks for your review. >> >> Few queries: >> >> 1. Similar changes have been made in mpi_sata_completion() surrounding >> spin_*lock_irq*(&t->task->state_lock) >> Should those changes also need to revert back ? > > I was talking generically, yes, in both set of functions, everything associated with the task_state_lock. The change as described needs to be applied to both sets of functions. > >>> The change you did was not inert, and result in the IRQ being disabled upon exit should a >>> SAS_TASK_STATE_ABORTED task state condition occur then following block will enable IRQ. >> >> 2. I could not get this point. >> If "SAS_TASK_STATE_ABORTED" task state condition occurs then following >> block will enable IRQ. >> >> if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { >> spin_unlock_irq(&t->task_state_lock); >> // HERE IRQ will be enabled. >> ....... >> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >> } > > Exactly, the IRQ is enabled, then the routine exits. All other paths leave the IRQ disabled (with a spin_lock_irq()). > >> 3. How bad will be "spin_lock_irq / spin_unlock_irq" in this case compared to >> "spin_lock_irqsave / spin_unlock_irqrestore " > > Efficiency is not so much the requirement, but correctness. Yes, a spin_unlock(&t->task_state_lock) will do, but the 'world' likes to see the balance of flag save/restore, one should NOT make the assumption the interrupt is enabled or disabled upon entry, and should preserve such upon exit. > > Given this statement, perhaps for the case of the hacks surrounding the completion handler calls and host locks, one would do a save of the irq, unlock with irq enabled, lock, and then restore it back to the value upon entry. This is balanced. Your changes make the (correct) assumption that the irq is disabled upon entry. But pedantically incorrect, I'd take the extra steps, but I do not view that as a critical deficiency in your patch. > >> Regards >> Santosh >> >>> >>> I propose (annotated below) you leave the flags automatic variable, but uninitialized. The changes surrounding spin_*lock_irq*(&pm8001_ha->lock, flags) are OK, but revert back the changes surrounding the spin_*lock_irq*(&t->task->state_lock,flags) so that lock would be properly nested. >>> >>> To be pedantic, and to be more correct, this code should have spawned a work queue task to perform the t->task_done(t) operation in a separate thread rather than inline and precariously unlocked; but that would be beyond the scope of these changes and should be left for future work to decide if it is even necessary. Not sure how such a change would affect performance (using the work queue) ... so I am 'afraid' of pushing such a change in any case given the relatively reliable operation of this driver (and the dynamic nature of the changes Dan is making to the SATA side of things for instance ;-} ). >>> >>> I am sorry for taking so long to get to the task of reviewing this (and the previous) code. I look forward to your comments. >>> >>> Sincerely -- Mark Salyzyn >>> >>> On Feb 26, 2012, at 8:33 AM, santosh nayak wrote: >>> >>>> @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) >>>> static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>>> { >>>> struct sas_task *t; >>>> - unsigned long flags = 0; >>> MGS> unsigned long flags; >>>> struct task_status_struct *ts; >>>> struct pm8001_ccb_info *ccb; >>>> struct pm8001_device *pm8001_dev; >>>> @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>>> ts->stat = SAS_QUEUE_FULL; >>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>> mb();/*ditto*/ >>>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>>> + spin_unlock_irq(&pm8001_ha->lock); >>>> t->task_done(t); >>>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>>> + spin_lock_irq(&pm8001_ha->lock); >>>> return; >>>> } >>>> break; >>>> @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>>> ts->stat = SAS_OPEN_TO; >>>> break; >>>> } >>>> - spin_lock_irqsave(&t->task_state_lock, flags); >>>> + spin_lock_irq(&t->task_state_lock); >>> MGS> spin_lock_irqsave(&t->task_state_lock, flags); >>>> t->task_state_flags &= ~SAS_TASK_STATE_PENDING; >>>> t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; >>>> t->task_state_flags |= SAS_TASK_STATE_DONE; >>>> if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { >>>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>>> + spin_unlock_irq(&t->task_state_lock); >>> MGS> spin_unlock_irqrestore(&t->task_state_lock, flags); >>>> PM8001_FAIL_DBG(pm8001_ha, >>>> pm8001_printk("task 0x%p done with io_status 0x%x" >>>> " resp 0x%x stat 0x%x but aborted by upper layer!\n", >>>> t, event, ts->resp, ts->stat)); >>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>> } else if (t->uldd_task) { >>>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>>> + spin_unlock_irq(&t->task_state_lock); >>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>> mb();/* ditto */ >>>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>>> + spin_unlock_irq(&pm8001_ha->lock); >>>> t->task_done(t); >>>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>>> + spin_lock_irq(&pm8001_ha->lock); >>>> } else if (!t->uldd_task) { >>>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>>> + spin_unlock_irq(&t->task_state_lock); >>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>> mb();/*ditto*/ >>>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>>> + spin_unlock_irq(&pm8001_ha->lock); >>>> t->task_done(t); >>>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>>> + spin_lock_irq(&pm8001_ha->lock); >>>> } >>>> } > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue. 2012-03-08 20:49 ` santosh prasad nayak @ 2012-03-08 21:16 ` Mark Salyzyn 0 siblings, 0 replies; 11+ messages in thread From: Mark Salyzyn @ 2012-03-08 21:16 UTC (permalink / raw) To: santosh prasad nayak Cc: Mark Salyzyn, Jack Wang, lindar_liu, James Bottomley, linux-scsi, linux-kernel, kernel-janitors, Dan Carpenter ACK Sincerely -- Mark Salyzyn On Mar 8, 2012, at 3:48 PM, santosh prasad nayak wrote: > I did changes as per Mark's suggestion. > > The following patch is only for review not a formal one. > After getting reviewed, I will send a formal patch > > > diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c > index 3619f6e..9d82ee5 100644 > --- a/drivers/scsi/pm8001/pm8001_hwi.c > +++ b/drivers/scsi/pm8001/pm8001_hwi.c > @@ -2093,6 +2093,7 @@ mpi_sata_completion(struct pm8001_hba_info > *pm8001_ha, void *piomb) > struct ata_task_resp *resp ; > u32 *sata_resp; > struct pm8001_device *pm8001_dev; > + unsigned long flags; > > psataPayload = (struct sata_completion_resp *)(piomb + 4); > status = le32_to_cpu(psataPayload->status); > @@ -2382,26 +2383,26 @@ mpi_sata_completion(struct pm8001_hba_info > *pm8001_ha, void *piomb) > ts->stat = SAS_DEV_NO_RESPONSE; > break; > } > - spin_lock_irq(&t->task_state_lock); > + spin_lock_irqsave(&t->task_state_lock, flags); > t->task_state_flags &= ~SAS_TASK_STATE_PENDING; > t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; > t->task_state_flags |= SAS_TASK_STATE_DONE; > if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { > - spin_unlock_irq(&t->task_state_lock); > + spin_unlock_irqrestore(&t->task_state_lock, flags); > PM8001_FAIL_DBG(pm8001_ha, > pm8001_printk("task 0x%p done with io_status 0x%x" > " resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, status, ts->resp, ts->stat)); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else if (t->uldd_task) { > - spin_unlock_irq(&t->task_state_lock); > + spin_unlock_irqrestore(&t->task_state_lock, flags); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/* ditto */ > spin_unlock_irq(&pm8001_ha->lock); > t->task_done(t); > spin_lock_irq(&pm8001_ha->lock); > } else if (!t->uldd_task) { > - spin_unlock_irq(&t->task_state_lock); > + spin_unlock_irqrestore(&t->task_state_lock, flags); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/*ditto*/ > spin_unlock_irq(&pm8001_ha->lock); > @@ -2423,6 +2424,7 @@ static void mpi_sata_event(struct > pm8001_hba_info *pm8001_ha , void *piomb) > u32 tag = le32_to_cpu(psataPayload->tag); > u32 port_id = le32_to_cpu(psataPayload->port_id); > u32 dev_id = le32_to_cpu(psataPayload->device_id); > + unsigned long flags; > > ccb = &pm8001_ha->ccb_info[tag]; > t = ccb->task; > @@ -2593,26 +2595,26 @@ static void mpi_sata_event(struct > pm8001_hba_info *pm8001_ha , void *piomb) > ts->stat = SAS_OPEN_TO; > break; > } > - spin_lock_irq(&t->task_state_lock); > + spin_lock_irqsave(&t->task_state_lock, flags); > t->task_state_flags &= ~SAS_TASK_STATE_PENDING; > t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; > t->task_state_flags |= SAS_TASK_STATE_DONE; > if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { > - spin_unlock_irq(&t->task_state_lock); > + spin_unlock_irqrestore(&t->task_state_lock, flags); > PM8001_FAIL_DBG(pm8001_ha, > pm8001_printk("task 0x%p done with io_status 0x%x" > " resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, event, ts->resp, ts->stat)); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else if (t->uldd_task) { > - spin_unlock_irq(&t->task_state_lock); > + spin_unlock_irqrestore(&t->task_state_lock, flags); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/* ditto */ > spin_unlock_irq(&pm8001_ha->lock); > t->task_done(t); > spin_lock_irq(&pm8001_ha->lock); > } else if (!t->uldd_task) { > - spin_unlock_irq(&t->task_state_lock); > + spin_unlock_irqrestore(&t->task_state_lock, flags); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > mb();/*ditto*/ > spin_unlock_irq(&pm8001_ha->lock); > > > > Regards > Santosh > > On Thu, Mar 8, 2012 at 10:41 PM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote: >> Comments embedded >> >> On Mar 8, 2012, at 11:50 AM, santosh prasad nayak wrote: >> >>> Hi Mark, >>> >>> Thanks for your review. >>> >>> Few queries: >>> >>> 1. Similar changes have been made in mpi_sata_completion() surrounding >>> spin_*lock_irq*(&t->task->state_lock) >>> Should those changes also need to revert back ? >> >> I was talking generically, yes, in both set of functions, everything associated with the task_state_lock. The change as described needs to be applied to both sets of functions. >> >>>> The change you did was not inert, and result in the IRQ being disabled upon exit should a >>>> SAS_TASK_STATE_ABORTED task state condition occur then following block will enable IRQ. >>> >>> 2. I could not get this point. >>> If "SAS_TASK_STATE_ABORTED" task state condition occurs then following >>> block will enable IRQ. >>> >>> if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { >>> spin_unlock_irq(&t->task_state_lock); >>> // HERE IRQ will be enabled. >>> ....... >>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>> } >> >> Exactly, the IRQ is enabled, then the routine exits. All other paths leave the IRQ disabled (with a spin_lock_irq()). >> >>> 3. How bad will be "spin_lock_irq / spin_unlock_irq" in this case compared to >>> "spin_lock_irqsave / spin_unlock_irqrestore " >> >> Efficiency is not so much the requirement, but correctness. Yes, a spin_unlock(&t->task_state_lock) will do, but the 'world' likes to see the balance of flag save/restore, one should NOT make the assumption the interrupt is enabled or disabled upon entry, and should preserve such upon exit. >> >> Given this statement, perhaps for the case of the hacks surrounding the completion handler calls and host locks, one would do a save of the irq, unlock with irq enabled, lock, and then restore it back to the value upon entry. This is balanced. Your changes make the (correct) assumption that the irq is disabled upon entry. But pedantically incorrect, I'd take the extra steps, but I do not view that as a critical deficiency in your patch. >> >>> Regards >>> Santosh >>> >>>> >>>> I propose (annotated below) you leave the flags automatic variable, but uninitialized. The changes surrounding spin_*lock_irq*(&pm8001_ha->lock, flags) are OK, but revert back the changes surrounding the spin_*lock_irq*(&t->task->state_lock,flags) so that lock would be properly nested. >>>> >>>> To be pedantic, and to be more correct, this code should have spawned a work queue task to perform the t->task_done(t) operation in a separate thread rather than inline and precariously unlocked; but that would be beyond the scope of these changes and should be left for future work to decide if it is even necessary. Not sure how such a change would affect performance (using the work queue) ... so I am 'afraid' of pushing such a change in any case given the relatively reliable operation of this driver (and the dynamic nature of the changes Dan is making to the SATA side of things for instance ;-} ). >>>> >>>> I am sorry for taking so long to get to the task of reviewing this (and the previous) code. I look forward to your comments. >>>> >>>> Sincerely -- Mark Salyzyn >>>> >>>> On Feb 26, 2012, at 8:33 AM, santosh nayak wrote: >>>> >>>>> @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) >>>>> static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>>>> { >>>>> struct sas_task *t; >>>>> - unsigned long flags = 0; >>>> MGS> unsigned long flags; >>>>> struct task_status_struct *ts; >>>>> struct pm8001_ccb_info *ccb; >>>>> struct pm8001_device *pm8001_dev; >>>>> @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>>>> ts->stat = SAS_QUEUE_FULL; >>>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>>> mb();/*ditto*/ >>>>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>>>> + spin_unlock_irq(&pm8001_ha->lock); >>>>> t->task_done(t); >>>>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>>>> + spin_lock_irq(&pm8001_ha->lock); >>>>> return; >>>>> } >>>>> break; >>>>> @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb) >>>>> ts->stat = SAS_OPEN_TO; >>>>> break; >>>>> } >>>>> - spin_lock_irqsave(&t->task_state_lock, flags); >>>>> + spin_lock_irq(&t->task_state_lock); >>>> MGS> spin_lock_irqsave(&t->task_state_lock, flags); >>>>> t->task_state_flags &= ~SAS_TASK_STATE_PENDING; >>>>> t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; >>>>> t->task_state_flags |= SAS_TASK_STATE_DONE; >>>>> if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { >>>>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>>>> + spin_unlock_irq(&t->task_state_lock); >>>> MGS> spin_unlock_irqrestore(&t->task_state_lock, flags); >>>>> PM8001_FAIL_DBG(pm8001_ha, >>>>> pm8001_printk("task 0x%p done with io_status 0x%x" >>>>> " resp 0x%x stat 0x%x but aborted by upper layer!\n", >>>>> t, event, ts->resp, ts->stat)); >>>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>>> } else if (t->uldd_task) { >>>>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>>>> + spin_unlock_irq(&t->task_state_lock); >>>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>>> mb();/* ditto */ >>>>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>>>> + spin_unlock_irq(&pm8001_ha->lock); >>>>> t->task_done(t); >>>>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>>>> + spin_lock_irq(&pm8001_ha->lock); >>>>> } else if (!t->uldd_task) { >>>>> - spin_unlock_irqrestore(&t->task_state_lock, flags); >>>>> + spin_unlock_irq(&t->task_state_lock); >>>>> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); >>>>> mb();/*ditto*/ >>>>> - spin_unlock_irqrestore(&pm8001_ha->lock, flags); >>>>> + spin_unlock_irq(&pm8001_ha->lock); >>>>> t->task_done(t); >>>>> - spin_lock_irqsave(&pm8001_ha->lock, flags); >>>>> + spin_lock_irq(&pm8001_ha->lock); >>>>> } >>>>> } >> >> -- >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-03-08 21:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-26 13:45 [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue santosh nayak 2012-02-26 15:07 ` Dan Carpenter 2012-02-27 4:58 ` santosh prasad nayak 2012-02-27 6:55 ` Dan Carpenter 2012-02-27 8:29 ` Jack Wang 2012-03-08 13:32 ` Mark Salyzyn 2012-03-08 14:15 ` jack_wang 2012-03-08 16:51 ` santosh prasad nayak 2012-03-08 17:11 ` Mark Salyzyn 2012-03-08 20:49 ` santosh prasad nayak 2012-03-08 21:16 ` Mark Salyzyn
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).