| Message ID | 1265734665-22656-2-git-send-email-maximlevitsky@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote: > These functions can be used as long as we don't need access to global mtd table, but have > a pointer to the mtd device. > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com> > --- > drivers/mtd/mtdcore.c | 60 ++++++++++++++++++++++++++++++---------------- > include/linux/mtd/mtd.h | 3 +- > 2 files changed, 41 insertions(+), 22 deletions(-) > +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; > } > +void __put_mtd_device(struct mtd_info *mtd) > +{ > + --mtd->usecount; > + BUG_ON(mtd->usecount < 0); > + > if (mtd->put_device) > mtd->put_device(mtd); > > module_put(mtd->owner); > } That's racy, use kref.
On Tue, 2010-02-09 at 18:14 +0100, Peter Zijlstra wrote: > On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote: > > These functions can be used as long as we don't need access to global mtd table, but have > > a pointer to the mtd device. > > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com> > > --- > > drivers/mtd/mtdcore.c | 60 ++++++++++++++++++++++++++++++---------------- > > include/linux/mtd/mtd.h | 3 +- > > 2 files changed, 41 insertions(+), 22 deletions(-) > > > +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; > > } > > > +void __put_mtd_device(struct mtd_info *mtd) > > +{ > > + --mtd->usecount; > > + BUG_ON(mtd->usecount < 0); > > + > > if (mtd->put_device) > > mtd->put_device(mtd); > > > > module_put(mtd->owner); > > } > > That's racy, use kref. > Couldn't agree with you more. However, these functions aren't intended for general use, and probably will be used by mtd translation layer only. I do have a lock that protects concurrent use of these functions. Thus, I better add a comment about this? Best regards, Maxim Levitsky
On Tue, 2010-02-09 at 19:23 +0200, Maxim Levitsky wrote: > On Tue, 2010-02-09 at 18:14 +0100, Peter Zijlstra wrote: > > On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote: > > > These functions can be used as long as we don't need access to global mtd table, but have > > > a pointer to the mtd device. > > > > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com> > > > --- > > > drivers/mtd/mtdcore.c | 60 ++++++++++++++++++++++++++++++---------------- > > > include/linux/mtd/mtd.h | 3 +- > > > 2 files changed, 41 insertions(+), 22 deletions(-) > > > > > +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; > > > } > > > > > +void __put_mtd_device(struct mtd_info *mtd) > > > +{ > > > + --mtd->usecount; > > > + BUG_ON(mtd->usecount < 0); > > > + > > > if (mtd->put_device) > > > mtd->put_device(mtd); > > > > > > module_put(mtd->owner); > > > } > > > > That's racy, use kref. > > > Couldn't agree with you more. > > However, these functions aren't intended for general use, and probably > will be used by mtd translation layer only. I do have a lock that > protects concurrent use of these functions. > > Thus, I better add a comment about this? However on second thought, there is still a race if two FTLs access same mtd device. While this might seem impossible, and I say it is quite dangerous. I can imagine using both some FTL and mtdblock for testing. Just using kref (nothing against it) won't help here. The mtd->get_device/put_device aren't expecting to be called concurrently ether... I can add per mtd lock, but it is a bit ugly... What do you think? Best regards, Maxim Levitsky
On Tue, 2010-02-09 at 19:46 +0200, Maxim Levitsky wrote: > On Tue, 2010-02-09 at 19:23 +0200, Maxim Levitsky wrote: > > On Tue, 2010-02-09 at 18:14 +0100, Peter Zijlstra wrote: > > > On Tue, 2010-02-09 at 18:57 +0200, Maxim Levitsky wrote: > > > > These functions can be used as long as we don't need access to global mtd table, but have > > > > a pointer to the mtd device. > > > > > > > > Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com> > > > > --- David, I just got a great idea how to resolve the same issue in much cleaner way. Why not to drop the mtd table lock before calling the add notifers? If we assume no add_mtd_device/del_mtd_device running concurrently this should be ok. After all mtd table lock should protect only the mtd table. And the table only purpose it to locate an mtd device by number or name. This purpose will always be useful, be it a static table or not. I also remove all that mtd table junk from mtd_blktrans.c Best regards, Maxim Levitsky
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index c356c0a..3bdb7e8 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -453,27 +453,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; } /** @@ -524,14 +535,19 @@ out_unlock: void put_mtd_device(struct mtd_info *mtd) { - int c; - mutex_lock(&mtd_table_mutex); - c = --mtd->usecount; + __put_mtd_device(mtd); + mutex_unlock(&mtd_table_mutex); + +} + +void __put_mtd_device(struct mtd_info *mtd) +{ + --mtd->usecount; + BUG_ON(mtd->usecount < 0); + if (mtd->put_device) mtd->put_device(mtd); - mutex_unlock(&mtd_table_mutex); - BUG_ON(c < 0); module_put(mtd->owner); } @@ -569,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);
These functions can be used as long as we don't need access to global mtd table, but have a pointer to the mtd device. Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com> --- drivers/mtd/mtdcore.c | 60 ++++++++++++++++++++++++++++++---------------- include/linux/mtd/mtd.h | 3 +- 2 files changed, 41 insertions(+), 22 deletions(-)