[PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Aaron Wu]

In this bug, the address operation range may exceed the
window size defined by gpio expanded flash MTD partition.

Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
---
 drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index a4c477b..ca9282f 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssi
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, from);
+	int this_len;
 
 	/* BUG if operation crosses the win_size */
 	BUG_ON(!((from + len) % state->win_size <= (from + len)));
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_fromio(to, map->virt + (from % state->win_size), len);
+	while (len) {
+		if ((from % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (from % state->win_size);
+		else
+			this_len = len;
+
+		gf_set_gpios(state, from);
+		memcpy_fromio(to, map->virt + (from % state->win_size)
+			, this_len);
+		len -= this_len;
+		from += this_len;
+		to += this_len;
+	}
 }
 
 /**
@@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, to);
+	int this_len;
 
 	/* BUG if operation crosses the win_size */
 	BUG_ON(!((to + len) % state->win_size <= (to + len)));
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_toio(map->virt + (to % state->win_size), from, len);
+	while (len) {
+		if ((to % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (to % state->win_size);
+		else
+			this_len = len;
+
+		gf_set_gpios(state, to);
+		memcpy_toio(map->virt + (to % state->win_size), from, len);
+
+		len -= this_len;
+		to += this_len;
+		from += this_len;
+	}
 }
 
 static const char * const part_probe_types[] = {
-- 
1.7.9.5

RE: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Wu, Aaron]

[-- Attachment #1: Type: text/plain, Size: 6806 bytes --]



>-----Original Message-----
>From: Brian Norris [mailto:computersforpeace@gmail.com]
>Sent: Tuesday, August 05, 2014 4:29 AM
>To: Wu, Aaron
>Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic; Mike
>Frysinger
>Subject: Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash
>
>Hi Aaron,
>
>Please don't top post (like I'm doing right now). It makes it harder to follow
>what's actually being said.
>
>And please *do* selectively quote (unlike me right now), so it's easy to see
>what you've responded to without too much other noise.
>
>Mike hasn't been involved in MTD recently, so I don't fully expect an answer.
>
>The BUG_ON() is certainly useless, so we should drop it.
>
>It also seems like your patch is contradicting the other existing comments, so
>your patch should include more detailed explanation, and it should modify the
>comments appropriately. Particularly, you should note why this is no longer
>the case:
>
>  "We rely on the MTD layer to chunk up copies such that a single
>  request here will not cross a window size. [...]"
>
>So yes, you need a new patch.

Hi Brian, I git send-email a newer version however I myself did not receive it from the mtd mail list so attach it here. Thanks. 

>Thanks,
>Brian
>
>On Fri, Aug 01, 2014 at 08:34:03AM +0000, Wu, Aaron wrote:
>> Hi Brian and Mike,
>>
>> AFAICT I agree with you, I think the BUG_ON is not likely to be executed, I'm
>OK with removing it, Mike, what's your idea.
>>
>> And do you want me to make a new patch or make another patch to
>remove the BUG_ON and the "," issue if we can reach agreement on this
>issue?
>>
>> Best regards,
>> Aaron
>>
>> -----Original Message-----
>> From: Wu, Aaron
>> Sent: Friday, August 01, 2014 3:00 PM
>> To: 'Brian Norris'
>> Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic;
>> Mike Frysinger
>> Subject: RE: [PATCH] gpio_flash: fix the CPLB miss bug for gpio
>> expanded flash
>>
>> Hi Brian,
>>
>> Thanks for your time and comments.
>>
>> This patch originally intend to solve the issue that Flash operation address
>passed in from upper layer may exceed the maxim address boundary allowed
>by the window size defined by the gpio style of flash.
>>
>> Take the read, memcpy_fromio for example, this address is described by
>(map->virt + (from % state->win_size)  +  this_len), where this_len varies in
>different situation.  If the address do exceed the boundary then we repeat
>read to work around it.
>>
>> I actually did not pay enough attention on the BUG_ON condition from Mike,
>I will check later and be back.
>>
>> At last this patch fixed a customer reported issue so I suppose this "cross the
>boundary" bug do exits and this patch roughly fixed it.
>>
>> Best regards,
>> Aaron
>>
>> -----Original Message-----
>> From: Brian Norris [mailto:computersforpeace@gmail.com]
>> Sent: Friday, August 01, 2014 2:35 PM
>> To: Wu, Aaron
>> Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic;
>> Mike Frysinger
>> Subject: Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio
>> expanded flash
>>
>> + Mike Frysinger
>>
>> Hi Aaron,
>>
>> I'm trying to judge the quality of your patch, but I'm very distracted from this
>by trying to figure out the original intent of this code.
>> Perhaps Mike can comment.
>>
>> On Thu, Jul 03, 2014 at 04:13:34PM +0800, Aaron Wu wrote:
>> > In this bug, the address operation range may exceed the window size
>> > defined by gpio expanded flash MTD partition.
>> >
>> > Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
>> > ---
>> >  drivers/mtd/maps/gpio-addr-flash.c |   34
>++++++++++++++++++++++++++++------
>> >  1 file changed, 28 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/mtd/maps/gpio-addr-flash.c
>> > b/drivers/mtd/maps/gpio-addr-flash.c
>> > index a4c477b..ca9282f 100644
>> > --- a/drivers/mtd/maps/gpio-addr-flash.c
>> > +++ b/drivers/mtd/maps/gpio-addr-flash.c
>> > @@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map,
>> > void *to, unsigned long from, ssi  {
>> >  	struct async_state *state = gf_map_info_to_state(map);
>> >
>> > -	gf_set_gpios(state, from);
>> > +	int this_len;
>> >
>> >  	/* BUG if operation crosses the win_size */
>> >  	BUG_ON(!((from + len) % state->win_size <= (from + len)));
>>
>> What do you think of this BUG_ON()? AFAICT, the BUG_ON() predicate is
>> provably false [1], at least whenever
>>
>> 	from >= 0 && len >= 0 && state->win_size > 0.
>>
>> Was the intent more like this?
>>
>> 	BUG_ON((from + len - 1) / state->win_size != from / state->win_size);
>>
>> If so, then it seems like maybe this patch is trying to address the case that
>was previously considered a BUG()?
>>
>> In any case, if we can't figure out what the BUG_ON() really should have
>been, then maybe we should kill it and take something like your patch to fix
>the case you're looking at, Aaron.
>>
>> >
>> > -	/* operation does not cross the win_size, so one shot it */
>> > -	memcpy_fromio(to, map->virt + (from % state->win_size), len);
>> > +	while (len) {
>> > +		if ((from % state->win_size) + len > state->win_size)
>> > +			this_len = state->win_size - (from % state->win_size);
>> > +		else
>> > +			this_len = len;
>> > +
>> > +		gf_set_gpios(state, from);
>> > +		memcpy_fromio(to, map->virt + (from % state->win_size)
>> > +			, this_len);
>> > +		len -= this_len;
>> > +		from += this_len;
>> > +		to += this_len;
>> > +	}
>> >  }
>> >
>> >  /**
>> > @@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map,
>> > unsigned long to,  {
>> >  	struct async_state *state = gf_map_info_to_state(map);
>> >
>> > -	gf_set_gpios(state, to);
>> > +	int this_len;
>> >
>> >  	/* BUG if operation crosses the win_size */
>> >  	BUG_ON(!((to + len) % state->win_size <= (to + len)));
>>
>> Same here. I think the BUG_ON() condition is a no-op (always false).
>>
>> >
>> > -	/* operation does not cross the win_size, so one shot it */
>> > -	memcpy_toio(map->virt + (to % state->win_size), from, len);
>> > +	while (len) {
>> > +		if ((to % state->win_size) + len > state->win_size)
>> > +			this_len = state->win_size - (to % state->win_size);
>> > +		else
>> > +			this_len = len;
>> > +
>> > +		gf_set_gpios(state, to);
>> > +		memcpy_toio(map->virt + (to % state->win_size), from, len);
>> > +
>> > +		len -= this_len;
>> > +		to += this_len;
>> > +		from += this_len;
>> > +	}
>> >  }
>> >
>> >  static const char * const part_probe_types[] = {
>>
>> Brian
>>
>> [1] If it was provably false, then I should be able to write out a proof! It relies
>on something like this:
>>
>> 	If x >= 0 and y > 0, then x % y <= x.

[-- Attachment #2: 0001-mtd-gpio_flash-fix-the-bug-in-handling-gpio-flash-re.patch --]
[-- Type: application/octet-stream, Size: 3033 bytes --]

From e566b3919e52e246b980758383e1109fef788673 Mon Sep 17 00:00:00 2001
From: Aaron Wu <Aaron.wu@analog.com>
Date: Tue, 5 Aug 2014 13:26:04 +0800
Subject: [PATCH] [mtd] gpio_flash: fix the bug in handling gpio flash
 read/write when offset + len from mtd exceeds the window
 size

Signed-off-by: Aaron Wu <Aaron.wu@analog.com>

fix the bug in handling gpio flash read/write when offset + len
from mtd exceeds the window size
---
 drivers/mtd/maps/gpio-addr-flash.c |   43 ++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index a4c477b..947631a 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -99,22 +99,29 @@ static map_word gf_read(struct map_info *map, unsigned long ofs)
  *	@from: flash offset to copy from
  *	@len:  how much to copy
  *
- * We rely on the MTD layer to chunk up copies such that a single request here
- * will not cross a window size.  This allows us to only wiggle the GPIOs once
- * before falling back to a normal memcpy.  Reading the higher layer code shows
- * that this is indeed the case, but add a BUG_ON() to future proof.
+ *Toggle the correct GPIO according to @from to enable the right flash bank,
+ *still the read offset plus len may execceds the actual Window size,when
+ *this happnes, reverts the value for multiple read until all data is read.
  */
 static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssize_t len)
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, from);
+	int this_len;
 
-	/* BUG if operation crosses the win_size */
-	BUG_ON(!((from + len) % state->win_size <= (from + len)));
+	while (len) {
+		if ((from % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (from % state->win_size);
+		else
+			this_len = len;
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_fromio(to, map->virt + (from % state->win_size), len);
+		gf_set_gpios(state, from);
+		memcpy_fromio(to, map->virt + (from % state->win_size),
+			 this_len);
+		len -= this_len;
+		from += this_len;
+		to += this_len;
+	}
 }
 
 /**
@@ -147,13 +154,21 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, to);
+	int this_len;
+
+	while (len) {
+		if ((to % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (to % state->win_size);
+		else
+			this_len = len;
 
-	/* BUG if operation crosses the win_size */
-	BUG_ON(!((to + len) % state->win_size <= (to + len)));
+		gf_set_gpios(state, to);
+		memcpy_toio(map->virt + (to % state->win_size), from, len);
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_toio(map->virt + (to % state->win_size), from, len);
+		len -= this_len;
+		to += this_len;
+		from += this_len;
+	}
 }
 
 static const char * const part_probe_types[] = {
-- 
1.7.9.5

Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Brian Norris]

