Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1308823 > unrolled thread

[PATCH] mtd: nuc900_nand: read correct SMISR register

Back to article view | Back to linux.kernel

Contents

  [PATCH] mtd: nuc900_nand: read correct SMISR register Arnd Bergmann <arnd@arndb.de> - 2016-01-13 22:40 +0100
    Re: [PATCH] mtd: nuc900_nand: read correct SMISR register Brian Norris <computersforpeace@gmail.com> - 2016-01-13 23:00 +0100
      Re: [PATCH] mtd: nuc900_nand: read correct SMISR register Arnd Bergmann <arnd@arndb.de> - 2016-01-13 23:10 +0100
        Re: [PATCH] mtd: nuc900_nand: read correct SMISR register Brian Norris <computersforpeace@gmail.com> - 2016-01-14 01:30 +0100
          Re: [PATCH] mtd: nuc900_nand: read correct SMISR register Wan Zongshun <vw@iommu.org> - 2016-01-14 10:50 +0100
            Re: [PATCH] mtd: nuc900_nand: read correct SMISR register Arnd Bergmann <arnd@arndb.de> - 2016-01-14 13:10 +0100
              Re: [PATCH] mtd: nuc900_nand: read correct SMISR register Wan Zongshun <vw@iommu.org> - 2016-01-15 09:20 +0100

#1308823 — [PATCH] mtd: nuc900_nand: read correct SMISR register

The nuc900_nand driver has always passed an incorrect register
address in its nuc900_check_rb() function, which cannot possibly
work, and in some configurations gives us a build warning:

drivers/mtd/nand/nuc900_nand.c: In function 'nuc900_check_rb':
drivers/mtd/nand/nuc900_nand.c:27:23: warning: passing argument 1 of '__raw_readl' makes pointer from integer without a cast [-Wint-conversion]
 #define REG_SMISR     0xac
drivers/mtd/nand/nuc900_nand.c:118:20: note: in expansion of macro 'REG_SMISR'
  val = __raw_readl(REG_SMISR);

This makes sure we actually read from the register rather than
from (void *)0x000000ac in user space.

