[MTD] My updates [RESEND]

[MTD] My updates [RESEND]

[Maxim Levitsky]

Here are few patches for 2.6.37 that I have in my patch queue for MTD.

Best regards,
	Maxim Levitsky

[PATCH 1/5] MTD: blktrans: Allow to unload the mtdtrans module if its block devices aren't open

[Maxim Levitsky]

Now it once again possible to remove mtdtrans module.
You still need to ensure that block devices of that module aren't mounted.
This is due to the fact that as long as a block device is open, it still exists,
therefore if we were to allow module removal, this block device might became used again.

This time in addition to code review, I also made the code
pass some torture tests like module reload in  a loop + read in a loop +
card insert/removal all at same time.

The blktrans_open/blktrans_release don't take the mtd table lock because:

While device is added (that includes execution of add_mtd_blktrans_dev)
the lock is already taken

Now suppose the device will never be removed. In this case even if we have changes
in mtd table, the entry that we need will stay exactly the same. (Note that we don't
look at table at all, just following private pointer of block device).

Now suppose that someone tries to remove the mtd device.
This will be propagated to trans driver which _ought_ to call del_mtd_blktrans_dev
which will take the per device lock, release the mtd device and set trans->mtd = NULL.
>From this point on, following opens won't even be able to know anything about that mtd device
(which at that point is likely not to exist)
Also the same care is taken not to trip over NULL mtd pointer in blktrans_dev_release.


Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/mtd/mtd_blkdevs.c |   52 ++++++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 62e6870..352831b 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -176,7 +176,7 @@ 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*/
@@ -184,17 +184,17 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 	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);
+	__module_get(dev->tr->owner);
+
+	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);
@@ -205,7 +205,7 @@ unlock:
 static int blktrans_release(struct gendisk *disk, fmode_t mode)
 {
 	struct mtd_blktrans_dev *dev = blktrans_dev_get(disk);
-	int ret = -ENXIO;
+	int ret = 0;
 
 	if (!dev)
 		return ret;
@@ -213,13 +213,16 @@ static int blktrans_release(struct gendisk *disk, fmode_t mode)
 	lock_kernel();
 	mutex_lock(&dev->lock);
 
-	/* Release one reference, we sure its not the last one here*/
-	kref_put(&dev->ref, blktrans_dev_release);
-
-	if (!dev->mtd)
+	if (--dev->open)
 		goto unlock;
 
-	ret = !--dev->open && dev->tr->release ? dev->tr->release(dev) : 0;
+	kref_put(&dev->ref, blktrans_dev_release);
+	module_put(dev->tr->owner);
+
+	if (dev->mtd) {
+		ret = dev->tr->release ? dev->tr->release(dev) : 0;
+		__put_mtd_device(dev->mtd);
+	}
 unlock:
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
@@ -385,9 +388,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 
 	gd->queue = new->rq;
 
-	__get_mtd_device(new->mtd);
-	__module_get(tr->owner);
-
 	/* Create processing thread */
 	/* TODO: workqueue ? */
 	new->thread = kthread_run(mtd_blktrans_thread, new,
@@ -410,8 +410,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	}
 	return 0;
 error4:
-	module_put(tr->owner);
-	__put_mtd_device(new->mtd);
 	blk_cleanup_queue(new->rq);
 error3:
 	put_disk(new->disk);
@@ -448,17 +446,15 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
 	blk_start_queue(old->rq);
 	spin_unlock_irqrestore(&old->queue_lock, flags);
 
-	/* Ask trans driver for release to the mtd device */
+	/* If the device is currently open, tell trans driver to close it,
+		then put mtd device, and don't touch it again */
 	mutex_lock(&old->lock);
-	if (old->open && old->tr->release) {
-		old->tr->release(old);
-		old->open = 0;
+	if (old->open) {
+		if (old->tr->release)
+			old->tr->release(old);
+		__put_mtd_device(old->mtd);
 	}
 
-	__put_mtd_device(old->mtd);
-	module_put(old->tr->owner);
-
-	/* At that point, we don't touch the mtd anymore */
 	old->mtd = NULL;
 
 	mutex_unlock(&old->lock);
-- 
1.7.1

[PATCH 2/5] MTD: blktrans: kill the BKL.

[Maxim Levitsky]

It not needed, because I already added locking for all fops
methods.

Signed-off-by: Maxim Levitsky
---
 drivers/mtd/mtd_blkdevs.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 352831b..040c2d9 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -181,7 +181,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 	if (!dev)
 		return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
 
-	lock_kernel();
 	mutex_lock(&dev->lock);
 
 	if (dev->open++)
@@ -198,7 +197,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 unlock:
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
-	unlock_kernel();
 	return ret;
 }
 
@@ -210,7 +208,6 @@ static int blktrans_release(struct gendisk *disk, fmode_t mode)
 	if (!dev)
 		return ret;
 
-	lock_kernel();
 	mutex_lock(&dev->lock);
 
 	if (--dev->open)
@@ -226,7 +223,6 @@ static int blktrans_release(struct gendisk *disk, fmode_t mode)
 unlock:
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
-	unlock_kernel();
 	return ret;
 }
 
