Re: [PATCH 1/3] MTD: blktrans: Final version of hotplug support

[Artem Bityutskiy <dedekind1@gmail.com>]

In one of your e-mails you said this patch is minimalistic. Here are
some suggestions.

On Sun, 2010-09-19 at 01:12 +0200, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> ---
>  drivers/mtd/mtd_blkdevs.c |   67 +++++++++++++++++++-------------------------
>  1 files changed, 29 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 62e6870..d773a40 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -47,7 +47,6 @@ void blktrans_dev_release(struct kref *kref)
>  		container_of(kref, struct mtd_blktrans_dev, ref);
>  
>  	dev->disk->private_data = NULL;
> -	blk_cleanup_queue(dev->rq);

Them can be a separate patch.
>  	put_disk(dev->disk);
>  	list_del(&dev->list);
>  	kfree(dev);
> @@ -133,6 +132,10 @@ static int mtd_blktrans_thread(void *arg)
>  
>  		if (!req && !(req = blk_fetch_request(rq))) {
>  			set_current_state(TASK_INTERRUPTIBLE);
> +
> +			if (kthread_should_stop())
> +				break;

This could be a separate patch.

> +
>  			spin_unlock_irq(rq->queue_lock);
>  			schedule();
>  			spin_lock_irq(rq->queue_lock);
> @@ -176,54 +179,51 @@ static void mtd_blktrans_request(struct request_queue *rq)
>  static int blktrans_open(struct block_device *bdev, fmode_t mode)
>  {
>  	struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk);
> -	int ret;
> +	int ret = 0;
>  
>  	if (!dev)
> -		return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
> +		return -EIO;

Hmm, why EIO? ENODEV is better. And this can be a separate patch, may
be?

> -	lock_kernel();
>  	mutex_lock(&dev->lock);
>  
> -	if (!dev->mtd) {
> -		ret = -ENXIO;
> +	if (dev->open++)
>  		goto unlock;
> -	}
>  
> -	ret = !dev->open++ && dev->tr->open ? dev->tr->open(dev) : 0;
> +	kref_get(&dev->ref);
> +
> +	if (dev->mtd) {
> +		ret = dev->tr->open ? dev->tr->open(dev) : 0;
> +		__get_mtd_device(dev->mtd);
> +	}
>  
> -	/* Take another reference on the device so it won't go away till
> -		last release */
> -	if (!ret)
> -		kref_get(&dev->ref);
>  unlock:
>  	mutex_unlock(&dev->lock);
>  	blktrans_dev_put(dev);
> -	unlock_kernel();

So you kill BKL in the same patch? No, make it separately, please.

Did not look further.

Please, make a nice set of independent minimalistic patches. This is
very important for patches which will go to 2.6.36 to be minimal and
just fix the problem. Then the rest of the patches can do more things
and they will go to 2.6.37.
 

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)