2010-01-22 15:32:15

by Maxim Levitsky

[permalink] [raw]
Subject: [PATCH 0/4] Cleanups for mtd translation layer

Hello,

I have broken up the patch series that I created for proper
SmartMedia/xD support.

This patch series contain patches that fix hotplug & suspend/resume in
mtd translation layer.

Patches are (almost) unmodified from last submission, and well tested on
real hotplugable card.
I can say that for a long time (even before last submission) I had no
issues regarding hotplug and/or suspend resume even in exterme cases.
(like suspending the system while doing 'dd if=/dev/zero of=/dev/smblka'
or yanking the card out while read or write is performed)

First two patches, prepare ground for real work and don't add new
functionality.

Third patch adds everything needed to common translation layer to make
the hotplug work.

Last patch makes suspend/resume work much better by allowing to freeze
the IO thread, thus avoiding situation you suspend in middle of IO.


Best regards,
Maxim Levitsky


2010-01-22 15:33:35

by Maxim Levitsky

[permalink] [raw]
Subject: [PATCH 1/4] MTD: call remove notifiers before removing the device

>From e74bb4a1e1222ec35fa8ed0d0d40b8e3607992d8 Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <[email protected]>
Date: Fri, 22 Jan 2010 15:00:53 +0200
Subject: [PATCH 1/4] MTD: call remove notifiers before removing the device

This is first step in implementing proper hotplug support.
Now all users of the mtd device have a chance to put the mtd device
when they are notified to do so, and they have to do so to make hotplug work.

Signed-off-by: Maxim Levitsky <[email protected]>
---
drivers/mtd/mtdcore.c | 21 +++++++++++----------
1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c356c0a..87ff83f 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -341,31 +341,32 @@ int add_mtd_device(struct mtd_info *mtd)
int del_mtd_device (struct mtd_info *mtd)
{
int ret;
+ struct mtd_notifier *not;

mutex_lock(&mtd_table_mutex);

if (mtd_table[mtd->index] != mtd) {
ret = -ENODEV;
- } else if (mtd->usecount) {
+ goto out_error;
+ }
+
+ /* No need to get a refcount on the module containing
+ the notifier, since we hold the mtd_table_mutex */
+ list_for_each_entry(not, &mtd_notifiers, list)
+ not->remove(mtd);
+
+ if (mtd->usecount) {
printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
mtd->index, mtd->name, mtd->usecount);
ret = -EBUSY;
} else {
- struct mtd_notifier *not;
-
device_unregister(&mtd->dev);
-
- /* No need to get a refcount on the module containing
- the notifier, since we hold the mtd_table_mutex */
- list_for_each_entry(not, &mtd_notifiers, list)
- not->remove(mtd);
-
mtd_table[mtd->index] = NULL;
-
module_put(THIS_MODULE);
ret = 0;
}

+out_error:
mutex_unlock(&mtd_table_mutex);
return ret;
}
--
1.6.3.3


2010-01-22 15:34:26

by Maxim Levitsky

[permalink] [raw]
Subject: [PATCH 2/4] MTD: create lockless versions of {get,put}_mtd_device

>From 628ad1a6a240d444dbd4d6897181daa096ac6226 Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <[email protected]>
Date: Fri, 22 Jan 2010 15:00:54 +0200
Subject: [PATCH 2/4] MTD: create lockless versions of {get,put}_mtd_device

This will be used to resolve deadlock in block translation layer.

Signed-off-by: Maxim Levitsky <[email protected]>
---
drivers/mtd/mtdcore.c | 61 ++++++++++++++++++++++++++++++-----------------
include/linux/mtd/mtd.h | 3 +-
2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 87ff83f..96b4a05 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -454,27 +454,38 @@ struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num)
ret = NULL;
}

