[PATCH] mtd: phram: error handling

[PATCH] mtd: phram: error handling

[Saurabh Sengar]

registering the device with NULL pointer can lead to crash,
hence fixing it.

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
in case of 'illegal start address' or 'illegal device length', name pointer is getting freed.
we shouldn't register the device with NULL pointer. 

 drivers/mtd/devices/phram.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 8b66e52..9a7aed3 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -249,12 +249,14 @@ static int phram_setup(const char *val)
 	if (ret) {
 		kfree(name);
 		parse_err("illegal start address\n");
+		goto err;
 	}
 
 	ret = parse_num64(&len, token[2]);
 	if (ret) {
 		kfree(name);
 		parse_err("illegal device length\n");
+		goto err;
 	}
 
 	ret = register_device(name, start, len);
@@ -262,7 +264,7 @@ static int phram_setup(const char *val)
 		pr_info("%s device: %#llx at %#llx\n", name, len, start);
 	else
 		kfree(name);
-
+err:
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH] mtd: phram: error handling

[Andy Shevchenko]

On Sun, Nov 8, 2015 at 10:47 AM, Saurabh Sengar <saurabh.truth@gmail.com> wrote:
> registering the device with NULL pointer can lead to crash,
> hence fixing it.

Hmm… Why not just checking it before an register attempt? I think user
is in right to know as many problems as they have at one shot, with
your patch if there are two problems the user has to try twice.

>
> Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
> ---
> in case of 'illegal start address' or 'illegal device length', name pointer is getting freed.
> we shouldn't register the device with NULL pointer.
>
>  drivers/mtd/devices/phram.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 8b66e52..9a7aed3 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -249,12 +249,14 @@ static int phram_setup(const char *val)
>         if (ret) {
>                 kfree(name);
>                 parse_err("illegal start address\n");
> +               goto err;
>         }
>
>         ret = parse_num64(&len, token[2]);
>         if (ret) {
>                 kfree(name);
>                 parse_err("illegal device length\n");
> +               goto err;
>         }
>
>         ret = register_device(name, start, len);
> @@ -262,7 +264,7 @@ static int phram_setup(const char *val)
>                 pr_info("%s device: %#llx at %#llx\n", name, len, start);
>         else
>                 kfree(name);
> -
> +err:
>         return ret;
>  }
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] mtd: phram: error handling

[Saurabh Sengar]

registering the device with NULL pointer can lead to crash,
hence fixing it

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
> Andy Shevchenko wrote:
> Hmm… Why not just checking it before an register attempt? I think user
> is in right to know as many problems as they have at one shot, with
> your patch if there are two problems the user has to try twice.
Yes, taken your feedback, fixing it here in v2 as you recommended

 drivers/mtd/devices/phram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 8b66e52..46b7a8a 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -257,6 +257,9 @@ static int phram_setup(const char *val)
 		parse_err("illegal device length\n");
 	}
 
+	if(!name)
+		return -EINVAL;
+
 	ret = register_device(name, start, len);
 	if (!ret)
 		pr_info("%s device: %#llx at %#llx\n", name, len, start);
-- 
1.9.1

Re: [PATCH v2] mtd: phram: error handling

[Brian Norris]

On Mon, Nov 09, 2015 at 11:53:18AM +0530, Saurabh Sengar wrote:
> registering the device with NULL pointer can lead to crash,
> hence fixing it
> 
> Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
> ---
> > Andy Shevchenko wrote:
> > Hmm… Why not just checking it before an register attempt? I think user
> > is in right to know as many problems as they have at one shot, with
> > your patch if there are two problems the user has to try twice.
> Yes, taken your feedback, fixing it here in v2 as you recommended
> 
>  drivers/mtd/devices/phram.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 8b66e52..46b7a8a 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -257,6 +257,9 @@ static int phram_setup(const char *val)
>  		parse_err("illegal device length\n");
>  	}
>  
> +	if(!name)
> +		return -EINVAL;

