[PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[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

[PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Dan Carpenter]

[-- 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 --]

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Dan Carpenter]

[-- 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 --]

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

 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
>

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

 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 linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

 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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Dan Carpenter]

[-- 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 --]

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Dan Carpenter]

[-- 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 --]

RE: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Jack Wang]

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
> >

RE: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Jack Wang]

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

RE: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Jack Wang]

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
> >

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Mark Salyzyn]

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);
> 	}
> }

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Mark Salyzyn]

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);
> 	}
> }

Re: Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[jack_wang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 4643 bytes --]

Thanks Mark for look into this.You're right. The change for task_state_lock is not right.
Sorry for not look carefully about the change.



--------------
jack_wang
>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);
>> 	}
>> }
>
>--
>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
>
>__________ Information from ESET NOD32 Antivirus, version of virus signature database 5659 (20101129) __________
>
>The message was checked by ESET NOD32 Antivirus.
>
>http://www.eset.com
>
>
>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[jack_wang]

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=

Re: Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[jack_wang]

Thanks Mark for look into this.You're right. The change for task_state_lock is not right.
Sorry for not look carefully about the change.



--------------
jack_wang
>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);
>> 	}
>> }
>
>--
>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
>
>__________ Information from ESET NOD32 Antivirus, version of virus signature database 5659 (20101129) __________
>
>The message was checked by ESET NOD32 Antivirus.
>
>http://www.eset.com
>
>
>

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

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 linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Mark Salyzyn]

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);
>>>       }
>>> }

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Mark Salyzyn]

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);
>>>       }
>>> }

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Mark Salyzyn]

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

Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Mark Salyzyn]

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