- if (!ret)
- goto out_unlock;
-
- if (!try_module_get(ret->owner))
- goto out_unlock;
-
- if (ret->get_device) {
- err = ret->get_device(ret);
- if (err)
- goto out_put;
+ if (!ret) {
+ ret = ERR_PTR(err);
+ goto out;
}

- ret->usecount++;
+ err = __get_mtd_device(ret);
+ if (err)
+ ret = ERR_PTR(err);
+out:
mutex_unlock(&mtd_table_mutex);
return ret;
+}

-out_put:
- module_put(ret->owner);
-out_unlock:
- mutex_unlock(&mtd_table_mutex);
- return ERR_PTR(err);
+
+int __get_mtd_device(struct mtd_info *mtd)
+{
+ int err;
+
+ if (!try_module_get(mtd->owner))
+ return -ENODEV;
+
+ if (mtd->get_device) {
+
+ err = mtd->get_device(mtd);
+
+ if (err) {
+ module_put(mtd->owner);
+ return err;
+ }
+ }
+ mtd->usecount++;
+ return 0;
}

/**
@@ -525,18 +536,22 @@ out_unlock:

void put_mtd_device(struct mtd_info *mtd)
{
- int c;
-
mutex_lock(&mtd_table_mutex);
- c = --mtd->usecount;
- if (mtd->put_device)
- mtd->put_device(mtd);
+ __put_mtd_device(mtd);
mutex_unlock(&mtd_table_mutex);
- BUG_ON(c < 0);

module_put(mtd->owner);
}

+void __put_mtd_device(struct mtd_info *mtd)
+{
+ --mtd->usecount;
+ BUG_ON(mtd->usecount < 0);
+
+ if (mtd->put_device)
+ mtd->put_device(mtd);
+}
+
/* default_mtd_writev - default mtd writev method for MTD devices that
* don't implement their own
*/
@@ -570,7 +585,9 @@ EXPORT_SYMBOL_GPL(add_mtd_device);
EXPORT_SYMBOL_GPL(del_mtd_device);
EXPORT_SYMBOL_GPL(get_mtd_device);
EXPORT_SYMBOL_GPL(get_mtd_device_nm);
+EXPORT_SYMBOL_GPL(__get_mtd_device);
EXPORT_SYMBOL_GPL(put_mtd_device);
+EXPORT_SYMBOL_GPL(__put_mtd_device);
EXPORT_SYMBOL_GPL(register_mtd_user);
EXPORT_SYMBOL_GPL(unregister_mtd_user);
EXPORT_SYMBOL_GPL(default_mtd_writev);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 0f32a9b..662d747 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -290,8 +290,9 @@ extern int add_mtd_device(struct mtd_info *mtd);
extern int del_mtd_device (struct mtd_info *mtd);

extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
+extern int __get_mtd_device(struct mtd_info *mtd);
+extern void __put_mtd_device(struct mtd_info *mtd);
extern struct mtd_info *get_mtd_device_nm(const char *name);
-
extern void put_mtd_device(struct mtd_info *mtd);


--
1.6.3.3


2010-01-22 15:35:18

by Maxim Levitsky

[permalink] [raw]
Subject: [PATCH 3/4] MTD: blkdevs: make hotplug work

>From 368127067f4d39ba89096e4a5cb3be2dee361bad Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <[email protected]>
Date: Fri, 22 Jan 2010 15:00:55 +0200
Subject: [PATCH 3/4] MTD: blkdevs: make hotplug work

This changes the blkdev common module for translation layers
to survive when underlying mtd device disappears.

To do so the following conceptual changes were made:

* disk queue and thread are now one per mtd device
This was it is easy to flush and destroy the queue

* the struct mtd_blktrans_dev will now be freed automaticly when last user
of the device quits.
All existing translation layers are adjusted

* ->open and release function of the translation layer will never be called
twise or more in the row.
This makes code simplier.
Also the ->release will be called just before mtd device disappears
This and above is the only visable changes on the outside.

Tested with mtdblock, ssfdc and my own sm_ftl on top of physicly hotplugable
nand card.