I suspect nobody noticed this before because the nuc900_nand_devready()
function never gets called, or nobody uses this driver on an upstream
kernel. Possibly even both.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
index 220ddfcf29f5..dbc5b571c2bb 100644
--- a/drivers/mtd/nand/nuc900_nand.c
+++ b/drivers/mtd/nand/nuc900_nand.c
@@ -113,7 +113,7 @@ static int nuc900_check_rb(struct nuc900_nand *nand)
 {
 	unsigned int val;
 	spin_lock(&nand->lock);
-	val = __raw_readl(REG_SMISR);
+	val = __raw_readl(nand->reg + REG_SMISR);
 	val &= READYBUSY;
 	spin_unlock(&nand->lock);
 

[toc] | [next] | [standalone]

#1308849

On Wed, Jan 13, 2016 at 10:38:08PM +0100, Arnd Bergmann wrote:
> The nuc900_nand driver has always passed an incorrect register
> address in its nuc900_check_rb() function, which cannot possibly
> work, and in some configurations gives us a build warning:
> 
> drivers/mtd/nand/nuc900_nand.c: In function 'nuc900_check_rb':
> drivers/mtd/nand/nuc900_nand.c:27:23: warning: passing argument 1 of '__raw_readl' makes pointer from integer without a cast [-Wint-conversion]
>  #define REG_SMISR     0xac
> drivers/mtd/nand/nuc900_nand.c:118:20: note: in expansion of macro 'REG_SMISR'
>   val = __raw_readl(REG_SMISR);
> 
> This makes sure we actually read from the register rather than
> from (void *)0x000000ac in user space.
> 
> I suspect nobody noticed this before because the nuc900_nand_devready()
> function never gets called, or nobody uses this driver on an upstream
> kernel. Possibly even both.

Almost definitely not the first. That's an absolutely essential
function for this driver (it doesn't have ->waitfunc(), so we use
->dev_ready() all the time). Quite likely the latter.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
> index 220ddfcf29f5..dbc5b571c2bb 100644
> --- a/drivers/mtd/nand/nuc900_nand.c
> +++ b/drivers/mtd/nand/nuc900_nand.c
> @@ -113,7 +113,7 @@ static int nuc900_check_rb(struct nuc900_nand *nand)
>  {
>  	unsigned int val;
>  	spin_lock(&nand->lock);
> -	val = __raw_readl(REG_SMISR);
> +	val = __raw_readl(nand->reg + REG_SMISR);
>  	val &= READYBUSY;
>  	spin_unlock(&nand->lock);
>  

Looks OK to me, though I kinda hate dragging on support for
obviously-unused drivers...

Brian

[toc] | [prev] | [next] | [standalone]

#1308861

On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
> 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > diff --git a/drivers/mtd/nand/nuc900_nand.c b/drivers/mtd/nand/nuc900_nand.c
> > index 220ddfcf29f5..dbc5b571c2bb 100644
> > --- a/drivers/mtd/nand/nuc900_nand.c
> > +++ b/drivers/mtd/nand/nuc900_nand.c
> > @@ -113,7 +113,7 @@ static int nuc900_check_rb(struct nuc900_nand *nand)
> >  {
> >       unsigned int val;
> >       spin_lock(&nand->lock);
> > -     val = __raw_readl(REG_SMISR);
> > +     val = __raw_readl(nand->reg + REG_SMISR);
> >       val &= READYBUSY;
> >       spin_unlock(&nand->lock);
> >  
> 
> Looks OK to me, though I kinda hate dragging on support for
> obviously-unused drivers...

Should we mark that driver in Kconfig as obviously broken then?

Let's wait for Wan ZongShun to reply first, it's possible that the
entire w90x900 platform has come to the point where we are better off
removing it than fixing ancient bugs.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1308912

On Wed, Jan 13, 2016 at 11:02:06PM +0100, Arnd Bergmann wrote:
> On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
> > 
> > Looks OK to me, though I kinda hate dragging on support for
> > obviously-unused drivers...
> 
> Should we mark that driver in Kconfig as obviously broken then?

Well, it won't be obviously broken if I apply your patch... But if
that's the right step toward removal, then I could be OK with that.

> Let's wait for Wan ZongShun to reply first, it's possible that the
> entire w90x900 platform has come to the point where we are better off
> removing it than fixing ancient bugs.

Sounds good.

Brian

[toc] | [prev] | [next] | [standalone]

#1309122

-------- Original Message --------
> On Wed, Jan 13, 2016 at 11:02:06PM +0100, Arnd Bergmann wrote:
>> On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
>>>
>>> Looks OK to me, though I kinda hate dragging on support for
>>> obviously-unused drivers...
>>
>> Should we mark that driver in Kconfig as obviously broken then?
>
> Well, it won't be obviously broken if I apply your patch... But if
> that's the right step toward removal, then I could be OK with that.
>
>> Let's wait for Wan ZongShun to reply first, it's possible that the
>> entire w90x900 platform has come to the point where we are better off
>> removing it than fixing ancient bugs.
>
> Sounds good.

Actually, Nuvoton should still leverage this upstream w90x900 codes for 
their old and new arm chip BSP, but I am not sure their open source plan 
for the new chip now, I will check with Nuvoton for this topic, and give 
you feedback here, so please hold on its removal.

>
> Brian
>

[toc] | [prev] | [next] | [standalone]

#1309209

On Thursday 14 January 2016 17:34:33 Wan Zongshun wrote:
> -------- Original Message --------
> > On Wed, Jan 13, 2016 at 11:02:06PM +0100, Arnd Bergmann wrote:
> >> On Wednesday 13 January 2016 13:50:13 Brian Norris wrote:
> >>>
> >>> Looks OK to me, though I kinda hate dragging on support for
> >>> obviously-unused drivers...
> >>
> >> Should we mark that driver in Kconfig as obviously broken then?
> >
> > Well, it won't be obviously broken if I apply your patch... But if
> > that's the right step toward removal, then I could be OK with that.
> >
> >> Let's wait for Wan ZongShun to reply first, it's possible that the
> >> entire w90x900 platform has come to the point where we are better off
> >> removing it than fixing ancient bugs.
> >
> > Sounds good.
> 
> Actually, Nuvoton should still leverage this upstream w90x900 codes for 
> their old and new arm chip BSP, but I am not sure their open source plan 
> for the new chip now, I will check with Nuvoton for this topic, and give 
> you feedback here, so please hold on its removal.

Ok, sure. Thanks for the quick reply!

I've had a look around at the current produce lineup, and it seems that
nuc900 (w90x900) is still marketed, and as you say is similar to the n329
series.

There has been one attempt to do a modern port for n329 in 2014 but
it never got submitted. See http://comments.gmane.org/gmane.linux.kernel.kernelnewbies/49077
and https://github.com/mpthompson/linux/tree/n329

The code looks rather nice, so it's a pity that the effort stalled,
but it should not be hard for anyone to start out with Mike's tree
and forward-port it to 4.5.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1309942

>>
>> Actually, Nuvoton should still leverage this upstream w90x900 codes for
>> their old and new arm chip BSP, but I am not sure their open source plan
>> for the new chip now, I will check with Nuvoton for this topic, and give
>> you feedback here, so please hold on its removal.
>
> Ok, sure. Thanks for the quick reply!
>
> I've had a look around at the current produce lineup, and it seems that
> nuc900 (w90x900) is still marketed, and as you say is similar to the n329
> series.
>
> There has been one attempt to do a modern port for n329 in 2014 but
> it never got submitted. See http://comments.gmane.org/gmane.linux.kernel.kernelnewbies/49077
> and https://github.com/mpthompson/linux/tree/n329
>
> The code looks rather nice, so it's a pity that the effort stalled,
> but it should not be hard for anyone to start out with Mike's tree
> and forward-port it to 4.5.

I will try to get this board you mentioned and try to cowork Mike to 
submit it into upstream.

After checking with Nuvoton people, and I will get another board and 
help them update the latest kernel BSP and also submit new nuc970 chip 
BSP into upstream.

spec:
http://www.nuvoton.com/hq/products/microprocessors/arm9-mpus/nuc900-series/?__locale=en

>
> 	Arnd
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web