[PATCH 02/02] force module loaded with partitions set

[PATCH 02/02] force module loaded with partitions set

[Yang Ruirui]

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;
+	}
+
 	return register_mtd_blktrans(&mtdswap_ops);
 }
 

Re: [PATCH 02/02] force module loaded with partitions set

[Artem Bityutskiy]

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


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

Re: [PATCH 02/02] force module loaded with partitions set

[Yang Rui Rui]

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.


-- 
Thanks
Yang Ruirui

Re: [PATCH 02/02] force module loaded with partitions set

[Artem Bityutskiy]

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.

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

Re: [PATCH 02/02] force module loaded with partitions set

[Yang Rui Rui]

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.

-- 
Thanks
Yang Ruirui

Re: [PATCH 02/02] force module loaded with partitions set

[Artem Bityutskiy]

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.

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

Re: [PATCH 02/02] force module loaded with partitions set

[Yang Rui Rui]

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.

-- 
Thanks
Yang Ruirui

Re: [PATCH 02/02] force module loaded with partitions set

[Artem Bityutskiy]

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

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

Re: [PATCH 02/02] force module loaded with partitions set

[Yang Rui Rui]

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.

-- 
Thanks
Yang Ruirui