Hi Aaron,

Please don't top post (like I'm doing right now). It makes it harder to
follow what's actually being said.

And please *do* selectively quote (unlike me right now), so it's easy to
see what you've responded to without too much other noise.

Mike hasn't been involved in MTD recently, so I don't fully expect an
answer.

The BUG_ON() is certainly useless, so we should drop it.

It also seems like your patch is contradicting the other existing
comments, so your patch should include more detailed explanation, and it
should modify the comments appropriately. Particularly, you should note
why this is no longer the case:

  "We rely on the MTD layer to chunk up copies such that a single
  request here will not cross a window size. [...]"

So yes, you need a new patch.

Thanks,
Brian

On Fri, Aug 01, 2014 at 08:34:03AM +0000, Wu, Aaron wrote:
> Hi Brian and Mike,
> 
> AFAICT I agree with you, I think the BUG_ON is not likely to be executed, I'm OK with removing it, Mike, what's your idea.
> 
> And do you want me to make a new patch or make another patch to remove the BUG_ON and the "," issue if we can reach agreement on this issue?
> 
> Best regards,
> Aaron 
> 
> -----Original Message-----
> From: Wu, Aaron 
> Sent: Friday, August 01, 2014 3:00 PM
> To: 'Brian Norris'
> Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic; Mike Frysinger
> Subject: RE: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash
> 
> Hi Brian,
> 
> Thanks for your time and comments.
> 
> This patch originally intend to solve the issue that Flash operation address passed in from upper layer may exceed the maxim address boundary allowed by the window size defined by the gpio style of flash.
> 
> Take the read, memcpy_fromio for example, this address is described by (map->virt + (from % state->win_size)  +  this_len), where this_len varies in different situation.  If the address do exceed the boundary then we repeat read to work around it.
> 
> I actually did not pay enough attention on the BUG_ON condition from Mike, I will check later and be back.
> 
> At last this patch fixed a customer reported issue so I suppose this "cross the boundary" bug do exits and this patch roughly fixed it.
> 
> Best regards,
> Aaron   
> 
> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: Friday, August 01, 2014 2:35 PM
> To: Wu, Aaron
> Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic; Mike Frysinger
> Subject: Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash
> 
> + Mike Frysinger
> 
> Hi Aaron,
> 
> I'm trying to judge the quality of your patch, but I'm very distracted from this by trying to figure out the original intent of this code.
> Perhaps Mike can comment.
> 
> On Thu, Jul 03, 2014 at 04:13:34PM +0800, Aaron Wu wrote:
> > In this bug, the address operation range may exceed the window size 
> > defined by gpio expanded flash MTD partition.
> > 
> > Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
> > ---
> >  drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
> >  1 file changed, 28 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mtd/maps/gpio-addr-flash.c
> > b/drivers/mtd/maps/gpio-addr-flash.c
> > index a4c477b..ca9282f 100644
> > --- a/drivers/mtd/maps/gpio-addr-flash.c
> > +++ b/drivers/mtd/maps/gpio-addr-flash.c
> > @@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, 
> > void *to, unsigned long from, ssi  {
> >  	struct async_state *state = gf_map_info_to_state(map);
> >  
> > -	gf_set_gpios(state, from);
> > +	int this_len;
> >  
> >  	/* BUG if operation crosses the win_size */
> >  	BUG_ON(!((from + len) % state->win_size <= (from + len)));
> 
> What do you think of this BUG_ON()? AFAICT, the BUG_ON() predicate is provably false [1], at least whenever
> 
> 	from >= 0 && len >= 0 && state->win_size > 0.
> 
> Was the intent more like this?
> 
> 	BUG_ON((from + len - 1) / state->win_size != from / state->win_size);
> 
> If so, then it seems like maybe this patch is trying to address the case that was previously considered a BUG()?
> 
> In any case, if we can't figure out what the BUG_ON() really should have been, then maybe we should kill it and take something like your patch to fix the case you're looking at, Aaron.
> 
> >  
> > -	/* operation does not cross the win_size, so one shot it */
> > -	memcpy_fromio(to, map->virt + (from % state->win_size), len);
> > +	while (len) {
> > +		if ((from % state->win_size) + len > state->win_size)
> > +			this_len = state->win_size - (from % state->win_size);
> > +		else
> > +			this_len = len;
> > +
> > +		gf_set_gpios(state, from);
> > +		memcpy_fromio(to, map->virt + (from % state->win_size)
> > +			, this_len);
> > +		len -= this_len;
> > +		from += this_len;
> > +		to += this_len;
> > +	}
> >  }
> >  
> >  /**
> > @@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, 
> > unsigned long to,  {
> >  	struct async_state *state = gf_map_info_to_state(map);
> >  
> > -	gf_set_gpios(state, to);
> > +	int this_len;
> >  
> >  	/* BUG if operation crosses the win_size */
> >  	BUG_ON(!((to + len) % state->win_size <= (to + len)));
> 
> Same here. I think the BUG_ON() condition is a no-op (always false).
> 
> >  
> > -	/* operation does not cross the win_size, so one shot it */
> > -	memcpy_toio(map->virt + (to % state->win_size), from, len);
> > +	while (len) {
> > +		if ((to % state->win_size) + len > state->win_size)
> > +			this_len = state->win_size - (to % state->win_size);
> > +		else
> > +			this_len = len;
> > +
> > +		gf_set_gpios(state, to);
> > +		memcpy_toio(map->virt + (to % state->win_size), from, len);
> > +
> > +		len -= this_len;
> > +		to += this_len;
> > +		from += this_len;
> > +	}
> >  }
> >  
> >  static const char * const part_probe_types[] = {
> 
> Brian
> 
> [1] If it was provably false, then I should be able to write out a proof! It relies on something like this:
> 
> 	If x >= 0 and y > 0, then x % y <= x.

RE: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Wu, Aaron]

