From: Santosh Nayak <[email protected]>
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 <[email protected]>
---
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
On Sun, Feb 26, 2012 at 07:03:30PM +0530, santosh nayak wrote:
> From: Santosh Nayak <[email protected]>
>
> 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
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 <[email protected]> wrote:
> On Sun, Feb 26, 2012 at 07:03:30PM +0530, santosh nayak wrote:
>> From: Santosh Nayak <[email protected]>
>>
>> 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
>
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
Thanks for fix.
Acked-by: Jack Wang <[email protected]>
>
> 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 <[email protected]>
> wrote:
> > On Sun, Feb 26, 2012 at 07:03:30PM +0530, santosh nayak wrote:
> >> From: Santosh Nayak <[email protected]>
> >>
> >> 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
> >