re: [SCSI] pm8001: Fix bogus interrupt state flag issue.

re: [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Dan Carpenter]

Hello Santosh Nayak,

The patch bdaefbf580cd: "[SCSI] pm8001: Fix bogus interrupt state 
flag issue." from Feb 26, 2012, leads to the following warning:
drivers/scsi/pm8001/pm8001_hwi.c:2400 mpi_sata_completion()
	 error: double unlock 'irq:'


        } 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);
                ^^^^^^^^^^^^^^^
It doesn't make sense to enable IRQs twice.  I'm not sure if it should
be the first or second unlock which enables them.

                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);
                ^^^^^^^^^^^^^^^
Same thing again.

                t->task_done(t);
-               spin_lock_irqsave(&pm8001_ha->lock, flags);
+               spin_lock_irq(&pm8001_ha->lock);
        }

regards,
dan carpenter

Re: [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

The warnings are  there since from the beginning.
Even before my fix.

There are two locks:
1.  pm8001_ha->lock
2. t->task_state_lock

regards
santosh

On Thu, Mar 1, 2012 at 7:52 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Santosh Nayak,
>
> The patch bdaefbf580cd: "[SCSI] pm8001: Fix bogus interrupt state
> flag issue." from Feb 26, 2012, leads to the following warning:
> drivers/scsi/pm8001/pm8001_hwi.c:2400 mpi_sata_completion()
>         error: double unlock 'irq:'
>
>
>        } 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);
>                ^^^^^^^^^^^^^^^
> It doesn't make sense to enable IRQs twice.  I'm not sure if it should
> be the first or second unlock which enables them.
>
>                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);
>                ^^^^^^^^^^^^^^^
> Same thing again.
>
>                t->task_done(t);
> -               spin_lock_irqsave(&pm8001_ha->lock, flags);
> +               spin_lock_irq(&pm8001_ha->lock);
>        }
>
> 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: [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Thu, Mar 01, 2012 at 07:54:57PM +0530, santosh prasad nayak wrote:
> The warnings are  there since from the beginning.
> Even before my fix.
> 
> There are two locks:
> 1.  pm8001_ha->lock
> 2. t->task_state_lock
> 

Yes.  But IRQs are either enabled or disabled for the CPU.  You
can't enable them twice.

In this case we enable them in the first call to unlock and the
second call doesn't do anything.  So possibly the current behavior
is correct and we should change the second unlock or possibly the
current behavior is wrong and we should change the first unlock.

But actually the email was more of just a form letter.  Perhaps Jack
knows what to do here...

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [SCSI] pm8001: Fix bogus interrupt state flag issue.

[santosh prasad nayak]

t->task_state_lock  can be changed to simple spin lock instead of 'irq
spin lock'

Initially I thought to do so. But could not get the intention of the
developer why he used
an irq lock there. Hence left it unchanged. Just changed from 'irqsave
spin lock' to 'irq spin lock'

regard
santosh



On Thu, Mar 1, 2012 at 8:07 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Mar 01, 2012 at 07:54:57PM +0530, santosh prasad nayak wrote:
>> The warnings are  there since from the beginning.
>> Even before my fix.
>>
>> There are two locks:
>> 1.  pm8001_ha->lock
>> 2. t->task_state_lock
>>
>
> Yes.  But IRQs are either enabled or disabled for the CPU.  You
> can't enable them twice.
>
> In this case we enable them in the first call to unlock and the
> second call doesn't do anything.  So possibly the current behavior
> is correct and we should change the second unlock or possibly the
> current behavior is wrong and we should change the first unlock.
>
> But actually the email was more of just a form letter.  Perhaps Jack
> knows what to do here...
>
> 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: [SCSI] pm8001: Fix bogus interrupt state flag issue.

[Jack Wang]

Dear Dan and Santosh Nayak,

I change task_state_lock to spin_lock is OK. Please feel free to send patch
out.

Thanks.

Jack
> 
> Hello Santosh Nayak,
> 
> The patch bdaefbf580cd: "[SCSI] pm8001: Fix bogus interrupt state
> flag issue." from Feb 26, 2012, leads to the following warning:
> drivers/scsi/pm8001/pm8001_hwi.c:2400 mpi_sata_completion()
> 	 error: double unlock 'irq:'
> 
> 
>         } 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);
>                 ^^^^^^^^^^^^^^^
> It doesn't make sense to enable IRQs twice.  I'm not sure if it should
> be the first or second unlock which enables them.
> 
>                 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);
>                 ^^^^^^^^^^^^^^^
> Same thing again.
> 
>                 t->task_done(t);
> -               spin_lock_irqsave(&pm8001_ha->lock, flags);
> +               spin_lock_irq(&pm8001_ha->lock);
>         }
> 
> 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