Hi Brian and Mike,

AFAICT I agree with you, I think the BUG_ON is not likely to be executed, I'm OK with removing it, Mike, what's your idea.

And do you want me to make a new patch or make another patch to remove the BUG_ON and the "," issue if we can reach agreement on this issue?

Best regards,
Aaron 

-----Original Message-----
From: Wu, Aaron 
Sent: Friday, August 01, 2014 3:00 PM
To: 'Brian Norris'
Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic; Mike Frysinger
Subject: RE: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

Hi Brian,

Thanks for your time and comments.

This patch originally intend to solve the issue that Flash operation address passed in from upper layer may exceed the maxim address boundary allowed by the window size defined by the gpio style of flash.

Take the read, memcpy_fromio for example, this address is described by (map->virt + (from % state->win_size)  +  this_len), where this_len varies in different situation.  If the address do exceed the boundary then we repeat read to work around it.

I actually did not pay enough attention on the BUG_ON condition from Mike, I will check later and be back.

At last this patch fixed a customer reported issue so I suppose this "cross the boundary" bug do exits and this patch roughly fixed it.

Best regards,
Aaron   

-----Original Message-----
From: Brian Norris [mailto:computersforpeace@gmail.com]
Sent: Friday, August 01, 2014 2:35 PM
To: Wu, Aaron
Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic; Mike Frysinger
Subject: Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

