Re: [PATCH v2] mtd: mtd_blkdevs: Get|Put mtd_device in add|del_mtd_blktrans

[Alexander Sverdlin]

Hello!

On 15/06/2021 11:13, Zhihao Cheng wrote:
> There are two reasons to move mtd_device get|put operations into mtd
> blktrans adding/deleting:

...

> Fixes: 073db4a51ee43c ("mtd: fix: avoid race condition when ...")
> Fixes: 008c751ec78587 ("mtd: allow to unload the mtdtrans module if ...")
> Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>

> ---
>  drivers/mtd/mtd_blkdevs.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index fb8e12d590a1..19a2870a097a 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -209,7 +209,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
>  	if (!dev)
>  		return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
>  
> -	mutex_lock(&mtd_table_mutex);
>  	mutex_lock(&dev->lock);
>  
>  	if (dev->open)
> @@ -227,26 +226,18 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
>  			goto error_put;
>  	}
>  
> -	ret = __get_mtd_device(dev->mtd);
> -	if (ret)
> -		goto error_release;
>  	dev->file_mode = mode;
>  
>  unlock:
>  	dev->open++;
>  	mutex_unlock(&dev->lock);
> -	mutex_unlock(&mtd_table_mutex);
>  	blktrans_dev_put(dev);
>  	return ret;
>  
> -error_release:
> -	if (dev->tr->release)
> -		dev->tr->release(dev);
>  error_put:
>  	module_put(dev->tr->owner);
>  	kref_put(&dev->ref, blktrans_dev_release);
>  	mutex_unlock(&dev->lock);
> -	mutex_unlock(&mtd_table_mutex);
>  	blktrans_dev_put(dev);
>  	return ret;
>  }
> @@ -258,7 +249,6 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
>  	if (!dev)
>  		return;
>  
> -	mutex_lock(&mtd_table_mutex);
>  	mutex_lock(&dev->lock);
>  
>  	if (--dev->open)
> @@ -267,14 +257,10 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
>  	kref_put(&dev->ref, blktrans_dev_release);
>  	module_put(dev->tr->owner);
>  
> -	if (dev->mtd) {
> -		if (dev->tr->release)
> -			dev->tr->release(dev);
> -		__put_mtd_device(dev->mtd);
> -	}
> +	if (dev->mtd && dev->tr->release)
> +		dev->tr->release(dev);
>  unlock:
>  	mutex_unlock(&dev->lock);
> -	mutex_unlock(&mtd_table_mutex);
>  	blktrans_dev_put(dev);
>  }
>  
> @@ -425,6 +411,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>  	}
>  
>  	gd->queue = new->rq;
> +	__get_mtd_device(new->mtd);
>  
>  	if (new->readonly)
>  		set_disk_ro(gd, 1);
> @@ -474,15 +461,15 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
>  	blk_mq_unquiesce_queue(old->rq);
>  	blk_mq_unfreeze_queue(old->rq);
>  
> -	/* If the device is currently open, tell trans driver to close it,
> -		then put mtd device, and don't touch it again */
> +	/* If the device is currently open, tell trans driver to close it */
>  	mutex_lock(&old->lock);
>  	if (old->open) {
>  		if (old->tr->release)
>  			old->tr->release(old);
> -		__put_mtd_device(old->mtd);
>  	}
> +	__put_mtd_device(old->mtd);
>  
> +	/* At that point, we don't touch the mtd anymore */
>  	old->mtd = NULL;
>  
>  	mutex_unlock(&old->lock);

-- 
Best regards,
Alexander Sverdlin.