diff mbox

[2/3] mtd: move zero length verification to MTD API functions

Message ID 1328707598.22240.64.camel@sauron.fi.intel.com
State New, archived
Headers show

Commit Message

Artem Bityutskiy Feb. 8, 2012, 1:26 p.m. UTC 
On Wed, 2012-02-08 at 14:42 +0200, Artem Bityutskiy wrote:
> On Tue, 2012-02-07 at 14:35 +0200, Shmulik Ladkani wrote:
> > On Mon,  6 Feb 2012 14:03:08 +0200 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > > @@ -712,6 +717,8 @@ int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
> > >  		return -EOPNOTSUPP;
> > >  	if (from < 0 || from > mtd->size || len > mtd->size - from)
> > >  		return -EINVAL;
> > > +	if (!len)
> > > +		return 0;
> > >  	return mtd->_point(mtd, from, len, retlen, virt, phys);
> > >  }
> > 
> > Previously, '_point' implementors usually assigned *virt (e.g. for NOR -
> > to the relevant ioremapped address), regardless 'len' value.
> > Meaning, *virt was set, even in the 'len == 0' case.
> > New 'mtd_point()' does not set *virt in this case.
> > 
> > (Luckily, seems there are no calls to 'mtd_point' with zero 'len'...)
> > 
> > I guess it is safe to assume the *virt assignment in the '!len' case was
> > wrong?
> 
> If length is zero then 'virt' is anyway useless. But we should set it to
> NULL in 'mtd_point()' to make sure that if someone accesses it he gets
> an oops. I'll amend the patch and update the commit message
> correspondingly, thanks a lot for review.


I guess we can go a bit further and sanitize 'mtd_point()' like this:

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Wed, 8 Feb 2012 15:13:26 +0200
Subject: [PATCH] mtd: harmonize mtd_point interface implementation

Some MTD drivers return -EINVAL if the 'phys' parameter is not NULL, trying to
convey that they cannot return the physical address. However, this is not very
logical because they still can return the virtual address ('virt'). But some
drivers (lpddr) just ignore the 'phys' parameter instead, which is a more
logical thing to do.

Let's harmonize this and:

1. Always initialize 'virt' and 'phys' to 'NULL' in 'mtd_point()'.
2. Do not return an error if the physical address cannot be found.

So as a result, all drivers will set 'phys' to 'NULL' if it is not supported.
None of the 'mtd_point()' users use 'phys' anyway, so this should not break
anything. I guess we could also just delete this parameter later.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/devices/mtdram.c |    3 ---
 drivers/mtd/devices/phram.c  |    4 ----
 drivers/mtd/devices/pmc551.c |    4 ----
 drivers/mtd/devices/slram.c  |    3 ---
 drivers/mtd/mtdcore.c        |    3 +++
 5 files changed, 3 insertions(+), 14 deletions(-)

Comments

Shmulik Ladkani Feb. 8, 2012, 2:54 p.m. UTC | #1 
On Wed, 08 Feb 2012 15:26:38 +0200 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> I guess we can go a bit further and sanitize 'mtd_point()' like this:
> 
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Date: Wed, 8 Feb 2012 15:13:26 +0200
> Subject: [PATCH] mtd: harmonize mtd_point interface implementation
> 
> Some MTD drivers return -EINVAL if the 'phys' parameter is not NULL, trying to
> convey that they cannot return the physical address. However, this is not very
> logical because they still can return the virtual address ('virt'). But some
> drivers (lpddr) just ignore the 'phys' parameter instead, which is a more
> logical thing to do.
> 
> Let's harmonize this and:
> 
> 1. Always initialize 'virt' and 'phys' to 'NULL' in 'mtd_point()'.
> 2. Do not return an error if the physical address cannot be found.
> 
> So as a result, all drivers will set 'phys' to 'NULL' if it is not supported.
> None of the 'mtd_point()' users use 'phys' anyway, so this should not break
> anything. I guess we could also just delete this parameter later.

According to a98889f3, it makes some sense to return an error when users
explicitly asked for the 'phys' but driver is unable to return it.

I guess new interface (checking the returned 'phys') is reasonable as
well. And, nothing gets broken since no such users...

Reviewed-by: shmulik.ladkani@gmail.com
Artem Bityutskiy Feb. 8, 2012, 4:06 p.m. UTC | #2 
On Wed, 2012-02-08 at 16:54 +0200, Shmulik Ladkani wrote:
> I guess new interface (checking the returned 'phys') is reasonable as
> well. And, nothing gets broken since no such users...
> 
> Reviewed-by: shmulik.ladkani@gmail.com

Thanks, but I added more preferable
Reviewed-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
diff mbox

Patch

diff --git a/drivers/mtd/devices/mtdram.c b/drivers/mtd/devices/mtdram.c
index 0e0e6ed..ec59d65 100644
--- a/drivers/mtd/devices/mtdram.c
+++ b/drivers/mtd/devices/mtdram.c
@@ -43,9 +43,6 @@  static int ram_erase(struct mtd_info *mtd, struct erase_info *instr)
 static int ram_point(struct mtd_info *mtd, loff_t from, size_t len,
 		size_t *retlen, void **virt, resource_size_t *phys)
 {
-	/* can we return a physical address with this driver? */
-	if (phys)
-		return -EINVAL;
 	*virt = mtd->priv + from;
 	*retlen = len;
 	return 0;
diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index d3474a4..9d2bf17 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -51,10 +51,6 @@  static int phram_erase(struct mtd_info *mtd, struct erase_info *instr)
 static int phram_point(struct mtd_info *mtd, loff_t from, size_t len,
 		size_t *retlen, void **virt, resource_size_t *phys)
 {
-	/* can we return a physical address with this driver? */
-	if (phys)
-		return -EINVAL;
-
 	*virt = mtd->priv + from;
 	*retlen = len;
 	return 0;
diff --git a/drivers/mtd/devices/pmc551.c b/drivers/mtd/devices/pmc551.c
index 6269a43..c4368ec 100644
--- a/drivers/mtd/devices/pmc551.c
+++ b/drivers/mtd/devices/pmc551.c
@@ -205,10 +205,6 @@  static int pmc551_point(struct mtd_info *mtd, loff_t from, size_t len,
 	printk(KERN_DEBUG "pmc551_point(%ld, %ld)\n", (long)from, (long)len);
 #endif
 
-	/* can we return a physical address with this driver? */
-	if (phys)
-		return -EINVAL;
-
 	soff_hi = from & ~(priv->asize - 1);
 	soff_lo = from & (priv->asize - 1);
 
diff --git a/drivers/mtd/devices/slram.c b/drivers/mtd/devices/slram.c
index 842e489..ccd39ff 100644
--- a/drivers/mtd/devices/slram.c
+++ b/drivers/mtd/devices/slram.c
@@ -99,9 +99,6 @@  static int slram_point(struct mtd_info *mtd, loff_t from, size_t len,
 {
 	slram_priv_t *priv = mtd->priv;
 
-	/* can we return a physical address with this driver? */
-	if (phys)
-		return -EINVAL;
 	*virt = priv->start + from;
 	*retlen = len;
 	return(0);
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index ead52b3..1680ef5 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -706,6 +706,9 @@  int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 	      void **virt, resource_size_t *phys)
 {
 	*retlen = 0;
+	*virt = NULL;
+	if (phys)
+		*phys = NULL;
 	if (!mtd->_point)
 		return -EOPNOTSUPP;
 	if (from < 0 || from > mtd->size || len > mtd->size - from)