+ Mike Frysinger

Hi Aaron,

I'm trying to judge the quality of your patch, but I'm very distracted from this by trying to figure out the original intent of this code.
Perhaps Mike can comment.

On Thu, Jul 03, 2014 at 04:13:34PM +0800, Aaron Wu wrote:
> In this bug, the address operation range may exceed the window size 
> defined by gpio expanded flash MTD partition.
> 
> Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
> ---
>  drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/maps/gpio-addr-flash.c
> b/drivers/mtd/maps/gpio-addr-flash.c
> index a4c477b..ca9282f 100644
> --- a/drivers/mtd/maps/gpio-addr-flash.c
> +++ b/drivers/mtd/maps/gpio-addr-flash.c
> @@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, 
> void *to, unsigned long from, ssi  {
>  	struct async_state *state = gf_map_info_to_state(map);
>  
> -	gf_set_gpios(state, from);
> +	int this_len;
>  
>  	/* BUG if operation crosses the win_size */
>  	BUG_ON(!((from + len) % state->win_size <= (from + len)));

What do you think of this BUG_ON()? AFAICT, the BUG_ON() predicate is provably false [1], at least whenever

	from >= 0 && len >= 0 && state->win_size > 0.

Was the intent more like this?

	BUG_ON((from + len - 1) / state->win_size != from / state->win_size);

If so, then it seems like maybe this patch is trying to address the case that was previously considered a BUG()?

In any case, if we can't figure out what the BUG_ON() really should have been, then maybe we should kill it and take something like your patch to fix the case you're looking at, Aaron.

>  
> -	/* operation does not cross the win_size, so one shot it */
> -	memcpy_fromio(to, map->virt + (from % state->win_size), len);
> +	while (len) {
> +		if ((from % state->win_size) + len > state->win_size)
> +			this_len = state->win_size - (from % state->win_size);
> +		else
> +			this_len = len;
> +
> +		gf_set_gpios(state, from);
> +		memcpy_fromio(to, map->virt + (from % state->win_size)
> +			, this_len);
> +		len -= this_len;
> +		from += this_len;
> +		to += this_len;
> +	}
>  }
>  
>  /**
> @@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, 
> unsigned long to,  {
>  	struct async_state *state = gf_map_info_to_state(map);
>  
> -	gf_set_gpios(state, to);
> +	int this_len;
>  
>  	/* BUG if operation crosses the win_size */
>  	BUG_ON(!((to + len) % state->win_size <= (to + len)));