I'm not sure how this is supposed to do anything... just because you
kfree()'d the name doesn't mean it is NULL. In fact, I don't see how
you'd get name==NULL at all. It is assigned once (in parse_name()), and
if it's NULL, we already exit early. And 'name' is never modified after
that point.

So... did you test your patch?

(*looks at the existing code a bit more*)

Hey, I think your patch is all futile anyway. Did you notice that
there's a "return" statement embedded in the parse_err() macro? So there
was no bug in the first place, and I think you're just blowing smoke.

Please verify that you're actually fixing bugs, and please test your
patches. Otherwise, you're wasting my time.

Brian

> +
>  	ret = register_device(name, start, len);
>  	if (!ret)
>  		pr_info("%s device: %#llx at %#llx\n", name, len, start);
> -- 
> 1.9.1
> 

[PATCH] mtd: phram: error handling

[Joe Perches]

Expand parse_err macro with hidden flow in-place.
Remove the now unused parse_err macro.

Miscellanea:

o Use invalid not illegal for error messages

Noticed-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
> Did you notice that
> there's a "return" statement embedded in the parse_err() macro? So there
> was no bug in the first place.

 drivers/mtd/devices/phram.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 8b66e52..d93b85e 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -199,11 +199,6 @@ static inline void kill_final_newline(char *str)
 }
 
 
