Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb

From: Guido Guenther <agx@sigxcpu.org>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Linus Torvalds <torvalds@osdl.org>,
	adaplas@pol.net,
	Linux Fbdev development list 
	<linux-fbdev-devel@lists.sourceforge.net>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@osdl.org>
Subject: Re: [Linux-fbdev-devel] Re: [PATCH] fbdev: Fix IO access in rivafb
Date: Sat, 13 Nov 2004 12:22:35 +0100	[thread overview]
Message-ID: <20041113112234.GA5523@bogon.ms20.nix> (raw)
In-Reply-To: <1100309972.20511.103.camel@gaston>

On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:
> 
> > O.k., it was the __raw_{write,read}b which broke things, not the
> > "alignment". This one works:
> > 
> > diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h
> > --- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h	2004-11-12 13:42:54.000000000 +0100
> > +++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-12 17:39:22.000000000 +0100
> > @@ -75,8 +75,8 @@
> >   */
> >  #include <asm/io.h>
> >  
> > -#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
> > -#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
> > +#define NV_WR08(p,i,d)  (writeb((d), (void __iomem *)(p) + (i)))
> > +#define NV_RD08(p,i)    (readb((void __iomem *)(p) + (i)))
> 
> Interesting. The only difference here should be barriers. I hate the
> lack of barriers in that driver ... I'm not sure the driver may not have
Yes, it's a real pain. Missing barriers in RIVA_FIFO_FREE caused lots of
lockups, until I found them in your 2.4 tree.

> other bugs related to the lack of them in the 16 and 32 bits accessors.
> It does use non-barrier version on purpose in some accel ops though,
> when filling the fifo with pixels, but that's pretty much the only case
> where it makes sense.
> 
> > There aren't any, I actually attached the wrong patch. The non-working
> > version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
> > riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
> > writeb/readb now looks correct since these map to {in,out}_8 now.
> 
> {in,out}_8 are ppc-specific things that are identical to readb/writeb
> indeed, with barriers.
In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
__raw_{read,write}b is different. So you mean __raw_{read,write}b in the
above? (no nitpicking, just want to be sure I understand this
correctly).
Cheers,
 -- Guido