Signed-off-by: Maxim Levitsky <[email protected]>
---
drivers/mtd/ftl.c | 1 -
drivers/mtd/inftlcore.c | 1 -
drivers/mtd/mtd_blkdevs.c | 237 ++++++++++++++++++++++++++---------------
drivers/mtd/mtdblock.c | 1 -
drivers/mtd/mtdblock_ro.c | 1 -
drivers/mtd/mtdcore.c | 3 +-
drivers/mtd/nftlcore.c | 1 -
drivers/mtd/rfd_ftl.c | 1 -
drivers/mtd/ssfdc.c | 1 -
include/linux/mtd/blktrans.h | 13 ++-
10 files changed, 160 insertions(+), 100 deletions(-)

diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c
index e56d6b4..62da9eb 100644
--- a/drivers/mtd/ftl.c
+++ b/drivers/mtd/ftl.c
@@ -1082,7 +1082,6 @@ static void ftl_remove_dev(struct mtd_blktrans_dev *dev)
{
del_mtd_blktrans_dev(dev);
ftl_freepart((partition_t *)dev);
- kfree(dev);
}

static struct mtd_blktrans_ops ftl_tr = {
diff --git a/drivers/mtd/inftlcore.c b/drivers/mtd/inftlcore.c
index 8aca552..015a7fe 100755
--- a/drivers/mtd/inftlcore.c
+++ b/drivers/mtd/inftlcore.c
@@ -139,7 +139,6 @@ static void inftl_remove_dev(struct mtd_blktrans_dev *dev)

kfree(inftl->PUtable);
kfree(inftl->VUtable);
- kfree(inftl);
}

/*
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index c82e09b..04a875f 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -26,11 +26,6 @@

static LIST_HEAD(blktrans_majors);

-struct mtd_blkcore_priv {
- struct task_struct *thread;
- struct request_queue *rq;
- spinlock_t queue_lock;
-};

static int do_blktrans_request(struct mtd_blktrans_ops *tr,
struct mtd_blktrans_dev *dev,
@@ -80,14 +75,13 @@ static int do_blktrans_request(struct mtd_blktrans_ops *tr,

static int mtd_blktrans_thread(void *arg)
{
- struct mtd_blktrans_ops *tr = arg;
- struct request_queue *rq = tr->blkcore_priv->rq;
+ struct mtd_blktrans_dev *dev = arg;
+ struct request_queue *rq = dev->rq;
struct request *req = NULL;

spin_lock_irq(rq->queue_lock);

while (!kthread_should_stop()) {
- struct mtd_blktrans_dev *dev;
int res;

if (!req && !(req = blk_fetch_request(rq))) {
@@ -98,13 +92,10 @@ static int mtd_blktrans_thread(void *arg)
continue;
}

- dev = req->rq_disk->private_data;
- tr = dev->tr;
-
spin_unlock_irq(rq->queue_lock);

mutex_lock(&dev->lock);
- res = do_blktrans_request(tr, dev, req);
+ res = do_blktrans_request(dev->tr, dev, req);
mutex_unlock(&dev->lock);

spin_lock_irq(rq->queue_lock);
@@ -123,8 +114,14 @@ static int mtd_blktrans_thread(void *arg)

static void mtd_blktrans_request(struct request_queue *rq)
{
- struct mtd_blktrans_ops *tr = rq->queuedata;
- wake_up_process(tr->blkcore_priv->thread);
+ struct mtd_blktrans_dev *dev = rq->queuedata;
+ struct request *req = NULL;
+
+ if (dev->deleted)
+ while ((req = blk_fetch_request(rq)) != NULL)
+ __blk_end_request_all(req, -ENODEV);
+ else
+ wake_up_process(dev->thread);
}


@@ -132,72 +129,108 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
{
struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data;
struct mtd_blktrans_ops *tr = dev->tr;
- int ret = -ENODEV;
+ int ret = 0;
+
+ mutex_lock(&dev->lock);
+ if (dev->open++)
+ goto out;

- if (!get_mtd_device(NULL, dev->mtd->index))
+ if (dev->deleted)
goto out;

+ ret = -ENODEV;
if (!try_module_get(tr->owner))
- goto out_tr;
+ goto out;

- /* FIXME: Locking. A hot pluggable device can go away
- (del_mtd_device can be called for it) without its module
- being unloaded. */
- dev->mtd->usecount++;
+ if (__get_mtd_device(dev->mtd)) {
+ module_put(tr->owner);
+ goto out;
+ }

