Patchwork [v5,5/9,MTD] remove bogus warning about missing boot bank location

login
register
mail settings
Submitter Guillaume LECERF
Date April 24, 2010, 3:58 p.m.
Message ID <20100424155812.14723.66554.stgit@shiryu.yomgui.biz>
Download mbox | patch
Permalink /patch/50902/
State New
Headers show

Comments

Guillaume LECERF - April 24, 2010, 3:58 p.m.
From: Uwe Kleine-König <Uwe.Kleine-Koenigi@digi.com>

After the deleted block bootloc is only used once as follows:

	if (bootloc == 3 && something_else) {
		...

So setting bootloc = 2 doesn't change anything.  Taking that the warning is
wrong and missleading.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
Acked-by: Christopher Moore <moore@free.fr>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)
Artem Bityutskiy - April 29, 2010, 5:57 a.m.
On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote:
> From: Uwe Kleine-König <Uwe.Kleine-Koenigi@digi.com>
> 
> After the deleted block bootloc is only used once as follows:
> 
> 	if (bootloc == 3 && something_else) {
> 		...
> 
> So setting bootloc = 2 doesn't change anything.  Taking that the warning is
> wrong and missleading.
> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
> Acked-by: Christopher Moore <moore@free.fr>

Pushed this v5 series to l2-mtd-2.6 / dunno. But please, if you re-send
the series, use consistent "mtd: cfi_cmdset_0002:" prefix for this
patch. Then I will be able to do git-am without manual work. Now I have
to tweak this patch because git am removes [MTD].
Guillaume LECERF - April 29, 2010, 11:01 a.m.
2010/4/29 Artem Bityutskiy <dedekind1@gmail.com>:
> Pushed this v5 series to l2-mtd-2.6 / dunno. But please, if you re-send
> the series, use consistent "mtd: cfi_cmdset_0002:" prefix for this
> patch. Then I will be able to do git-am without manual work. Now I have
> to tweak this patch because git am removes [MTD].

Thanks for pushing it.
And I take your advice into consideration for my next series.
Guillaume LECERF - May 11, 2010, 8:43 a.m.
Hi.

2010/4/29 Artem Bityutskiy <dedekind1@gmail.com>:
> Pushed this v5 series to l2-mtd-2.6 / dunno. But please, if you re-send
> the series, use consistent "mtd: cfi_cmdset_0002:" prefix for this
> patch. Then I will be able to do git-am without manual work. Now I have
> to tweak this patch because git am removes [MTD].

David, could you please pick this series and push it to mtd-2.6.git
before the merge window ?
Thanks.
David Woodhouse - May 14, 2010, 12:19 a.m.
On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote:
> From: Uwe Kleine-König <Uwe.Kleine-Koenigi@digi.com>
> 
> After the deleted block bootloc is only used once as follows:
> 
>         if (bootloc == 3 && something_else) {
>                 ...
> 
> So setting bootloc = 2 doesn't change anything.  Taking that the
> warning is wrong and missleading.
> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
> Acked-by: Christopher Moore <moore@free.fr> 

The warning should be preserved for invalid values, or values we don't
support. I don't think it's correct to just remove it.

Looking at the latest datasheet, it looks like values of 0x04 or 0x05
are acceptable, meaning "Uniform, bottom WP protect" and "Uniform, top
WP protect" respectively.

Zero is acceptable only for pre-CFI 1.1 devices, and in that case it
says "refer to device ID code for Top/Bottom boot version of AM29LV160
or AM29LV116". But perhaps we should just probe such devices using the
JEDEC mode anyway.
Chris Moore - May 14, 2010, 6:17 a.m.
Le 14/05/2010 02:19, David Woodhouse a écrit :
> On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote:
>    
>> From: Uwe Kleine-König<Uwe.Kleine-Koenigi@digi.com>
>>
>> After the deleted block bootloc is only used once as follows:
>>
>>          if (bootloc == 3&&  something_else) {
>>                  ...
>>
>> So setting bootloc = 2 doesn't change anything.  Taking that the
>> warning is wrong and missleading.
>>
>> Signed-off-by: Uwe Kleine-König<Uwe.Kleine-Koenig@digi.com>
>> Signed-off-by: Guillaume LECERF<glecerf@gmail.com>
>> Acked-by: Christopher Moore<moore@free.fr>
>>      
> The warning should be preserved for invalid values, or values we don't
> support. I don't think it's correct to just remove it.
>
>    

But, David, the only effect of this code is to print an *erroneous* message.
The message says "Assuming top" but sets bootloc to bottom (2) :(
See this thread: http://thread.gmane.org/gmane.linux.drivers.mtd/22176
Remark: this would be even clearer if the bootloc values were macros 
instead of 2, 3, ...

If you really want to keep the message then it must at least be changed 
to "Assuming bottom".

Cheers,
Chris
David Woodhouse - May 14, 2010, 7:54 a.m.
On Fri, 2010-05-14 at 08:17 +0200, Chris Moore wrote:
> Le 14/05/2010 02:19, David Woodhouse a écrit :
> > On Sat, 2010-04-24 at 17:58 +0200, Guillaume LECERF wrote:
> >    
> >> From: Uwe Kleine-König<Uwe.Kleine-Koenigi@digi.com>
> >>
> >> After the deleted block bootloc is only used once as follows:
> >>
> >>          if (bootloc == 3&&  something_else) {
> >>                  ...
> >>
> >> So setting bootloc = 2 doesn't change anything.  Taking that the
> >> warning is wrong and missleading.
> >>
> >> Signed-off-by: Uwe Kleine-König<Uwe.Kleine-Koenig@digi.com>
> >> Signed-off-by: Guillaume LECERF<glecerf@gmail.com>
> >> Acked-by: Christopher Moore<moore@free.fr>
> >>      
> > The warning should be preserved for invalid values, or values we don't
> > support. I don't think it's correct to just remove it.
> >
> >    
> 
> But, David, the only effect of this code is to print an *erroneous* message.
> The message says "Assuming top" but sets bootloc to bottom (2) :(
> See this thread: http://thread.gmane.org/gmane.linux.drivers.mtd/22176
> Remark: this would be even clearer if the bootloc values were macros 
> instead of 2, 3, ...
> 
> If you really want to keep the message then it must at least be changed 
> to "Assuming bottom".

http://git.infradead.org/mtd-2.6.git/commitdiff/412da2f6
Guillaume LECERF - May 14, 2010, 8:09 a.m.
2010/5/14 David Woodhouse <dwmw2@infradead.org>:
> http://git.infradead.org/mtd-2.6.git/commitdiff/412da2f6

Thanks David.

One thing I noted :

+                       if ((bootloc < 2) || (bootloc > 5)) {
+                               printk(KERN_WARNING "%s: CFI contains
unrecognised boot "
+                                      "bank location (%d). Assuming bottom.\n",
+                                      bootloc, map->name);

I think bootloc and map->name need to be swapped to match the printk
pattern order (%s first then %d).
David Woodhouse - May 14, 2010, 8:15 a.m.
On Fri, 2010-05-14 at 10:09 +0200, Guillaume LECERF wrote:
> I think bootloc and map->name need to be swapped to match the printk
> pattern order (%s first then %d). 

Doh. Fixed; thanks.

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ea2a7f6..8da8655 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -391,11 +391,6 @@  struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 #endif
 
 		bootloc = extp->TopBottom;
-		if ((bootloc != 2) && (bootloc != 3)) {
-			printk(KERN_WARNING "%s: CFI does not contain boot "
-			       "bank location. Assuming top.\n", map->name);
-			bootloc = 2;
-		}
 
 		if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
 			printk(KERN_WARNING "%s: Swapping erase regions for broken CFI table.\n", map->name);