[PATCH] MTD: OneNAND: Avoid deadlock in erase callback; release chip lock first.

[PATCH] MTD: OneNAND: Avoid deadlock in erase callback; release chip lock first.

[Adrian Hunter]

When the erase callback performs some other action on the flash, it's
highly likely to deadlock unless we actually release the chip lock
before calling it.

This patch mirrors that same change already done for NAND.

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mtd/onenand/onenand_base.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index a8c426b..dd28355 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1711,13 +1711,14 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
 erase_exit:
 
 	ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
-	/* Do call back function */
-	if (!ret)
-		mtd_erase_callback(instr);
 
 	/* Deselect and wake up anyone waiting on the device */
 	onenand_release_device(mtd);
 
+	/* Do call back function */
+	if (!ret)
+		mtd_erase_callback(instr);
+
 	return ret;
 }
 
-- 
1.4.4.2

Re: [PATCH] MTD: OneNAND: Avoid deadlock in erase callback; release chip lock first.

[Jörn Engel]

On Fri, 12 October 2007 10:34:01 +0300, Adrian Hunter wrote:
> 
> When the erase callback performs some other action on the flash, it's
> highly likely to deadlock unless we actually release the chip lock
> before calling it.
> 
> This patch mirrors that same change already done for NAND.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> ---
>  drivers/mtd/onenand/onenand_base.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index a8c426b..dd28355 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -1711,13 +1711,14 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>  erase_exit:
>  
>  	ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;

Can you move this line down as well?  There is no point in holding the
lock here and shrinking critical sections - even if only minimally -
is generally advantageous.

> -	/* Do call back function */
> -	if (!ret)
> -		mtd_erase_callback(instr);
>  
>  	/* Deselect and wake up anyone waiting on the device */
>  	onenand_release_device(mtd);
>  
> +	/* Do call back function */
> +	if (!ret)
> +		mtd_erase_callback(instr);
> +
>  	return ret;
>  }

Jörn

-- 
But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens

Re: [PATCH] MTD: OneNAND: Avoid deadlock in erase callback; release chip lock first.

[Adrian Hunter]

ext Jörn Engel wrote:
> On Fri, 12 October 2007 10:34:01 +0300, Adrian Hunter wrote:
>> When the erase callback performs some other action on the flash, it's
>> highly likely to deadlock unless we actually release the chip lock
>> before calling it.
>>
>> This patch mirrors that same change already done for NAND.
>>
>> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
>> ---
>>  drivers/mtd/onenand/onenand_base.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
>> index a8c426b..dd28355 100644
>> --- a/drivers/mtd/onenand/onenand_base.c
>> +++ b/drivers/mtd/onenand/onenand_base.c
>> @@ -1711,13 +1711,14 @@ static int onenand_erase(struct mtd_info *mtd, struct erase_info *instr)
>>  erase_exit:
>>  
>>  	ret = instr->state == MTD_ERASE_DONE ? 0 : -EIO;
> 
> Can you move this line down as well?  There is no point in holding the
> lock here and shrinking critical sections - even if only minimally -
> is generally advantageous.
> 
>> -	/* Do call back function */
>> -	if (!ret)
>> -		mtd_erase_callback(instr);
>>  
>>  	/* Deselect and wake up anyone waiting on the device */
>>  	onenand_release_device(mtd);
>>  
>> +	/* Do call back function */
>> +	if (!ret)
>> +		mtd_erase_callback(instr);
>> +
>>  	return ret;
>>  }
> 
> Jörn
> 

I wanted the patch to be the same as NAND

see http://git.infradead.org/?p=mtd-2.6.git;a=commitdiff;h=49defc015ff58fda46a3afa3462dfdfa69bc8401

Anyone who feels strongly should change both places ;-)