ret = 0;
if (tr->open && (ret = tr->open(dev))) {
- dev->mtd->usecount--;
- put_mtd_device(dev->mtd);
- out_tr:
module_put(tr->owner);
+ __put_mtd_device(dev->mtd);
+ goto out;
}
out:
+ mutex_unlock(&dev->lock);
return ret;
}

+
static int blktrans_release(struct gendisk *disk, fmode_t mode)
{
struct mtd_blktrans_dev *dev = disk->private_data;
struct mtd_blktrans_ops *tr = dev->tr;
int ret = 0;

- if (tr->release)
- ret = tr->release(dev);
+ mutex_lock(&dev->lock);
+ dev->open--;
+ if (dev->open)
+ goto out;

- if (!ret) {
- dev->mtd->usecount--;
- put_mtd_device(dev->mtd);
+ /* Free the private data */
+ if (dev->deleted) {
module_put(tr->owner);
+ mutex_unlock(&dev->lock);
+ kfree(dev);
+ return 0;
}

+ ret = tr->release ? tr->release(dev) : 0;
+ module_put(tr->owner);
+
+ if (dev->mtd)
+ __put_mtd_device(dev->mtd);
+out:
+ mutex_unlock(&dev->lock);
return ret;
}

static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
{
struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data;
+ int error;
+
+ mutex_lock(&dev->lock);

+ error = -ENODEV;
+ if (dev->deleted)
+ goto out;
+
+ error = -ENOTTY;
if (dev->tr->getgeo)
- return dev->tr->getgeo(dev, geo);
- return -ENOTTY;
+ error = dev->tr->getgeo(dev, geo);
+out:
+ mutex_unlock(&dev->lock);
+ return error;
}

static int blktrans_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data;
- struct mtd_blktrans_ops *tr = dev->tr;
+ int error = -ENODEV;
+
+ mutex_lock(&dev->lock);
+
+ if (dev->deleted)
+ goto out;
+
+ error = -ENOTTY;

switch (cmd) {
case BLKFLSBUF:
- if (tr->flush)
- return tr->flush(dev);
- /* The core code did the work, we had nothing to do. */
- return 0;
+ if (dev->tr->flush)
+ error = dev->tr->flush(dev);
+ break;
default:
- return -ENOTTY;
+ break;
}
+out:
+ mutex_unlock(&dev->lock);
+ return error;
}

static const struct block_device_operations mtd_blktrans_ops = {
@@ -214,6 +247,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
struct mtd_blktrans_dev *d;
int last_devnum = -1;
struct gendisk *gd;
+ int ret;

if (mutex_trylock(&mtd_table_mutex)) {
mutex_unlock(&mtd_table_mutex);
@@ -239,12 +273,13 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
}
last_devnum = d->devnum;
}
+
+ ret = -EBUSY;
if (new->devnum == -1)
new->devnum = last_devnum+1;

- if ((new->devnum << tr->part_bits) > 256) {
- return -EBUSY;
- }
+ if ((new->devnum << tr->part_bits) > 256)
+ goto error1;

list_add_tail(&new->list, &tr->devs);
added:
@@ -252,11 +287,16 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
if (!tr->writesect)
new->readonly = 1;

