[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

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.

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

[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: [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.

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

[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