-#define parse_err(fmt, args...) do {	\
-	pr_err(fmt , ## args);	\
-	return 1;		\
-} while (0)
-
 #ifndef MODULE
 static int phram_init_called;
 /*
@@ -226,8 +221,10 @@ static int phram_setup(const char *val)
 	uint64_t len;
 	int i, ret;
 
-	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
-		parse_err("parameter too long\n");
+	if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
+		pr_err("parameter too long\n");
+		return 1;
+	}
 
 	strcpy(str, val);
 	kill_final_newline(str);
@@ -235,11 +232,15 @@ static int phram_setup(const char *val)
 	for (i = 0; i < 3; i++)
 		token[i] = strsep(&str, ",");
 
-	if (str)
-		parse_err("too many arguments\n");
+	if (str) {
+		pr_err("too many arguments\n");
+		return 1;
+	}
 
-	if (!token[2])
-		parse_err("not enough arguments\n");
+	if (!token[2]) {
+		pr_err("not enough arguments\n");
+		return 1;
+	}
 
 	ret = parse_name(&name, token[0]);
 	if (ret)
@@ -248,13 +249,15 @@ static int phram_setup(const char *val)
 	ret = parse_num64(&start, token[1]);
 	if (ret) {
 		kfree(name);
-		parse_err("illegal start address\n");
+		pr_err("invalid start address\n");
+		return 1;
 	}
 
 	ret = parse_num64(&len, token[2]);
 	if (ret) {
 		kfree(name);
-		parse_err("illegal device length\n");
+		pr_err("invalid device length\n");
+		return 1;
 	}
 
 	ret = register_device(name, start, len);

Re: [PATCH] mtd: phram: error handling

[Brian Norris]

On Tue, Nov 10, 2015 at 10:33:07AM -0800, Joe Perches wrote:
> Expand parse_err macro with hidden flow in-place.
> Remove the now unused parse_err macro.

Quick one... thanks, I guess.

> Miscellanea:
> 
> o Use invalid not illegal for error messages
> 
> Noticed-by: Brian Norris <computersforpeace@gmail.com>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> > Did you notice that
> > there's a "return" statement embedded in the parse_err() macro? So there
> > was no bug in the first place.

I forgot to add to that last sentence "except for a readability bug."
Thanks for following up.

>  drivers/mtd/devices/phram.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 8b66e52..d93b85e 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -199,11 +199,6 @@ static inline void kill_final_newline(char *str)
>  }
>  
>  
> -#define parse_err(fmt, args...) do {	\
> -	pr_err(fmt , ## args);	\
> -	return 1;		\
> -} while (0)
> -
>  #ifndef MODULE
>  static int phram_init_called;
>  /*
> @@ -226,8 +221,10 @@ static int phram_setup(const char *val)
>  	uint64_t len;
>  	int i, ret;
>  
> -	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
> -		parse_err("parameter too long\n");
> +	if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
> +		pr_err("parameter too long\n");
> +		return 1;
> +	}
>  
>  	strcpy(str, val);
>  	kill_final_newline(str);
> @@ -235,11 +232,15 @@ static int phram_setup(const char *val)
>  	for (i = 0; i < 3; i++)
>  		token[i] = strsep(&str, ",");
>  
> -	if (str)
> -		parse_err("too many arguments\n");
> +	if (str) {
> +		pr_err("too many arguments\n");
> +		return 1;
> +	}
>  
> -	if (!token[2])
> -		parse_err("not enough arguments\n");
> +	if (!token[2]) {
> +		pr_err("not enough arguments\n");
> +		return 1;
> +	}
>  
>  	ret = parse_name(&name, token[0]);
>  	if (ret)
> @@ -248,13 +249,15 @@ static int phram_setup(const char *val)
>  	ret = parse_num64(&start, token[1]);
>  	if (ret) {
>  		kfree(name);
> -		parse_err("illegal start address\n");
> +		pr_err("invalid start address\n");
> +		return 1;
>  	}
>  
>  	ret = parse_num64(&len, token[2]);
>  	if (ret) {
>  		kfree(name);
> -		parse_err("illegal device length\n");
> +		pr_err("invalid device length\n");
> +		return 1;
>  	}
>  
>  	ret = register_device(name, start, len);
> 

Looks better to me, though I think -EINVAL makes more sense than 1. That
could be a subsequent patch, I suppose.

I'll wait to see if the original reporter has anything to say.

Regards,
Brian

Re: [PATCH] mtd: phram: error handling

[Joe Perches]

On Tue, 2015-11-10 at 10:39 -0800, Brian Norris wrote:
> On Tue, Nov 10, 2015 at 10:33:07AM -0800, Joe Perches wrote:
> > Expand parse_err macro with hidden flow in-place.
> > Remove the now unused parse_err macro.
[]
> I think -EINVAL makes more sense than 1. That
> could be a subsequent patch, I suppose.

That means you have to trace all the callers
to verify that converting 1 to -22 is acceptable.

Maybe Saurabh wants to do that.

Re: [PATCH] mtd: phram: error handling

[Brian Norris]

On Tue, Nov 10, 2015 at 10:45:55AM -0800, Joe Perches wrote:
> On Tue, 2015-11-10 at 10:39 -0800, Brian Norris wrote:
> > On Tue, Nov 10, 2015 at 10:33:07AM -0800, Joe Perches wrote:
> > > Expand parse_err macro with hidden flow in-place.
> > > Remove the now unused parse_err macro.
> []
> > I think -EINVAL makes more sense than 1. That
> > could be a subsequent patch, I suppose.
> 
> That means you have to trace all the callers
> to verify that converting 1 to -22 is acceptable.

It's fairly simple. Module initialization and module parameter calls
both *should* follow 0/negative error conventions. For module init, see
in kernel/module.c:

        /* Start the module */
        if (mod->init != NULL)
                ret = do_one_initcall(mod->init);
        if (ret < 0) {
                goto fail_free_freeinit;
        }
        if (ret > 0) {
                pr_warn("%s: '%s'->init suspiciously returned %d, it should "
                        "follow 0/-E convention\n"
                        "%s: loading module anyway...\n",
                        __func__, mod->name, ret, __func__);
                dump_stack();
        }

and in include/linux/moduleparam.h:

struct kernel_param_ops {
...
        /* Returns 0, or -errno.  arg is in kp->arg. */
        int (*set)(const char *val, const struct kernel_param *kp);
...
};

And for built-in modules, the return code is ignored (see
do_initcall_level()).

So I think the only question is whether we should actually be reporting
these errors on module insertion and on the module parameter call. I'd
say "definitely" to the latter and "yes" to the former, since the init
function already handles the case of an empty input (so the module can
be loaded with a blank command line without tripping on an -EINVAL
parameter check).