+
+ /* Create gendisk */
+ ret = -ENOMEM;
gd = alloc_disk(1 << tr->part_bits);
- if (!gd) {
- list_del(&new->list);
- return -ENOMEM;
- }
+
+ if (!gd)
+ goto error2;
+
+ new->disk = gd;
+ gd->private_data = new;
gd->major = tr->major;
gd->first_minor = (new->devnum) << tr->part_bits;
gd->fops = &mtd_blktrans_ops;
@@ -274,25 +314,58 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
snprintf(gd->disk_name, sizeof(gd->disk_name),
"%s%d", tr->name, new->devnum);

- /* 2.5 has capacity in units of 512 bytes while still
- having BLOCK_SIZE_BITS set to 10. Just to keep us amused. */
set_capacity(gd, (new->size * tr->blksize) >> 9);

- gd->private_data = new;
- new->blkcore_priv = gd;
- gd->queue = tr->blkcore_priv->rq;
+
+ /* Create the request queue */
+ spin_lock_init(&new->queue_lock);
+ new->rq = blk_init_queue(mtd_blktrans_request, &new->queue_lock);
+
+ if (!new->rq)
+ goto error3;
+
+ new->rq->queuedata = new;
+ blk_queue_logical_block_size(new->rq, tr->blksize);
+
+ if (tr->discard)
+ queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
+ new->rq);
+
+ gd->queue = new->rq;
+
+ /* Create processing thread */
+ /* TODO: workqueue ? */
+ new->thread = kthread_run(mtd_blktrans_thread, new,
+ "%s%d", tr->name, new->mtd->index);
+ if (IS_ERR(new->thread)) {
+ ret = PTR_ERR(new->thread);
+ goto error4;
+ }
+
gd->driverfs_dev = &new->mtd->dev;

if (new->readonly)
set_disk_ro(gd, 1);

+ new->open = 0;
add_disk(gd);

return 0;
+error4:
+ blk_cleanup_queue(new->rq);
+error3:
+ put_disk(new->disk);
+error2:
+ list_del(&new->list);
+error1:
+ kfree(new);
+ return ret;
}

int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
{
+ unsigned long flags;
+
if (mutex_trylock(&mtd_table_mutex)) {
mutex_unlock(&mtd_table_mutex);
BUG();
@@ -300,9 +373,35 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)

list_del(&old->list);

- del_gendisk(old->blkcore_priv);
- put_disk(old->blkcore_priv);
+ /* stop new requests to arrive */
+ del_gendisk(old->disk);
+
+
+ /* flush current requests */
+ spin_lock_irqsave(&old->queue_lock, flags);
+ old->deleted = 1;
+ blk_start_queue(old->rq);
+ spin_unlock_irqrestore(&old->queue_lock, flags);
+
+
+ /* Stop the thread */
+ kthread_stop(old->thread);

+ /* Tell trans driver to release the device */
+ mutex_lock(&old->lock);
+
+ if (old->open) {
+ if (old->tr->release)
+ old->tr->release(old);
+ __put_mtd_device(old->mtd);
+ }
+
+ /* From now on, no calls into trans can be made */
+ /* Mtd device will be gone real soon now */
+ old->mtd = NULL;
+ mutex_unlock(&old->lock);
+
+ blk_cleanup_queue(old->rq);
return 0;
}

@@ -343,9 +442,6 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
if (!blktrans_notifier.list.next)
register_mtd_user(&blktrans_notifier);

- tr->blkcore_priv = kzalloc(sizeof(*tr->blkcore_priv), GFP_KERNEL);
- if (!tr->blkcore_priv)
- return -ENOMEM;

mutex_lock(&mtd_table_mutex);