Same here. I think the BUG_ON() condition is a no-op (always false).

>  
> -	/* operation does not cross the win_size, so one shot it */
> -	memcpy_toio(map->virt + (to % state->win_size), from, len);
> +	while (len) {
> +		if ((to % state->win_size) + len > state->win_size)
> +			this_len = state->win_size - (to % state->win_size);
> +		else
> +			this_len = len;
> +
> +		gf_set_gpios(state, to);
> +		memcpy_toio(map->virt + (to % state->win_size), from, len);
> +
> +		len -= this_len;
> +		to += this_len;
> +		from += this_len;
> +	}
>  }
>  
>  static const char * const part_probe_types[] = {

Brian

[1] If it was provably false, then I should be able to write out a proof! It relies on something like this:

	If x >= 0 and y > 0, then x % y <= x.

RE: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Wu, Aaron]

Hi Brian,

Thanks for your time and comments.

This patch originally intend to solve the issue that Flash operation address passed in from upper layer may exceed the maxim address boundary allowed by the window size defined by the gpio style of flash.

Take the read, memcpy_fromio for example, this address is described by (map->virt + (from % state->win_size)  +  this_len), where this_len varies in different situation.  If the address do exceed the boundary then we repeat read to work around it.

I actually did not pay enough attention on the BUG_ON condition from Mike, I will check later and be back.