@@ -259,7 +255,6 @@ static int blktrans_ioctl(struct block_device *bdev, fmode_t mode,
 	if (!dev)
 		return ret;
 
-	lock_kernel();
 	mutex_lock(&dev->lock);
 
 	if (!dev->mtd)
@@ -274,7 +269,6 @@ static int blktrans_ioctl(struct block_device *bdev, fmode_t mode,
 	}
 unlock:
 	mutex_unlock(&dev->lock);
-	unlock_kernel();
 	blktrans_dev_put(dev);
 	return ret;
 }
-- 
1.7.1

[PATCH 3/5] MTD: blktrans: fix a race vs kthread_stop.

[Maxim Levitsky]

There is small race window that could make kthread_stop hang forever.
I found that while hacking the IR subsystem.

Signed-off-by: Maxim Levitsky <maximlevisky@gmail.com>
---
 drivers/mtd/mtd_blkdevs.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 040c2d9..a919587 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -133,6 +133,10 @@ static int mtd_blktrans_thread(void *arg)
 
 		if (!req && !(req = blk_fetch_request(rq))) {
 			set_current_state(TASK_INTERRUPTIBLE);
+
+			if (kthread_should_stop())
+				set_current_state(TASK_RUNNING);
+
 			spin_unlock_irq(rq->queue_lock);
 			schedule();
 			spin_lock_irq(rq->queue_lock);
-- 
1.7.1

[PATCH 4/5] MTD: r852: remove useless pci powerup/down from suspend/resume routines.

[Maxim Levitsky]

It turns out that pci core now handles these, so this code is redundand
and can even cause bugs

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/mtd/nand/r852.c |   30 +-----------------------------
 drivers/mtd/nand/r852.h |    2 --
 2 files changed, 1 insertions(+), 31 deletions(-)

diff --git a/drivers/mtd/nand/r852.c b/drivers/mtd/nand/r852.c
index 5169ca6..d9d7efb 100644
--- a/drivers/mtd/nand/r852.c
+++ b/drivers/mtd/nand/r852.c
@@ -757,11 +757,6 @@ static irqreturn_t r852_irq(int irq, void *data)
 
 	spin_lock_irqsave(&dev->irqlock, flags);
 
-	/* We can recieve shared interrupt while pci is suspended
-		in that case reads will return 0xFFFFFFFF.... */
-	if (dev->insuspend)
-		goto out;
-
 	/* handle card detection interrupts first */
 	card_status = r852_read_reg(dev, R852_CARD_IRQ_STA);
 	r852_write_reg(dev, R852_CARD_IRQ_STA, card_status);
@@ -1035,7 +1030,6 @@ void r852_shutdown(struct pci_dev *pci_dev)
 int r852_suspend(struct device *device)
 {
 	struct r852_device *dev = pci_get_drvdata(to_pci_dev(device));
-	unsigned long flags;
 
 	if (dev->ctlreg & R852_CTL_CARDENABLE)
 		return -EBUSY;
@@ -1047,43 +1041,22 @@ int r852_suspend(struct device *device)
 	r852_disable_irqs(dev);
 	r852_engine_disable(dev);
 
-	spin_lock_irqsave(&dev->irqlock, flags);
-	dev->insuspend = 1;
-	spin_unlock_irqrestore(&dev->irqlock, flags);
-
-	/* At that point, even if interrupt handler is running, it will quit */
-	/* So wait for this to happen explictly */
-	synchronize_irq(dev->irq);
-
 	/* If card was pulled off just during the suspend, which is very
 		unlikely, we will remove it on resume, it too late now
 		anyway... */
 	dev->card_unstable = 0;
-
-	pci_save_state(to_pci_dev(device));
-	return pci_prepare_to_sleep(to_pci_dev(device));
+	return 0;
 }
 
 int r852_resume(struct device *device)
 {
 	struct r852_device *dev = pci_get_drvdata(to_pci_dev(device));
-	unsigned long flags;
-
-	/* Turn on the hardware */
-	pci_back_from_sleep(to_pci_dev(device));
-	pci_restore_state(to_pci_dev(device));
 
 	r852_disable_irqs(dev);
 	r852_card_update_present(dev);
 	r852_engine_disable(dev);
 
 
-	/* Now its safe for IRQ to run */
-	spin_lock_irqsave(&dev->irqlock, flags);
-	dev->insuspend = 0;
-	spin_unlock_irqrestore(&dev->irqlock, flags);
-
-
 	/* If card status changed, just do the work */
 	if (dev->card_detected != dev->card_registred) {
 		dbg("card was %s during low power state",
@@ -1121,7 +1094,6 @@ MODULE_DEVICE_TABLE(pci, r852_pci_id_tbl);
 
 SIMPLE_DEV_PM_OPS(r852_pm_ops, r852_suspend, r852_resume);
 
-
 static struct pci_driver r852_pci_driver = {
 	.name		= DRV_NAME,
 	.id_table	= r852_pci_id_tbl,
diff --git a/drivers/mtd/nand/r852.h b/drivers/mtd/nand/r852.h
index 8096cc2..e6a21d9 100644
--- a/drivers/mtd/nand/r852.h
+++ b/drivers/mtd/nand/r852.h
@@ -140,8 +140,6 @@ struct r852_device {
 	/* interrupt handling */
 	spinlock_t irqlock;		/* IRQ protecting lock */
 	int irq;			/* irq num */
-	int insuspend;			/* device is suspended */
-
 	/* misc */
 	void *tmp_buffer;		/* temporary buffer */
 	uint8_t ctlreg;			/* cached contents of control reg */
-- 
1.7.1

[PATCH 5/5] mtd: sm_ftl: cosmetic, use bool when possible

[Maxim Levitsky]

I didn't know that kernel allows use of that typedef.

Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
---
 drivers/mtd/sm_ftl.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/sm_ftl.h b/drivers/mtd/sm_ftl.h
index e30e48e..43bb730 100644
--- a/drivers/mtd/sm_ftl.h
+++ b/drivers/mtd/sm_ftl.h
@@ -20,7 +20,7 @@
 
 
 struct ftl_zone {
-	int initialized;
+	bool initialized;
 	int16_t *lba_to_phys_table;		/* LBA to physical table */
 	struct kfifo free_sectors;	/* queue of free sectors */
 };
@@ -37,8 +37,8 @@ struct sm_ftl {
 	int zone_count;			/* number of zones */
 	int max_lba;			/* maximum lba in a zone */
 	int smallpagenand;		/* 256 bytes/page nand */
-	int readonly;			/* is FS readonly */
-	int unstable;
+	bool readonly;			/* is FS readonly */
+	bool unstable;
 	int cis_block;			/* CIS block location */
 	int cis_boffset;		/* CIS offset in the block */
 	int cis_page_offset;		/* CIS offset in the page */
@@ -49,7 +49,7 @@ struct sm_ftl {
 	int cache_zone;			/* zone of cached block */
 	unsigned char *cache_data;	/* cached block data */
 	long unsigned int cache_data_invalid_bitmap;
-	int cache_clean;
+	bool cache_clean;
 	struct work_struct flush_work;
 	struct timer_list timer;
 
-- 
1.7.1

Re: [PATCH 2/5] MTD: blktrans: kill the BKL.

[Wolfram Sang]

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

On Fri, Oct 15, 2010 at 05:20:44PM +0200, Maxim Levitsky wrote:
> It not needed, because I already added locking for all fops
> methods.
> 
> Signed-off-by: Maxim Levitsky

This SoB got damaged.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [PATCH 2/5] MTD: blktrans: kill the BKL.

[Maxim Levitsky]

On Sat, 2010-10-16 at 07:12 +0200, Wolfram Sang wrote:
> On Fri, Oct 15, 2010 at 05:20:44PM +0200, Maxim Levitsky wrote:
> > It not needed, because I already added locking for all fops
> > methods.
> > 
> > Signed-off-by: Maxim Levitsky
> 
> This SoB got damaged.
> 

Should I resend it?
Signed-of-by: Maxim Levitsky <maximlevitky@gmail.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 2/5] MTD: blktrans: kill the BKL.

[Artem Bityutskiy]

On Sun, 2010-10-17 at 00:46 +0200, Maxim Levitsky wrote:
> On Sat, 2010-10-16 at 07:12 +0200, Wolfram Sang wrote:
> > On Fri, Oct 15, 2010 at 05:20:44PM +0200, Maxim Levitsky wrote:
> > > It not needed, because I already added locking for all fops
> > > methods.
> > > 
> > > Signed-off-by: Maxim Levitsky
> > 
> > This SoB got damaged.
> > 
> 
> Should I resend it?
> Signed-of-by: Maxim Levitsky <maximlevitky@gmail.com>

Ideally yes, the maintainer ideally should be able to just git am your
patch without having to copy-paste anything. Also, MTD subsystem's
prefix is "mtd:".

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

Re: [MTD] My updates [RESEND]

[Artem Bityutskiy]

On Fri, 2010-10-15 at 17:20 +0200, Maxim Levitsky wrote:
> Here are few patches for 2.6.37 that I have in my patch queue for MTD.
> 
> Best regards,
> 	Maxim Levitsky

I've just tested this and it fixes the issue Kevin Kernekee reported
here:
http://lists.infradead.org/pipermail/linux-mtd/2010-September/031925.html

CCing him as it is nice to inform reporters about fixes.

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

Re: [MTD] My updates [RESEND]

[Artem Bityutskiy]

On Fri, 2010-10-15 at 17:20 +0200, Maxim Levitsky wrote:
> Here are few patches for 2.6.37 that I have in my patch queue for MTD.
> 
> Best regards,
> 	Maxim Levitsky

I've added this series to my l2-mtd-2.6.git, thanks.

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

Re: [PATCH 3/5] MTD: blktrans: fix a race vs kthread_stop.

[David Woodhouse]

On Fri, 2010-10-15 at 17:20 +0200, Maxim Levitsky wrote:
> There is small race window that could make kthread_stop hang forever.
> I found that while hacking the IR subsystem.
> 
> Signed-off-by: Maxim Levitsky <maximlevisky@gmail.com>
> ---
>  drivers/mtd/mtd_blkdevs.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 040c2d9..a919587 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -133,6 +133,10 @@ static int mtd_blktrans_thread(void *arg)
>  
>  		if (!req && !(req = blk_fetch_request(rq))) {
>  			set_current_state(TASK_INTERRUPTIBLE);
> +
> +			if (kthread_should_stop())
> +				set_current_state(TASK_RUNNING);
> +
>  			spin_unlock_irq(rq->queue_lock);
>  			schedule();
>  			spin_lock_irq(rq->queue_lock);


Shouldn't we add a break in there too, so it immediately breaks out of
the loop (with the lock held)?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 3/5] MTD: blktrans: fix a race vs kthread_stop.

[Maxim Levitsky]

On Mon, 2010-10-25 at 01:31 +0100, David Woodhouse wrote:
> On Fri, 2010-10-15 at 17:20 +0200, Maxim Levitsky wrote:
> > There is small race window that could make kthread_stop hang forever.
> > I found that while hacking the IR subsystem.
> > 
> > Signed-off-by: Maxim Levitsky <maximlevisky@gmail.com>
> > ---
> >  drivers/mtd/mtd_blkdevs.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> > index 040c2d9..a919587 100644
> > --- a/drivers/mtd/mtd_blkdevs.c
> > +++ b/drivers/mtd/mtd_blkdevs.c
> > @@ -133,6 +133,10 @@ static int mtd_blktrans_thread(void *arg)
> >  
> >  		if (!req && !(req = blk_fetch_request(rq))) {
> >  			set_current_state(TASK_INTERRUPTIBLE);
> > +
> > +			if (kthread_should_stop())
> > +				set_current_state(TASK_RUNNING);
> > +
> >  			spin_unlock_irq(rq->queue_lock);
> >  			schedule();
> >  			spin_lock_irq(rq->queue_lock);
> 
> 
> Shouldn't we add a break in there too, so it immediately breaks out of
> the loop (with the lock held)?
It doesn't matter here, as schedule() won't make task sleep, so the
thread will break at next iteration.


Best regards
	Maxim Levitsky