@@ -353,39 +449,12 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
if (ret) {
printk(KERN_WARNING "Unable to register %s block device on major %d: %d\n",
tr->name, tr->major, ret);
- kfree(tr->blkcore_priv);
mutex_unlock(&mtd_table_mutex);
return ret;
}
- spin_lock_init(&tr->blkcore_priv->queue_lock);
-
- tr->blkcore_priv->rq = blk_init_queue(mtd_blktrans_request, &tr->blkcore_priv->queue_lock);
- if (!tr->blkcore_priv->rq) {
- unregister_blkdev(tr->major, tr->name);
- kfree(tr->blkcore_priv);
- mutex_unlock(&mtd_table_mutex);
- return -ENOMEM;
- }
-
- tr->blkcore_priv->rq->queuedata = tr;
- blk_queue_logical_block_size(tr->blkcore_priv->rq, tr->blksize);
- if (tr->discard)
- queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
- tr->blkcore_priv->rq);

tr->blkshift = ffs(tr->blksize) - 1;

- tr->blkcore_priv->thread = kthread_run(mtd_blktrans_thread, tr,
- "%sd", tr->name);
- if (IS_ERR(tr->blkcore_priv->thread)) {
- ret = PTR_ERR(tr->blkcore_priv->thread);
- blk_cleanup_queue(tr->blkcore_priv->rq);
- unregister_blkdev(tr->major, tr->name);
- kfree(tr->blkcore_priv);
- mutex_unlock(&mtd_table_mutex);
- return ret;
- }
-
INIT_LIST_HEAD(&tr->devs);
list_add(&tr->list, &blktrans_majors);

@@ -405,8 +474,6 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)

mutex_lock(&mtd_table_mutex);

- /* Clean up the kernel thread */
- kthread_stop(tr->blkcore_priv->thread);

/* Remove it from the list of active majors */
list_del(&tr->list);
@@ -414,13 +481,9 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
list_for_each_entry_safe(dev, next, &tr->devs, list)
tr->remove_dev(dev);

- blk_cleanup_queue(tr->blkcore_priv->rq);
unregister_blkdev(tr->major, tr->name);
-
mutex_unlock(&mtd_table_mutex);

- kfree(tr->blkcore_priv);
-
BUG_ON(!list_empty(&tr->devs));
return 0;
}
diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
index 9f41b1a..d8322cc 100644
--- a/drivers/mtd/mtdblock.c
+++ b/drivers/mtd/mtdblock.c
@@ -368,7 +368,6 @@ static void mtdblock_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
static void mtdblock_remove_dev(struct mtd_blktrans_dev *dev)
{
del_mtd_blktrans_dev(dev);
- kfree(dev);
}

static struct mtd_blktrans_ops mtdblock_tr = {
diff --git a/drivers/mtd/mtdblock_ro.c b/drivers/mtd/mtdblock_ro.c
index 852165f..54ff288 100644
--- a/drivers/mtd/mtdblock_ro.c
+++ b/drivers/mtd/mtdblock_ro.c
@@ -49,7 +49,6 @@ static void mtdblock_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
static void mtdblock_remove_dev(struct mtd_blktrans_dev *dev)
{
del_mtd_blktrans_dev(dev);
- kfree(dev);
}

static struct mtd_blktrans_ops mtdblock_tr = {
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 96b4a05..0e86208 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -540,7 +540,6 @@ void put_mtd_device(struct mtd_info *mtd)
__put_mtd_device(mtd);
mutex_unlock(&mtd_table_mutex);

- module_put(mtd->owner);
}

void __put_mtd_device(struct mtd_info *mtd)
@@ -550,6 +549,8 @@ void __put_mtd_device(struct mtd_info *mtd)

if (mtd->put_device)
mtd->put_device(mtd);
+
+ module_put(mtd->owner);
}