IOW, to use -EINVAL would be to actually enforce the error handling that
was intended in the first place. But that should be done in a separate
patch, and with an actual tester (since I doubt either you or Saurabh
are testing this driver).

Brian

[PATCH] mtd: phram: error handling

[Saurabh Sengar]

Expand parse_err macro with hidden flow in-place.
Remove the now unused parse_err macro.

Miscellanea:

o Use invalid not illegal for error messages

Noticed-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
>> I think -EINVAL makes more sense than 1. That
>> could be a subsequent patch, I suppose.

>That means you have to trace all the callers
>to verify that converting 1 to -22 is acceptable.

>Maybe Saurabh wants to do that.

I have checked that function is called only by init and module param,
and I understand in both the cases -EINVAL is a valid return type.
Sorry, I am not able to test the driver, sending the patch as asked above.
Also sorry for the noise I created in first report

 drivers/mtd/devices/phram.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 8b66e52..e39fe5c 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -199,11 +199,6 @@ static inline void kill_final_newline(char *str)
 }
 
 
-#define parse_err(fmt, args...) do {	\
-	pr_err(fmt , ## args);	\
-	return 1;		\
-} while (0)
-
 #ifndef MODULE
 static int phram_init_called;
 /*
@@ -226,8 +221,10 @@ static int phram_setup(const char *val)
 	uint64_t len;
 	int i, ret;
 
-	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
-		parse_err("parameter too long\n");
+	if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
+		pr_err("parameter too long\n");
+		return -EINVAL;
+	}
 
 	strcpy(str, val);
 	kill_final_newline(str);
@@ -235,11 +232,15 @@ static int phram_setup(const char *val)
 	for (i = 0; i < 3; i++)
 		token[i] = strsep(&str, ",");
 
-	if (str)
-		parse_err("too many arguments\n");
+	if (str) {
+		pr_err("too many arguments\n");
+		return -EINVAL;
+	}
 
-	if (!token[2])
-		parse_err("not enough arguments\n");
+	if (!token[2]) {
+		pr_err("not enough arguments\n");
+		return -EINVAL;
+	}
 
 	ret = parse_name(&name, token[0]);
 	if (ret)
@@ -248,13 +249,15 @@ static int phram_setup(const char *val)
 	ret = parse_num64(&start, token[1]);
 	if (ret) {
 		kfree(name);
-		parse_err("illegal start address\n");
+		pr_err("invalid start address\n");
+		return -EINVAL;
 	}
 
 	ret = parse_num64(&len, token[2]);
 	if (ret) {
 		kfree(name);
-		parse_err("illegal device length\n");
+		pr_err("invalid device length\n");
+		return -EINVAL;
 	}
 
 	ret = register_device(name, start, len);
-- 
1.9.1

Re: [PATCH] mtd: phram: error handling

[Brian Norris]

On Wed, Nov 11, 2015 at 12:57:55AM +0530, Saurabh Sengar wrote:
> Sorry, I am not able to test the driver, sending the patch as asked above.

Well, today you're in luck! You're touching a driver that requires no
special hardware, so you should be able to test it.

I think you can try using the mem= kernel parameter (see
Documentation/kernel-parameters.txt) to restrict your system memory, and
then try using that unreachable portions of memory for phram. You can
try this driver as either a module or built-in. For the former, you can
specify the parameters during modprobe time, or later by writing to
/sys/module/phram/parameters/phram. For the latter, you can specify on
the kernel command line or in /sys/module/phram/parameters/phram.

Let me know if you have any more questions about testing your patch.

Brian

[PATCH] mtd: phram: error handling

[Saurabh Sengar]

Brian Norris wrote:
> Well, today you're in luck! You're touching a driver that requires no
> special hardware, so you should be able to test it.

> I think you can try using the mem= kernel parameter (see
> Documentation/kernel-parameters.txt) to restrict your system memory, and
> then try using that unreachable portions of memory for phram. You can
> try this driver as either a module or built-in. For the former, you can
> specify the parameters during modprobe time, or later by writing to
> /sys/module/phram/parameters/phram. For the latter, you can specify on
> the kernel command line or in /sys/module/phram/parameters/phram.

> Let me know if you have any more questions about testing your patch.


Hi Brian,

As you have suggested I have restricted my laptop's memory to 1GB and tried to access unreachable portion of memory(2GB).
It seems to be successfully registered(this is OK right ?).

I have also tested below error scenarios; all exiting gracefully
  - parameter too long
  - too many arguments
  - not enough arguments
  - invalid start adddress
  - invalid device length


This is my first interaction with phram device so I request you to verify my testing.
Below are the commands and output I did for this testing.

-----------------------------------------------------
  <restricting memory of my machine to 1 GB>
saurabh@saurabh:~/little/Task02/linux$ cat /proc/meminfo | grep Mem
MemTotal:        1016588 kB
MemFree:          126016 kB
MemAvailable:     295508 kB
saurabh@saurabh:~/little/Task02/linux$ cat /proc/cmdline 
BOOT_IMAGE=/boot/vmlinuz-4.3.0-10333-gdfe4330-dirty mem=1024M root=UUID=2b66d4b2-ac21-4554-bf3b-64bc973e1dad ro quiet splash vt.handoff=7

  <accessing unreachable portions of memory>
saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=ram,2048Mi,1ki
saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
[  634.748495] phram: ram device: 0x400 at 0x80000000

  <parameter too long>
saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz,256Mi,1Mi
insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
[ 1469.763787] phram: parameter too long
[ 1469.763797] phram: `abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz,256Mi,1Mi' invalid for parameter `phram'


  <too many arguments>
saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256Mi,1Mi,extra_parameter
insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
[ 1650.081694] phram: too many arguments
[ 1650.081703] phram: `swap,256Mi,1Mi,extra_parameter' invalid for parameter `phram'

  <not enough arguments>
saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256Mi
insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
[ 1707.437130] phram: not enough arguments
[ 1707.437138] phram: `swap,256Mi' invalid for parameter `phram'


  <invalid start address>
saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256xyz,1Mi
insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
[ 1783.014351] phram: invalid start address
[ 1783.014359] phram: `swap,256xyz,1Mi' invalid for parameter `phram'

  <invalid device length>
saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256Mi,1xyz
insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
[ 1831.746108] phram: invalid device length
[ 1831.746117] phram: `swap,256Mi,1xyz' invalid for parameter `phram'
-------------------------------------------

Re: [PATCH] mtd: phram: error handling

[Brian Norris]

Hi Saurabh,

On Wed, Nov 11, 2015 at 01:53:58PM +0530, Saurabh Sengar wrote:
> Brian Norris wrote:
> > Well, today you're in luck! You're touching a driver that requires no
> > special hardware, so you should be able to test it.
> 
> > I think you can try using the mem= kernel parameter (see
> > Documentation/kernel-parameters.txt) to restrict your system memory, and
> > then try using that unreachable portions of memory for phram. You can
> > try this driver as either a module or built-in. For the former, you can
> > specify the parameters during modprobe time, or later by writing to
> > /sys/module/phram/parameters/phram. For the latter, you can specify on
> > the kernel command line or in /sys/module/phram/parameters/phram.
> 
> > Let me know if you have any more questions about testing your patch.
> 
> 
> Hi Brian,
> 
> As you have suggested I have restricted my laptop's memory to 1GB and tried to access unreachable portion of memory(2GB).
> It seems to be successfully registered(this is OK right ?).

Seems OK.

> I have also tested below error scenarios; all exiting gracefully
>   - parameter too long
>   - too many arguments
>   - not enough arguments
>   - invalid start adddress
>   - invalid device length

Nice! Glad to see you've tested quite a few good scenarios.

> This is my first interaction with phram device so I request you to verify my testing.
> Below are the commands and output I did for this testing.
> 
> -----------------------------------------------------
>   <restricting memory of my machine to 1 GB>
> saurabh@saurabh:~/little/Task02/linux$ cat /proc/meminfo | grep Mem
> MemTotal:        1016588 kB
> MemFree:          126016 kB
> MemAvailable:     295508 kB
> saurabh@saurabh:~/little/Task02/linux$ cat /proc/cmdline 
> BOOT_IMAGE=/boot/vmlinuz-4.3.0-10333-gdfe4330-dirty mem=1024M root=UUID=2b66d4b2-ac21-4554-bf3b-64bc973e1dad ro quiet splash vt.handoff=7
> 
>   <accessing unreachable portions of memory>
> saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=ram,2048Mi,1ki
> saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
> [  634.748495] phram: ram device: 0x400 at 0x80000000
> 
>   <parameter too long>
> saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz,256Mi,1Mi
> insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
> saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
> [ 1469.763787] phram: parameter too long
> [ 1469.763797] phram: `abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz,256Mi,1Mi' invalid for parameter `phram'
> 
> 
>   <too many arguments>
> saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256Mi,1Mi,extra_parameter
> insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
> saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
> [ 1650.081694] phram: too many arguments
> [ 1650.081703] phram: `swap,256Mi,1Mi,extra_parameter' invalid for parameter `phram'
> 
>   <not enough arguments>
> saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256Mi
> insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
> saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
> [ 1707.437130] phram: not enough arguments
> [ 1707.437138] phram: `swap,256Mi' invalid for parameter `phram'
> 
> 
>   <invalid start address>
> saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256xyz,1Mi
> insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
> saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
> [ 1783.014351] phram: invalid start address
> [ 1783.014359] phram: `swap,256xyz,1Mi' invalid for parameter `phram'
> 
>   <invalid device length>
> saurabh@saurabh:~/little/Task02/linux$ sudo insmod drivers/mtd/devices/phram.ko phram=swap,256Mi,1xyz
> insmod: ERROR: could not insert module drivers/mtd/devices/phram.ko: Invalid parameters
> saurabh@saurabh:~/little/Task02/linux$ sudo dmesg -c
> [ 1831.746108] phram: invalid device length
> [ 1831.746117] phram: `swap,256Mi,1xyz' invalid for parameter `phram'
> -------------------------------------------

This all looks very good. For the successful cases, it might be nice to
make sure you can still read and write the "device". (Try dd on
/dev/mtd0, or see mtd-utils for tools like flash_erase.) But you're not
touching the actual I/O behavior, so this isn't so important.

More importantly, it's good to test these cases too:

 * phram is built-in (not a module), with and without a phram= line on
   the commandline
 * writing to /sys/module/phram/parameters/phram (for both the module
   and built-in cases) 

Thanks,
Brian

[PATCH] mtd: phram: error handling

[Saurabh Sengar]

> More importantly, it's good to test these cases too:

> * phram is built-in (not a module), with and without a phram= line on
>   the commandline
> * writing to /sys/module/phram/parameters/phram (for both the module
>   and built-in cases)

Hi Brian,

1) I have tried phram as built-in, with and without phram= line in cmdline
but both the time there was no phram directory found in /sys/modules, neither /dev/mtd0
(do I need to enablesome config options ?)

2) There was no 'parameters' directory inside /sys/module/phram when I used phram as module,
though /dev/mtd0 and /dev/mtd0ro were present
 

I tried searching phram in kernel/Documentation but couldn't found anything.
I have few queries related to phram driver, please answer if your time permits.
(Feel free to ignore if I am taking too much your time, I know these are too many :) )

Q1) Phram driver is used for accessing memory which are there but not currently mapped in system? am I correct?

Q2) When I register device with junk names like phram=saurabh,0x1f7000000,0x400, it registers fine with name 'saurabh', isn't it wrong ?

Q3) If I access some memory which does not even exist, driver still registers and even read operation is successfull to it.
eg: phram=ram,8Gi,1ki (My laptop have 4GB ram but accessing 8GB address of ram)


Regards,
Saurabh

-------------------------------------------