At last this patch fixed a customer reported issue so I suppose this "cross the boundary" bug do exits and this patch roughly fixed it.

Best regards,
Aaron   

-----Original Message-----
From: Brian Norris [mailto:computersforpeace@gmail.com] 
Sent: Friday, August 01, 2014 2:35 PM
To: Wu, Aaron
Cc: linux-mtd@lists.infradead.org; dwmw2@infradead.org; Zhang, Sonic; Mike Frysinger
Subject: Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

+ Mike Frysinger

Hi Aaron,

I'm trying to judge the quality of your patch, but I'm very distracted from this by trying to figure out the original intent of this code.
Perhaps Mike can comment.

On Thu, Jul 03, 2014 at 04:13:34PM +0800, Aaron Wu wrote:
> In this bug, the address operation range may exceed the window size 
> defined by gpio expanded flash MTD partition.
> 
> Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
> ---
>  drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/maps/gpio-addr-flash.c 
> b/drivers/mtd/maps/gpio-addr-flash.c
> index a4c477b..ca9282f 100644
> --- a/drivers/mtd/maps/gpio-addr-flash.c
> +++ b/drivers/mtd/maps/gpio-addr-flash.c
> @@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, 
> void *to, unsigned long from, ssi  {
>  	struct async_state *state = gf_map_info_to_state(map);
>  
> -	gf_set_gpios(state, from);
> +	int this_len;
>  
>  	/* BUG if operation crosses the win_size */
>  	BUG_ON(!((from + len) % state->win_size <= (from + len)));

What do you think of this BUG_ON()? AFAICT, the BUG_ON() predicate is provably false [1], at least whenever

	from >= 0 && len >= 0 && state->win_size > 0.

Was the intent more like this?

	BUG_ON((from + len - 1) / state->win_size != from / state->win_size);

If so, then it seems like maybe this patch is trying to address the case that was previously considered a BUG()?

In any case, if we can't figure out what the BUG_ON() really should have been, then maybe we should kill it and take something like your patch to fix the case you're looking at, Aaron.

>  
> -	/* operation does not cross the win_size, so one shot it */
> -	memcpy_fromio(to, map->virt + (from % state->win_size), len);
> +	while (len) {
> +		if ((from % state->win_size) + len > state->win_size)
> +			this_len = state->win_size - (from % state->win_size);
> +		else
> +			this_len = len;
> +
> +		gf_set_gpios(state, from);
> +		memcpy_fromio(to, map->virt + (from % state->win_size)
> +			, this_len);
> +		len -= this_len;
> +		from += this_len;
> +		to += this_len;
> +	}
>  }
>  
>  /**
> @@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, 
> unsigned long to,  {
>  	struct async_state *state = gf_map_info_to_state(map);
>  
> -	gf_set_gpios(state, to);
> +	int this_len;
>  
>  	/* BUG if operation crosses the win_size */
>  	BUG_ON(!((to + len) % state->win_size <= (to + len)));