/* default_mtd_writev - default mtd writev method for MTD devices that
diff --git a/drivers/mtd/nftlcore.c b/drivers/mtd/nftlcore.c
index 1002e18..a4578bf 100644
--- a/drivers/mtd/nftlcore.c
+++ b/drivers/mtd/nftlcore.c
@@ -126,7 +126,6 @@ static void nftl_remove_dev(struct mtd_blktrans_dev *dev)
del_mtd_blktrans_dev(dev);
kfree(nftl->ReplUnitTable);
kfree(nftl->EUNtable);
- kfree(nftl);
}

/*
diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
index d2aa9c4..63b83c0 100644
--- a/drivers/mtd/rfd_ftl.c
+++ b/drivers/mtd/rfd_ftl.c
@@ -817,7 +817,6 @@ static void rfd_ftl_remove_dev(struct mtd_blktrans_dev *dev)
vfree(part->sector_map);
kfree(part->header_cache);
kfree(part->blocks);
- kfree(part);
}

static struct mtd_blktrans_ops rfd_ftl_tr = {
diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
index 3f67e00..81c4ecd 100644
--- a/drivers/mtd/ssfdc.c
+++ b/drivers/mtd/ssfdc.c
@@ -375,7 +375,6 @@ static void ssfdcr_remove_dev(struct mtd_blktrans_dev *dev)

del_mtd_blktrans_dev(dev);
kfree(ssfdc->logic_block_map);
- kfree(ssfdc);
}

static int ssfdcr_readsect(struct mtd_blktrans_dev *dev,
diff --git a/include/linux/mtd/blktrans.h b/include/linux/mtd/blktrans.h
index 8b4aa05..054b053 100644
--- a/include/linux/mtd/blktrans.h
+++ b/include/linux/mtd/blktrans.h
@@ -24,10 +24,15 @@ struct mtd_blktrans_dev {
int devnum;
unsigned long size;
int readonly;
- void *blkcore_priv; /* gendisk in 2.5, devfs_handle in 2.4 */
-};
+ int deleted;
+ int open;

-struct blkcore_priv; /* Differs for 2.4 and 2.5 kernels; private */
+ struct gendisk *disk;
+ struct task_struct *thread;
+ struct request_queue *rq;
+ spinlock_t queue_lock;
+ void *priv;
+};

struct mtd_blktrans_ops {
char *name;
@@ -60,8 +65,6 @@ struct mtd_blktrans_ops {
struct list_head devs;
struct list_head list;
struct module *owner;
-
- struct mtd_blkcore_priv *blkcore_priv;
};

extern int register_mtd_blktrans(struct mtd_blktrans_ops *tr);
--
1.6.3.3


2010-01-22 15:36:13

by Maxim Levitsky

[permalink] [raw]
Subject: [PATCH 4/4] MTD: make mtdtrans thread freezeable.

>From 0904b692e95b22564a570e03956e89911a5e56b3 Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <[email protected]>
Date: Fri, 22 Jan 2010 15:00:55 +0200
Subject: [PATCH 4/4] MTD: make mtdtrans thread freezeable.

This makes the mtd blktrans thread enter the freezer in between accesses
to nand device.
This will ensure that we aren't suspening the system in the middle
of page read/write and even worse erase, which is a bad idea.

Signed-off-by: Maxim Levitsky <[email protected]>
---
drivers/mtd/mtd_blkdevs.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 04a875f..db996d6 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -20,6 +20,7 @@
#include <linux/init.h>
#include <linux/mutex.h>
#include <linux/kthread.h>
+#include <linux/freezer.h>
#include <asm/uaccess.h>

#include "mtdcore.h"
@@ -79,36 +80,35 @@ static int mtd_blktrans_thread(void *arg)
struct request_queue *rq = dev->rq;
struct request *req = NULL;

- spin_lock_irq(rq->queue_lock);
+ set_freezable();

while (!kthread_should_stop()) {
int res;

+ try_to_freeze();
+
+ spin_lock_irq(rq->queue_lock);
if (!req && !(req = blk_fetch_request(rq))) {
set_current_state(TASK_INTERRUPTIBLE);
spin_unlock_irq(rq->queue_lock);
schedule();
- spin_lock_irq(rq->queue_lock);
continue;
}
-
spin_unlock_irq(rq->queue_lock);

+
mutex_lock(&dev->lock);
res = do_blktrans_request(dev->tr, dev, req);
mutex_unlock(&dev->lock);

spin_lock_irq(rq->queue_lock);
-
if (!__blk_end_request_cur(req, res))
req = NULL;
+ spin_unlock_irq(rq->queue_lock);
}

if (req)
__blk_end_request_all(req, -EIO);
-
- spin_unlock_irq(rq->queue_lock);
-
return 0;
}

