force module loaded with partitions set

PATCH In review Yang Ruirui 2 participants 9 messages Mar 14, 2011 – Mar 17, 2011 Memory technology devices (MTD)

lore

Share & tools

lore

Yang Ruirui

author

2011-03-14 01:42 UTC #0 lore raw

PATCH 2/2 force module loaded with partitions set

From: Yang Ruirui<[email protected]>

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<[email protected]>

Tested-by Shao Yanqing<[email protected]>

Tested-by Xiao Yang<[email protected]>

---
 drivers/mtd/mtdswap.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/mtd/mtdswap.c +6 -0

@@ -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);
 }

Artem Bityutskiy

2011-03-14 08:45 UTC #1 lore raw

Quoted 17 lines · Yang Ruirui

From: Yang Ruirui<[email protected]>
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<[email protected]>
Tested-by Shao Yanqing<[email protected]>
Tested-by Xiao Yang<[email protected]>
---
 drivers/mtd/mtdswap.c |    6 ++++++
 1 file changed, 6 insertions(+)

drivers/mtd/mtdswap.c static struct mtd_blktrans_ops mtdswap_o · line 1576

Quoted 8 lines

drivers/mtd/mtdswap.c +5 -0

@@ -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() ?

Signature · 2 lines

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

Yang Rui Rui

author

2011-03-14 08:47 UTC #2 lore raw

Quoted 19 lines · Artem Bityutskiy

Quoted 18 lines · Yang Ruirui
From: Yang Ruirui<[email protected]>
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<[email protected]>
Tested-by Shao Yanqing<[email protected]>
Tested-by Xiao Yang<[email protected]>
---
  drivers/mtd/mtdswap.c |    6 ++++++
  1 file changed, 6 insertions(+)
drivers/mtd/mtdswap.c +0 -0
@@ -1569,6 +1569,12 @@ static struct mtd_blktrans_ops mtdswap_o
 

Quoted 11 lines

drivers/mtd/mtdswap.c static struct mtd_blktrans_ops mtdswap_o · line 1576
Quoted 7 lines
drivers/mtd/mtdswap.c +5 -0
  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.

Signature · 2 lines

Thanks
Yang Ruirui

Artem Bityutskiy

2011-03-14 08:55 UTC #3 lore raw

Quoted 36 lines · Yang Rui Rui

Quoted 4 lines · Artem Bityutskiy
Quoted 3 lines · Yang Ruirui
From: Yang Ruirui<[email protected]>
partitions can not be set after module loaded, the moduel param
mode is 0444.
Quoted 2 lines
Quoted 2 lines
this patch force module loaded with param partitions set, if user
does not
Quoted 10 lines
Quoted 10 lines
set partitions then give out a warning and return -EINVAL
Signed-off-by Yang Ruirui<[email protected]>
Tested-by Shao Yanqing<[email protected]>
Tested-by Xiao Yang<[email protected]>
---
  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
Quoted 1 line
Quoted 1 line
+++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14
09:46:30.229993534 +0800
Quoted 13 lines
drivers/mtd/mtdswap.c static struct mtd_blktrans_ops mtdswap_o · line 1576
Quoted 9 lines
drivers/mtd/mtdswap.c +5 -0
@@ -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() ?

Quoted 3 lines

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.

Signature · 2 lines

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

Yang Rui Rui

author

2011-03-14 09:03 UTC #4 lore raw

Quoted 41 lines · Artem Bityutskiy

Quoted 36 lines · Yang Rui Rui
Quoted 4 lines · Artem Bityutskiy
Quoted 3 lines · Yang Ruirui
From: Yang Ruirui<[email protected]>
partitions can not be set after module loaded, the moduel param
mode is 0444.
Quoted 2 lines
Quoted 2 lines
this patch force module loaded with param partitions set, if user
does not
Quoted 10 lines
Quoted 10 lines
set partitions then give out a warning and return -EINVAL
Signed-off-by Yang Ruirui<[email protected]>
Tested-by Shao Yanqing<[email protected]>
Tested-by Xiao Yang<[email protected]>
---
   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
Quoted 1 line
Quoted 1 line
+++ mtd-2.6-fc2ff59/drivers/mtd/mtdswap.c 2011-03-14
09:46:30.229993534 +0800
Quoted 13 lines
drivers/mtd/mtdswap.c static struct mtd_blktrans_ops mtdswap_o · line 1576
Quoted 9 lines
drivers/mtd/mtdswap.c +5 -0
@@ -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() ?
Quoted 3 lines
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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Signature · 2 lines

Thanks
Yang Ruirui

Artem Bityutskiy

2011-03-14 09:35 UTC #5 lore raw

Quoted 6 lines · Yang Rui Rui

Quoted 4 lines
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.

Signature · 2 lines

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

Yang Rui Rui

author

2011-03-17 08:40 UTC #6 lore raw

Quoted 10 lines · Artem Bityutskiy

Quoted 6 lines · Yang Rui Rui
Quoted 4 lines
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.

Signature · 2 lines

Thanks
Yang Ruirui

Artem Bityutskiy

2011-03-17 08:53 UTC #7 lore raw

Quoted 9 lines · Yang Rui Rui

Quoted 8 lines · Artem Bityutskiy
Quoted 6 lines · Yang Rui Rui
Quoted 4 lines
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?

Quoted 8 lines

Quoted 2 lines
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

Signature · 2 lines

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

Yang Rui Rui

author

2011-03-17 09:14 UTC #8 lore raw

Quoted 16 lines · Artem Bityutskiy

Quoted 15 lines · Yang Rui Rui
Quoted 10 lines · Artem Bityutskiy
Quoted 6 lines · Yang Rui Rui
Quoted 4 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Signature · 2 lines

Thanks
Yang Ruirui