Same here. I think the BUG_ON() condition is a no-op (always false).

>  
> -	/* operation does not cross the win_size, so one shot it */
> -	memcpy_toio(map->virt + (to % state->win_size), from, len);
> +	while (len) {
> +		if ((to % state->win_size) + len > state->win_size)
> +			this_len = state->win_size - (to % state->win_size);
> +		else
> +			this_len = len;
> +
> +		gf_set_gpios(state, to);
> +		memcpy_toio(map->virt + (to % state->win_size), from, len);
> +
> +		len -= this_len;
> +		to += this_len;
> +		from += this_len;
> +	}
>  }
>  
>  static const char * const part_probe_types[] = {

Brian

[1] If it was provably false, then I should be able to write out a proof! It relies on something like this:

	If x >= 0 and y > 0, then x % y <= x.

Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Brian Norris]

On Thu, Jul 03, 2014 at 04:13:34PM +0800, Aaron Wu wrote:
> @@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssi
>  {
[...]
> +		memcpy_fromio(to, map->virt + (from % state->win_size)
> +			, this_len);

And one nitpick: please put that stray comma on the previous line.

[...]

Thanks,
Brian

Re: [PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Brian Norris]

+ Mike Frysinger

Hi Aaron,

I'm trying to judge the quality of your patch, but I'm very distracted
from this by trying to figure out the original intent of this code.
Perhaps Mike can comment.

On Thu, Jul 03, 2014 at 04:13:34PM +0800, Aaron Wu wrote:
> In this bug, the address operation range may exceed the
> window size defined by gpio expanded flash MTD partition.
> 
> Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
> ---
>  drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> index a4c477b..ca9282f 100644
> --- a/drivers/mtd/maps/gpio-addr-flash.c
> +++ b/drivers/mtd/maps/gpio-addr-flash.c
> @@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssi
>  {
>  	struct async_state *state = gf_map_info_to_state(map);
>  
> -	gf_set_gpios(state, from);
> +	int this_len;
>  
>  	/* BUG if operation crosses the win_size */
>  	BUG_ON(!((from + len) % state->win_size <= (from + len)));

What do you think of this BUG_ON()? AFAICT, the BUG_ON() predicate is
provably false [1], at least whenever

	from >= 0 && len >= 0 && state->win_size > 0.

Was the intent more like this?

	BUG_ON((from + len - 1) / state->win_size != from / state->win_size);

If so, then it seems like maybe this patch is trying to address the case
that was previously considered a BUG()?

In any case, if we can't figure out what the BUG_ON() really should have
been, then maybe we should kill it and take something like your patch to
fix the case you're looking at, Aaron.

>  
> -	/* operation does not cross the win_size, so one shot it */
> -	memcpy_fromio(to, map->virt + (from % state->win_size), len);
> +	while (len) {
> +		if ((from % state->win_size) + len > state->win_size)
> +			this_len = state->win_size - (from % state->win_size);
> +		else
> +			this_len = len;
> +
> +		gf_set_gpios(state, from);
> +		memcpy_fromio(to, map->virt + (from % state->win_size)
> +			, this_len);
> +		len -= this_len;
> +		from += this_len;
> +		to += this_len;
> +	}
>  }
>  
>  /**
> @@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
>  {
>  	struct async_state *state = gf_map_info_to_state(map);
>  
> -	gf_set_gpios(state, to);
> +	int this_len;
>  
>  	/* BUG if operation crosses the win_size */
>  	BUG_ON(!((to + len) % state->win_size <= (to + len)));

Same here. I think the BUG_ON() condition is a no-op (always false).

>  
> -	/* operation does not cross the win_size, so one shot it */
> -	memcpy_toio(map->virt + (to % state->win_size), from, len);
> +	while (len) {
> +		if ((to % state->win_size) + len > state->win_size)
> +			this_len = state->win_size - (to % state->win_size);
> +		else
> +			this_len = len;
> +
> +		gf_set_gpios(state, to);
> +		memcpy_toio(map->virt + (to % state->win_size), from, len);
> +
> +		len -= this_len;
> +		to += this_len;
> +		from += this_len;
> +	}
>  }
>  
>  static const char * const part_probe_types[] = {

Brian

[1] If it was provably false, then I should be able to write out a
proof! It relies on something like this:

	If x >= 0 and y > 0, then x % y <= x.

[PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Aaron Wu]

In this bug, the address operation range may exceed the
window size defined by gpio expanded flash MTD partition.

Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
---
 drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index a4c477b..ca9282f 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssi
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, from);
+	int this_len;
 
 	/* BUG if operation crosses the win_size */
 	BUG_ON(!((from + len) % state->win_size <= (from + len)));
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_fromio(to, map->virt + (from % state->win_size), len);
+	while (len) {
+		if ((from % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (from % state->win_size);
+		else
+			this_len = len;
+
+		gf_set_gpios(state, from);
+		memcpy_fromio(to, map->virt + (from % state->win_size)
+			, this_len);
+		len -= this_len;
+		from += this_len;
+		to += this_len;
+	}
 }
 
 /**
@@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, to);
+	int this_len;
 
 	/* BUG if operation crosses the win_size */
 	BUG_ON(!((to + len) % state->win_size <= (to + len)));
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_toio(map->virt + (to % state->win_size), from, len);
+	while (len) {
+		if ((to % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (to % state->win_size);
+		else
+			this_len = len;
+
+		gf_set_gpios(state, to);
+		memcpy_toio(map->virt + (to % state->win_size), from, len);
+
+		len -= this_len;
+		to += this_len;
+		from += this_len;
+	}
 }
 
 static const char * const part_probe_types[] = {
-- 
1.7.9.5

[PATCH] gpio_flash: fix the CPLB miss bug for gpio expanded flash

[Aaron Wu]

In this bug, the address operation range may exceed the
window size defined by gpio expanded flash MTD partition.

Signed-off-by: Aaron Wu <Aaron.wu@analog.com>
---
 drivers/mtd/maps/gpio-addr-flash.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index a4c477b..ca9282f 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -108,13 +108,24 @@ static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssi
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, from);
+	int this_len;
 
 	/* BUG if operation crosses the win_size */
 	BUG_ON(!((from + len) % state->win_size <= (from + len)));
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_fromio(to, map->virt + (from % state->win_size), len);
+	while (len) {
+		if ((from % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (from % state->win_size);
+		else
+			this_len = len;
+
+		gf_set_gpios(state, from);
+		memcpy_fromio(to, map->virt + (from % state->win_size)
+			, this_len);
+		len -= this_len;
+		from += this_len;
+		to += this_len;
+	}
 }
 
 /**
@@ -147,13 +158,24 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 {
 	struct async_state *state = gf_map_info_to_state(map);
 
-	gf_set_gpios(state, to);
+	int this_len;
 
 	/* BUG if operation crosses the win_size */
 	BUG_ON(!((to + len) % state->win_size <= (to + len)));
 
-	/* operation does not cross the win_size, so one shot it */
-	memcpy_toio(map->virt + (to % state->win_size), from, len);
+	while (len) {
+		if ((to % state->win_size) + len > state->win_size)
+			this_len = state->win_size - (to % state->win_size);
+		else
+			this_len = len;
+
+		gf_set_gpios(state, to);
+		memcpy_toio(map->virt + (to % state->win_size), from, len);
+
+		len -= this_len;
+		to += this_len;
+		from += this_len;
+	}
 }
 
 static const char * const part_probe_types[] = {
-- 
1.7.9.5