--
1.6.3.3


2010-01-22 16:01:00

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 3/4] MTD: blkdevs: make hotplug work

On Fri, 2010-01-22 at 17:35 +0200, Maxim Levitsky wrote:
> >From 368127067f4d39ba89096e4a5cb3be2dee361bad Mon Sep 17 00:00:00 2001
> From: Maxim Levitsky <[email protected]>
> Date: Fri, 22 Jan 2010 15:00:55 +0200
> Subject: [PATCH 3/4] MTD: blkdevs: make hotplug work
>
> This changes the blkdev common module for translation layers
> to survive when underlying mtd device disappears.
>
> To do so the following conceptual changes were made:
>
> * disk queue and thread are now one per mtd device
> This was it is easy to flush and destroy the queue
>
> * the struct mtd_blktrans_dev will now be freed automaticly when last user
> of the device quits.
> All existing translation layers are adjusted
>
> * ->open and release function of the translation layer will never be called
> twise or more in the row.
> This makes code simplier.
> Also the ->release will be called just before mtd device disappears
> This and above is the only visable changes on the outside.
>
> Tested with mtdblock, ssfdc and my own sm_ftl on top of physicly hotplugable
> nand card.
>
> Signed-off-by: Maxim Levitsky <[email protected]>

I'll try to find some time to review this later - I'm really busy
lately. But please, split this huge patch on smaller pieces - this will
help the reviewers a lot.

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

2010-01-22 21:51:26

by Maxim Levitsky

[permalink] [raw]
Subject: Re: [PATCH 3/4] MTD: blkdevs: make hotplug work

On Fri, 2010-01-22 at 17:59 +0200, Artem Bityutskiy wrote:
> On Fri, 2010-01-22 at 17:35 +0200, Maxim Levitsky wrote:
> > >From 368127067f4d39ba89096e4a5cb3be2dee361bad Mon Sep 17 00:00:00 2001
> > From: Maxim Levitsky <[email protected]>
> > Date: Fri, 22 Jan 2010 15:00:55 +0200
> > Subject: [PATCH 3/4] MTD: blkdevs: make hotplug work
> >
> > This changes the blkdev common module for translation layers
> > to survive when underlying mtd device disappears.
> >
> > To do so the following conceptual changes were made:
> >
> > * disk queue and thread are now one per mtd device
> > This was it is easy to flush and destroy the queue
> >
> > * the struct mtd_blktrans_dev will now be freed automaticly when last user
> > of the device quits.
> > All existing translation layers are adjusted
> >
> > * ->open and release function of the translation layer will never be called
> > twise or more in the row.
> > This makes code simplier.
> > Also the ->release will be called just before mtd device disappears
> > This and above is the only visable changes on the outside.
> >
> > Tested with mtdblock, ssfdc and my own sm_ftl on top of physicly hotplugable
> > nand card.
> >
> > Signed-off-by: Maxim Levitsky <[email protected]>
>
> I'll try to find some time to review this later - I'm really busy
> lately. But please, split this huge patch on smaller pieces - this will
> help the reviewers a lot.
>
Sure, but I afraid this won't be easy. To split the patch I will need
effectively to redo all the work from the beginning.
Isn't it possible just to review the mtd_blkdevs.c and blktrans.h as
if it was new driver?

This patch does touch all ftl drivers, but only removes the final free
of structure that holds or embeds the 'struct mtd_blktrans_dev'

Change in drivers/mtd/mtdcore.c is my mistake, this slipped from
previous patch, sorry.

Do you think its real to merge my work into 2.6.34 ?
What else should I do besides splitting this patch?

Best regards,
Maxim Levitsky