| Message ID | 20110314015148.GA6827@darkstar |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote: > From: Yang Ruirui<ruirui.r.yang@tieto.com> > > partitions can not be set after module loaded, the moduel param mode is 0444. > > this patch force module loaded with param partitions set, if user does not > set partitions then give out a warning and return -EINVAL > > Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com> > Tested-by: Shao Yanqing<yanqing.shao@tieto.com> > Tested-by: Xiao Yang<yang.xiao@tieto.com> > --- > drivers/mtd/mtdswap.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c 2011-03-14 09:36:09.283329099 +0800 > +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14 09:46:30.229993534 +0800 > @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o > > static int __init mtdswap_modinit(void) > { > + if (!partitions[0]) { > + printk(KERN_WARNING > + "Please load mtdswap with correct partitions param\n"); > + return -EINVAL; > + } I think a similar check is done in mtdswap_add_mtd() ?
On Mon, 2011-03-14 at 16:57 +0800, Yang Rui Rui wrote: > On 03/14/2011 04:43 PM, Artem Bityutskiy wrote: > > On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote: > >> From: Yang Ruirui<ruirui.r.yang@tieto.com> > >> > >> partitions can not be set after module loaded, the moduel param > mode is 0444. > >> > >> this patch force module loaded with param partitions set, if user > does not > >> set partitions then give out a warning and return -EINVAL > >> > >> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com> > >> Tested-by: Shao Yanqing<yanqing.shao@tieto.com> > >> Tested-by: Xiao Yang<yang.xiao@tieto.com> > >> --- > >> drivers/mtd/mtdswap.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c 2011-03-14 > 09:36:09.283329099 +0800 > >> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14 > 09:46:30.229993534 +0800 > >> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o > >> > >> static int __init mtdswap_modinit(void) > >> { > >> + if (!partitions[0]) { > >> + printk(KERN_WARNING > >> + "Please load mtdswap with correct partitions param\n"); > >> + return -EINVAL; > >> + } > > > > I think a similar check is done in mtdswap_add_mtd() ? > > > > > > Yes, that one should be removed if this is ok. This module just waste > memory without partitions set. And there's no chance to pass in the > params. I agree that the problem you are solving exist. But I think I do not agree with the solution. There are many other ways to end up with mtdswap module loaded but not functioning, e.g., users passes incorrect module parameters. Basically, any failed check in 'mtdswap_add_mtd()'. IOW, you are solving just one of many similar issues. I think you should rather re-work the framework a bit and make the "->add_mtd()" call-back return an integer error code. Then we could just return errors on error and prevent the module from being loaded.
On 03/14/2011 04:43 PM, Artem Bityutskiy wrote: > On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote: >> From: Yang Ruirui<ruirui.r.yang@tieto.com> >> >> partitions can not be set after module loaded, the moduel param mode is 0444. >> >> this patch force module loaded with param partitions set, if user does not >> set partitions then give out a warning and return -EINVAL >> >> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com> >> Tested-by: Shao Yanqing<yanqing.shao@tieto.com> >> Tested-by: Xiao Yang<yang.xiao@tieto.com> >> --- >> drivers/mtd/mtdswap.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c 2011-03-14 09:36:09.283329099 +0800 >> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14 09:46:30.229993534 +0800 >> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o >> >> static int __init mtdswap_modinit(void) >> { >> + if (!partitions[0]) { >> + printk(KERN_WARNING >> + "Please load mtdswap with correct partitions param\n"); >> + return -EINVAL; >> + } > > I think a similar check is done in mtdswap_add_mtd() ? > > Yes, that one should be removed if this is ok. This module just waste memory without partitions set. And there's no chance to pass in the params.
On 03/14/2011 04:53 PM, Artem Bityutskiy wrote: > On Mon, 2011-03-14 at 16:57 +0800, Yang Rui Rui wrote: >> On 03/14/2011 04:43 PM, Artem Bityutskiy wrote: >>> On Mon, 2011-03-14 at 09:51 +0800, Yang Ruirui wrote: >>>> From: Yang Ruirui<ruirui.r.yang@tieto.com> >>>> >>>> partitions can not be set after module loaded, the moduel param >> mode is 0444. >>>> >>>> this patch force module loaded with param partitions set, if user >> does not >>>> set partitions then give out a warning and return -EINVAL >>>> >>>> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com> >>>> Tested-by: Shao Yanqing<yanqing.shao@tieto.com> >>>> Tested-by: Xiao Yang<yang.xiao@tieto.com> >>>> --- >>>> drivers/mtd/mtdswap.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> --- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c 2011-03-14 >> 09:36:09.283329099 +0800 >>>> +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14 >> 09:46:30.229993534 +0800 >>>> @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o >>>> >>>> static int __init mtdswap_modinit(void) >>>> { >>>> + if (!partitions[0]) { >>>> + printk(KERN_WARNING >>>> + "Please load mtdswap with correct partitions param\n"); >>>> + return -EINVAL; >>>> + } >>> >>> I think a similar check is done in mtdswap_add_mtd() ? >>> >>> >> >> Yes, that one should be removed if this is ok. This module just waste >> memory without partitions set. And there's no chance to pass in the >> params. > > I agree that the problem you are solving exist. But I think I do not > agree with the solution. There are many other ways to end up with > mtdswap module loaded but not functioning, e.g., users passes incorrect > module parameters. Basically, any failed check in 'mtdswap_add_mtd()'. > > IOW, you are solving just one of many similar issues. Right, agree. > > I think you should rather re-work the framework a bit and make the > "->add_mtd()" call-back return an integer error code. Then we could just > return errors on error and prevent the module from being loaded. > I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute? Anyway, I will check the code again to see if can change as your suggestion.
On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote: > > I think you should rather re-work the framework a bit and make the > > "->add_mtd()" call-back return an integer error code. Then we could just > > return errors on error and prevent the module from being loaded. > > > > I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute? It may be possible, but would probably require some work.
On 03/14/2011 05:33 PM, Artem Bityutskiy wrote: > On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote: >>> I think you should rather re-work the framework a bit and make the >>> "->add_mtd()" call-back return an integer error code. Then we could just >>> return errors on error and prevent the module from being loaded. >>> >> >> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute? > > It may be possible, but would probably require some work. > mainline mtd & mtd_blkdev changes a lot since 2.6.32, but there's no way for me to test with linux-mtd tree due to lack real hardware. work on this with 2.6.32 seems not so necessary, So I think I will give up, sorry about that.
On Thu, 2011-03-17 at 16:50 +0800, Yang Rui Rui wrote: > On 03/14/2011 05:33 PM, Artem Bityutskiy wrote: > > On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote: > >>> I think you should rather re-work the framework a bit and make the > >>> "->add_mtd()" call-back return an integer error code. Then we could just > >>> return errors on error and prevent the module from being loaded. > >>> > >> > >> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute? > > > > It may be possible, but would probably require some work. > > > > mainline mtd & mtd_blkdev changes a lot since 2.6.32, but there's no > way for me to test with linux-mtd tree due to lack real hardware. > > work on this with 2.6.32 seems not so necessary, > So I think I will give up, sorry about that. No problem, but as a side note, you can always test on a PC with nandsim. Here is some UBI-related doc, but it gives the idea: http://www.linux-mtd.infradead.org/faq/ubi.html#L_how_debug
On 03/17/2011 04:51 PM, Artem Bityutskiy wrote: > On Thu, 2011-03-17 at 16:50 +0800, Yang Rui Rui wrote: >> On 03/14/2011 05:33 PM, Artem Bityutskiy wrote: >>> On Mon, 2011-03-14 at 17:12 +0800, Yang Rui Rui wrote: >>>>> I think you should rather re-work the framework a bit and make the >>>>> "->add_mtd()" call-back return an integer error code. Then we could just >>>>> return errors on error and prevent the module from being loaded. >>>>> >>>> >>>> I'm thinking is it possible to pass partitions param later? ie. use sysfs attribute? >>> >>> It may be possible, but would probably require some work. >>> >> >> mainline mtd& mtd_blkdev changes a lot since 2.6.32, but there's no >> way for me to test with linux-mtd tree due to lack real hardware. >> >> work on this with 2.6.32 seems not so necessary, >> So I think I will give up, sorry about that. > > No problem, but as a side note, you can always test on a PC with > nandsim. Here is some UBI-related doc, but it gives the idea: > > http://www.linux-mtd.infradead.org/faq/ubi.html#L_how_debug > very useful for playing with mtd, thank you.
--- mtd-2.6-fc2ff59.orig/drivers/mtd/mtdswap.c 2011-03-14 09:36:09.283329099 +0800 +++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14 09:46:30.229993534 +0800 @@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o static int __init mtdswap_modinit(void) { + if (!partitions[0]) { + printk(KERN_WARNING + "Please load mtdswap with correct partitions param\n"); + return -EINVAL; + } + return register_mtd_blktrans(&mtdswap_ops); }