[PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

[PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

[Ying Chu]

>From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
From: Andy <ayan@marvell.com>
Date: Mon, 4 May 2009 23:31:43 +0800
Subject: [PATCH 2/4] bug fix: deal lock 

TMF task need be issued with Interrupt Disabled, or Deadlock may take place.

Signed-off-by: Ying Chu <jasonchu@marvell.com>
Signed-off-by: Andy Yan <ayan@marvell.com>
Signed-off-by: Ke Wei <kewei@marvell.com>
---
 drivers/scsi/mvsas/mv_sas.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 318ec01..cb002ef 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
 		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
 		add_timer(&task->timer);
 
-		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
+		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);
 
 		if (res) {
 			del_timer(&task->timer);
-- 
1.6.0.3

-- 
Regards,
Ying Chu

Re: [PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

[James Bottomley]

On Fri, 2009-05-08 at 14:16 +0800, Ying Chu wrote:
> >From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
> From: Andy <ayan@marvell.com>
> Date: Mon, 4 May 2009 23:31:43 +0800
> Subject: [PATCH 2/4] bug fix: deal lock 
> 
> TMF task need be issued with Interrupt Disabled, or Deadlock may take place.
> 
> Signed-off-by: Ying Chu <jasonchu@marvell.com>
> Signed-off-by: Andy Yan <ayan@marvell.com>
> Signed-off-by: Ke Wei <kewei@marvell.com>
> ---
>  drivers/scsi/mvsas/mv_sas.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index 318ec01..cb002ef 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
>  		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
>  		add_timer(&task->timer);
>  
> -		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
> +		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);

That eliminates the last user of the unlocked case.

Can't we just drop the conditional lock from mvs_task_exec() now
instead?  It will make the code much cleaner.

James

Re: [PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

[Jeff Garzik]

James Bottomley wrote:
> On Fri, 2009-05-08 at 14:16 +0800, Ying Chu wrote:
>> >From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
>> From: Andy <ayan@marvell.com>
>> Date: Mon, 4 May 2009 23:31:43 +0800
>> Subject: [PATCH 2/4] bug fix: deal lock 
>>
>> TMF task need be issued with Interrupt Disabled, or Deadlock may take place.
>>
>> Signed-off-by: Ying Chu <jasonchu@marvell.com>
>> Signed-off-by: Andy Yan <ayan@marvell.com>
>> Signed-off-by: Ke Wei <kewei@marvell.com>
>> ---
>>  drivers/scsi/mvsas/mv_sas.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
>> index 318ec01..cb002ef 100644
>> --- a/drivers/scsi/mvsas/mv_sas.c
>> +++ b/drivers/scsi/mvsas/mv_sas.c
>> @@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
>>  		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
>>  		add_timer(&task->timer);
>>  
>> -		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
>> +		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);
> 
> That eliminates the last user of the unlocked case.
> 
> Can't we just drop the conditional lock from mvs_task_exec() now
> instead?  It will make the code much cleaner.

Agreed...

RE: [PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

[Andy Yan]

James,
	For considering the possibility of future use, I did not remove the parameter; I will check this and remove it with next patch if it is OK.

Regards
Andy Yan


-----Original Message-----
From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Jeff Garzik
Sent: 2009年5月9日 6:06
To: James Bottomley
Cc: Ying Chu; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

James Bottomley wrote:
> On Fri, 2009-05-08 at 14:16 +0800, Ying Chu wrote:
>> >From 58f754f51fced4a39adcf708319e3de946a0d61a Mon Sep 17 00:00:00 2001
>> From: Andy <ayan@marvell.com>
>> Date: Mon, 4 May 2009 23:31:43 +0800
>> Subject: [PATCH 2/4] bug fix: deal lock 
>>
>> TMF task need be issued with Interrupt Disabled, or Deadlock may take place.
>>
>> Signed-off-by: Ying Chu <jasonchu@marvell.com>
>> Signed-off-by: Andy Yan <ayan@marvell.com>
>> Signed-off-by: Ke Wei <kewei@marvell.com>
>> ---
>>  drivers/scsi/mvsas/mv_sas.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
>> index 318ec01..cb002ef 100644
>> --- a/drivers/scsi/mvsas/mv_sas.c
>> +++ b/drivers/scsi/mvsas/mv_sas.c
>> @@ -1446,7 +1446,7 @@ static int mvs_exec_internal_tmf_task(struct domain_device *dev,
>>  		task->timer.expires = jiffies + MVS_TASK_TIMEOUT*HZ;
>>  		add_timer(&task->timer);
>>  
>> -		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 0, 1, tmf);
>> +		res = mvs_task_exec(task, 1, GFP_KERNEL, NULL, 1, 1, tmf);
> 
> That eliminates the last user of the unlocked case.
> 
> Can't we just drop the conditional lock from mvs_task_exec() now
> instead?  It will make the code much cleaner.

Agreed...


--
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 2/4] mvsas: Deadlocks meet when TMF tasks issue

[Jeff Garzik]

Andy Yan wrote:
> James,
> 	For considering the possibility of future use, I did not remove the parameter; I will check this and remove it with next patch if it is OK.


Generally, we encourage immediate elimination of all code not actively used.

The only major exception to this rule is structures or ABI that are 
visible to userland, such as ioctls, where we must maintain binary 
compatibility.

	Jeff

RE: [PATCH 2/4] mvsas: Deadlocks meet when TMF tasks issue

[James Bottomley]

On Sat, 2009-05-09 at 00:25 -0700, Andy Yan wrote:
> 	For considering the possibility of future use, I did not remove the parameter; I will check this and remove it with next patch if it is OK.

Well, there are two reasons for removing it.  The first is that it's
unused, but the second is that conditional locking isn't very nice
programming style, since it makes control flow far harder to follow
rationally when just reading the code.  The preferred style is to define
a function that does the unlocked operation, usually with double
underscores and then have a locked version that takes the lock, calls
the double underscore unlocked version and releases the lock again.  In
your case, if you find a use for the unlocked version, you can split it
in the patch that's using it.

Looking at the routine, the only thing you call outside the lock is
mvs_find_dev_mvi().  The thing that strikes me here is that this is a
bit inefficient: why not store mvi in struct mvs_device?  Then you can
get it as a simple pointer operation instead of having to search.

James