More info for DSM w/r/t sunffb on 2.6.15-rc6

More info for DSM w/r/t sunffb on 2.6.15-rc6

[Michael Bishop]

I'd like to provide some more information in regards to the recent
thread concerning sunffb driver trouble in current kernels.

There seems to be some confusion regarding the exact failure mode.  Let
me try and explain in more detail.

I'm running 2.6.15-rc6 on an ultra-60 (dual 450mhz blackbird).  I have a
ffb (creator3d series 3) and am using x.org 6.8.2-r6. Console
framebuffer mode works fine.  When running a startx and using fmccor's
2.6 kernel example xorg.conf the following happens:

1.  Screen clears and there is an underscore character in the top left
corner.
2.  Slight screen distortion like a handful of white scratches appears
for a split second.
3.  Screen is blank except for the underscore, but stable w/no
distortion.   Hard drive activity is heard.
4.  CTRL-ALT-BACKSPACE works and kills X, putting me back at my shell
prompt.
5.  The following is seen appended to normal dmesg output:

Bad pte = 1fa00600a88, process = X, vm_flags = 184473, vaddr = 7001e000
Call Trace:
 [000000000047e0f4] copy_page_range+0x174/0x1e0
 [0000000000447fac] copy_mm+0x1ec/0x340
 [000000000044898c] copy_process+0x34c/0xc40
 [0000000000449344] do_fork+0x44/0x1e0
 [00000000004071d4] linux_sparc_syscall32+0x34/0x40
 [00000000701da950] 0x701da950
Bad pte = 1fa00600a88, process = ???, vm_flags = 184473, vaddr =
7001e000
Call Trace:
 [000000000047e5f4] unmap_page_range+0x174/0x1a0
 [000000000047e714] unmap_vmas+0xf4/0x260
 [0000000000483c48] exit_mmap+0x88/0x160
 [0000000000447c30] mmput+0x30/0xe0
 [000000000049c59c] exec_mmap+0x19c/0x220
 [000000000049c774] flush_old_exec+0xd4/0x7a0
 [0000000000432d74] load_elf_binary+0x3b4/0xec0
 [000000000049d138] search_binary_handler+0x98/0x360
 [00000000004be6e4] compat_do_execve+0x124/0x1e0
 [0000000000429fc8] sparc32_execve+0x48/0xc0
 [00000000004071d4] linux_sparc_syscall32+0x34/0x40
 [00000000701daaac] 0x701daaac
Bad pte = 800001fa00600e88, process = X, vm_flags = 184473, vaddr =
7001e000
Call Trace:
 [000000000047e5f4] unmap_page_range+0x174/0x1a0
 [000000000047e714] unmap_vmas+0xf4/0x260
 [000000000048354c] unmap_region+0x8c/0x140
 [00000000004838f0] do_munmap+0x110/0x160
 [000000000048395c] sys_munmap+0x1c/0x40
 [00000000004071d4] linux_sparc_syscall32+0x34/0x40
 [0000000070871988] 0x70871988

For the heck of it, I had tried using PROM console output rather than
the console framebuffer driver, thinking perhaps I couldn't use the
console framebuffer support AND x.org at the same time.  running startx
in this instance just gave me a blank white screen until i hit
ctrl-alt-bksp.

Please CC me directly on any responses to this thread as I am not
currently subscribed to the linux kernel mailing list.

I'd like to thank David Miller in advance for all his work on the sparc
support.  Fantastic job.

I should also note that initial tests with 2.6.15-rc5+ show that a bug I
had where the system would completely lock up solid during heavy IO load
appears to be fixed.  Ran a script to generate IO on a 10-disk raid-5
overnight and it was still running this morning.  Previously (2.6.13 and
back and 2.4 kernels) the system would lock up after only a couple
minutes.

regards,

mpb

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[David S. Miller]

From: "Michael Bishop" <michael.bishop@APPIQ.com>
Date: Fri, 23 Dec 2005 11:23:36 -0500

> Bad pte = 1fa00600a88, process = X, vm_flags = 184473, vaddr = 7001e000
> Call Trace:

Strange, I thought we'd fixed this.

Aww crap....

Linus, X.org is doing a MAP_PRIVATE mmap() of these discontiguous
I/O mappings of the sparc frame buffer device it seems.  So the
MAP_SHARED check in is_cow_mapping() doesn't pass.

Michael, does X.org work properly with your FFB card with 2.6.14 by
chance?

I really haven't used X.org on anything other than an ATI Radeon on
Sparc boxes, so it's highly possible that SunFFB support in X.org has
deteriorated into a non-working state due to not being looked after by
anyone.

Back to the MAP_SHARED issue, the culprit code in X.org can be fixed
but I know that this thing has been coded this way for years.  It
goes like this (hw/xfree86/os-support/bus/Sbus.c:xf86MapSbusMem()):

    ret = (pointer) mmap (NULL, len, PROT_READ | PROT_WRITE, MAP_PRIVATE,
                          psdp->fd, off);
    if (ret == (pointer) -1) {
        ret = (pointer) mmap (NULL, len, PROT_READ | PROT_WRITE, MAP_SHARED,
                              psdp->fd, off);
    }
    if (ret == (pointer) -1)
        return NULL;

and that should be fixed to just be:

    ret = (pointer) mmap (NULL, len, PROT_READ | PROT_WRITE, MAP_SHARED,
                          psdp->fd, off);
    if (ret == (pointer) -1)
        return NULL;

Ie. use MAP_SHARED unconditionally.

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[Linus Torvalds]

[ Hugh and Nick added to Cc, just in case they can see anything wrong with 
  this. It's actually very simple, but I hoped to avoid having to support
  the insane cases. I guess it was naive of me to think that there isn't 
  always _some_ insane user ;^]

David: please read the final note before the patch about software 
dirty/accessed bits. It may or may not be an issue on sparc64, I don't 
know where you do the dirty/accessed bit handling.

On Fri, 23 Dec 2005, David S. Miller wrote:
> 
> Linus, X.org is doing a MAP_PRIVATE mmap() of these discontiguous
> I/O mappings of the sparc frame buffer device it seems.  So the
> MAP_SHARED check in is_cow_mapping() doesn't pass.

Ok. I actually had a backup plan for that too, but was hoping that nobody 
would be quite that insane.

And doing a private mapping on a device and then expecting to do writes 
through that mapping is just totally insane. The fact that it happened to 
work before was arguably very much a bug (as it didn't actually create a 
private mapping).

But hey, here's the backup plan. It's entirely untested, but it is 
actually very simple, and has way more comments than actual code, so 
hopefully it's understandable and "obviously right" (yeah sure, famous 
last words ;).

NOTE! This very much means that an insane user that first does a private 
writable mapping of a device like this , and then a fork(), will _not_ see 
the mapping in the child. It will remain "private" and writable in the 
parent instead (writable only if the page protections that were passed in 
to remap_pfn_range() were writable, of course).

The reason? Doing a COW after the fork would be insane. Both from a VM 
complexity issue (it's what all the work has been trying to avoid), but 
also from a "what the hell does it mean?" kind of issue. 

So I'm pretty damn sure nobody depends on _that_ at least (old kernels 
would have done the insane and meaningless thing: it would basically be 
mapped shared until the fork() happened, and then it would be 
copy-on-write in _both_ processes after the fork).

NOTE NOTE NOTE! This will _not_ work if the pages need a software-dirty 
and/or software-accessed bit, and the low-level architecture page fault 
handler says "this is a write to a nonwritable area" and raises a SIGSEGV. 

So it may be that the insane sparc remap_pfn_range() users need to set the 
dirty/accessed bits in the page protection flags by hand before to avoid 
that. David?

Michael, can you test this patch (with the note that David may need to 
change something else too)?

So there are certainly some subtle issues left with this, and I'm not 
saying it's quite this simple, but they should be easy enough to handle.

			Linus

----
diff --git a/mm/memory.c b/mm/memory.c
index d8dde07..a93654d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1326,9 +1326,32 @@ int remap_pfn_range(struct vm_area_struc
 	 * un-COW'ed pages by matching them up with "vma->vm_pgoff".
 	 */
 	if (is_cow_mapping(vma->vm_flags)) {
-		if (addr != vma->vm_start || end != vma->vm_end)
-			return -EINVAL;
-		vma->vm_pgoff = pfn;
+		/*
+		 * We can do a real COW mapping _if_ it covers the whole area,
+		 * at which point we do the magic "vm_pgoff" trick.
+		 *
+		 * Otherwise we will have to turn it into non-copyable shared
+		 * area which has VM_WRITE turned off.
+		 *
+		 * NOTE NOTE NOTE! If the remap_pfn_range() was called with
+		 * a writable page protection, this means that the pages will
+		 * still be writable, but we will refuse to ever take a
+		 * write fault on any pages that weren't so.
+		 *
+		 * Also, we refuse to do the SHARED conversion if we already
+		 * have taken a C-O-W fault on the area.
+		 *
+		 * This is all just for insane old X servers, which map the
+		 * video pages private.
+		 */
+		if (addr == vma->vm_start && end == vma->vm_end) {
+			vma->vm_pgoff = pfn;
+		} else {
+			if (vma->anon_vma)
+				return -EINVAL;
+			vma->vm_flags |= VM_DONTCOPY | VM_SHARED;
+			vma->vm_flags &= ~(VM_WRITE | VM_MAYWRITE);
+		}
 	}
 
 	vma->vm_flags |= VM_IO | VM_RESERVED | VM_PFNMAP;

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Fri, 23 Dec 2005 12:53:16 -0800 (PST)

> So it may be that the insane sparc remap_pfn_range() users need to set the 
> dirty/accessed bits in the page protection flags by hand before to avoid 
> that. David?

I'm pretty sure we set the dirty accessed bits at mapping time, so
that shouldn't be an issue.

But I wonder whether any of this is necessary at all.

I did some digging to see how far back the "fall back to MAP_SHARED
if MAP_PRIVATE fails" logic is in the X11R6 tree.

I went back as far as I could in the XORG and XFree86 CVS for
that SBUS support code, and the fallback to MAP_SHARED code has
always been there.

So I think something as simple as returning -EINVAL in the SBUS
framebuffer mmap() driver if VM_SHARED is not set would be sufficient
to deal with this.

Something like this patch below.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/video/sbuslib.c b/drivers/video/sbuslib.c
index 646c43f..ac937da 100644
--- a/drivers/video/sbuslib.c
+++ b/drivers/video/sbuslib.c
@@ -46,6 +46,9 @@ int sbusfb_mmap_helper(struct sbus_mmap_
 	unsigned long off;
 	int i;
                                         
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;
+
 	size = vma->vm_end - vma->vm_start;
 	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
 		return -EINVAL;

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[Hugh Dickins]

On Fri, 23 Dec 2005, David S. Miller wrote:
> 
> So I think something as simple as returning -EINVAL in the SBUS
> framebuffer mmap() driver if VM_SHARED is not set would be sufficient
> to deal with this.

That certainly gets my vote: it should work around the bug correctly
and effectively without adding any complexity.

Though really the check ought to be in the sparc and sparc64
io_remap_pfn_range, which are the guilty parties giving shared write
access even when none has been asked for.  But I guess it's too risky
to add failures or change behaviour down there at this stage.

Those "prot = __pgprot(pg_iobits);" lines - any idea why they ever
got inserted?  I guess to add _PAGE_E in the sparc64 case, and
whatever the equivalent was in the earlier sparc cases?
Can they safely be corrected early in 2.6.16?

Hugh

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[David S. Miller]

From: Hugh Dickins <hugh@veritas.com>
Date: Sat, 24 Dec 2005 01:21:07 +0000 (GMT)

> That certainly gets my vote: it should work around the bug correctly
> and effectively without adding any complexity.

And it's even tested successfully :-)

There is an obscure program I wrote a long time ago, which
Michael Bishop spotted, which is in very limited use and doesn't
fall back to MAP_SHARED properly.  But that obscure tool can
be fixed.

> Though really the check ought to be in the sparc and sparc64
> io_remap_pfn_range, which are the guilty parties giving shared write
> access even when none has been asked for.  But I guess it's too risky
> to add failures or change behaviour down there at this stage.

That's a bit too far reaching right now.

> Those "prot = __pgprot(pg_iobits);" lines - any idea why they ever
> got inserted?  I guess to add _PAGE_E in the sparc64 case, and
> whatever the equivalent was in the earlier sparc cases?
> Can they safely be corrected early in 2.6.16?

Corrected?  By that you mean removed?  We have so many hacks
in the tree dealing with this kind of stuff.  For example,
pgprot_noncached() as used by things like snd_pccm_lib_mmap_iomem().
And the uses of that conditionalize with an ifdef, which is how
we get half-implemented interfaces like this which you can't
use consistently, oh and wo' be to the person who defines that
using an inline function and wonders why it never gets ued :-/

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[Hugh Dickins]

On Fri, 23 Dec 2005, David S. Miller wrote:
> From: Hugh Dickins <hugh@veritas.com>
> Date: Sat, 24 Dec 2005 01:21:07 +0000 (GMT)
> 
> > Those "prot = __pgprot(pg_iobits);" lines - any idea why they ever
> > got inserted?  I guess to add _PAGE_E in the sparc64 case, and
> > whatever the equivalent was in the earlier sparc cases?
> > Can they safely be corrected early in 2.6.16?
> 
> Corrected?  By that you mean removed?

Removed would make the source look prettier, but I assume it's
there for a reason, and should be corrected rather than removed.

I assume the reason is to add some necessary flagbits into prot;
and not to violate the permissions model by giving shared write
access to areas mapped privately.

I was wondering your estimation of the likelihood of problems if we
change sparc and sparc64 io_remap_pfn_range to respect the distinction
between shared and private, readonly and writable, early in 2.6.16,
or early in 2.6.17.

But this incident of X trying for MAP_PRIVATE (wanting that to mean
shared) before MAP_SHARED shows we cannot assume sanity around here.
Looks like we'd need to scatter VM_SHARED tests (like yours) around
various driver mmaps at the same time, to get X back to working on them.

I knew there were several drivers ignoring vm_page_prot in their calls
to (io_)remap_pfn_range; I hadn't realized that whole architectures
were doing so in low-level functions used by many.

Or is there a good argument that the shared-write/private-readonly
distinction makes no sense on anything you might apply
io_remap_pfn_range to?  That read access to the device amounts
to write access, because of side-effects?  (I know nothing of this,
I'm just trying to guess how I might be fussing unnecessarily over it.)

> We have so many hacks
> in the tree dealing with this kind of stuff.  For example,
> pgprot_noncached() as used by things like snd_pccm_lib_mmap_iomem().

pgprot_noncached looks okay to me: not pretty, but doing just what
I'd expect, adding in some necessary flagbits while respecting the
permissions.  A hack yes (a subsequent mprotect would lose the
added flagbits I think), but good enough for most.

Hugh

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[David S. Miller]

From: Hugh Dickins <hugh@veritas.com>
Date: Sat, 24 Dec 2005 09:07:29 +0000 (GMT)

> I was wondering your estimation of the likelihood of problems if we
> change sparc and sparc64 io_remap_pfn_range to respect the distinction
> between shared and private, readonly and writable, early in 2.6.16,
> or early in 2.6.17.

I don't anticipate any, what they are doing is clearly broken.

I'll push the following fix soon.

Thanks for pointing all of this stuff out Hugh.

diff --git a/arch/sparc/mm/generic.c b/arch/sparc/mm/generic.c
index 2cb0728..1ef7fa0 100644
--- a/arch/sparc/mm/generic.c
+++ b/arch/sparc/mm/generic.c
@@ -76,7 +76,6 @@ int io_remap_pfn_range(struct vm_area_st
 	vma->vm_pgoff = (offset >> PAGE_SHIFT) |
 		((unsigned long)space << 28UL);
 
-	prot = __pgprot(pg_iobits);
 	offset -= from;
 	dir = pgd_offset(mm, from);
 	flush_cache_range(vma, beg, end);
diff --git a/arch/sparc/mm/loadmmu.c b/arch/sparc/mm/loadmmu.c
index e9f9571..36b4d24 100644
--- a/arch/sparc/mm/loadmmu.c
+++ b/arch/sparc/mm/loadmmu.c
@@ -22,8 +22,6 @@ struct ctx_list *ctx_list_pool;
 struct ctx_list ctx_free;
 struct ctx_list ctx_used;
 
-unsigned int pg_iobits;
-
 extern void ld_mmu_sun4c(void);
 extern void ld_mmu_srmmu(void);
 
diff --git a/arch/sparc/mm/srmmu.c b/arch/sparc/mm/srmmu.c
index c664b96..27b0e0b 100644
--- a/arch/sparc/mm/srmmu.c
+++ b/arch/sparc/mm/srmmu.c
@@ -2130,6 +2130,13 @@ static unsigned long srmmu_pte_to_pgoff(
 	return pte_val(pte) >> SRMMU_PTE_FILE_SHIFT;
 }
 
+static pgprot_t srmmu_pgprot_noncached(pgprot_t prot)
+{
+	prot &= ~__pgprot(SRMMU_CACHE);
+
+	return prot;
+}
+
 /* Load up routines and constants for sun4m and sun4d mmu */
 void __init ld_mmu_srmmu(void)
 {
@@ -2150,9 +2157,9 @@ void __init ld_mmu_srmmu(void)
 	BTFIXUPSET_INT(page_readonly, pgprot_val(SRMMU_PAGE_RDONLY));
 	BTFIXUPSET_INT(page_kernel, pgprot_val(SRMMU_PAGE_KERNEL));
 	page_kernel = pgprot_val(SRMMU_PAGE_KERNEL);
-	pg_iobits = SRMMU_VALID | SRMMU_WRITE | SRMMU_REF;
 
 	/* Functions */
+	BTFIXUPSET_CALL(pgprot_noncached, srmmu_pgprot_noncached, BTFIXUPCALL_NORM);
 #ifndef CONFIG_SMP	
 	BTFIXUPSET_CALL(___xchg32, ___xchg32_sun4md, BTFIXUPCALL_SWAPG1G2);
 #endif
diff --git a/arch/sparc/mm/sun4c.c b/arch/sparc/mm/sun4c.c
index 731f196..49f28c1 100644
--- a/arch/sparc/mm/sun4c.c
+++ b/arch/sparc/mm/sun4c.c
@@ -1589,7 +1589,10 @@ static void sun4c_flush_tlb_page(struct 
 
 static inline void sun4c_mapioaddr(unsigned long physaddr, unsigned long virt_addr)
 {
-	unsigned long page_entry;
+	unsigned long page_entry, pg_iobits;
+
+	pg_iobits = _SUN4C_PAGE_PRESENT | _SUN4C_READABLE | _SUN4C_WRITEABLE |
+		    _SUN4C_PAGE_IO | _SUN4C_PAGE_NOCACHE;
 
 	page_entry = ((physaddr >> PAGE_SHIFT) & SUN4C_PFN_MASK);
 	page_entry |= ((pg_iobits | _SUN4C_PAGE_PRIV) & ~(_SUN4C_PAGE_PRESENT));
@@ -2134,6 +2137,13 @@ void __init sun4c_paging_init(void)
 	printk("SUN4C: %d mmu entries for the kernel\n", cnt);
 }
 
+static pgprot_t sun4c_pgprot_noncached(pgprot_t prot)
+{
+	prot |= __pgprot(_SUN4C_PAGE_IO | _SUN4C_PAGE_NOCACHE);
+
+	return prot;
+}
+
 /* Load up routines and constants for sun4c mmu */
 void __init ld_mmu_sun4c(void)
 {
@@ -2156,10 +2166,9 @@ void __init ld_mmu_sun4c(void)
 	BTFIXUPSET_INT(page_readonly, pgprot_val(SUN4C_PAGE_READONLY));
 	BTFIXUPSET_INT(page_kernel, pgprot_val(SUN4C_PAGE_KERNEL));
 	page_kernel = pgprot_val(SUN4C_PAGE_KERNEL);
-	pg_iobits = _SUN4C_PAGE_PRESENT | _SUN4C_READABLE | _SUN4C_WRITEABLE |
-		    _SUN4C_PAGE_IO | _SUN4C_PAGE_NOCACHE;
 
 	/* Functions */
+	BTFIXUPSET_CALL(pgprot_noncached, sun4c_pgprot_noncached, BTFIXUPCALL_NORM);
 	BTFIXUPSET_CALL(___xchg32, ___xchg32_sun4c, BTFIXUPCALL_NORM);
 	BTFIXUPSET_CALL(do_check_pgt_cache, sun4c_check_pgt_cache, BTFIXUPCALL_NORM);
 	
diff --git a/arch/sparc64/kernel/pci.c b/arch/sparc64/kernel/pci.c
index 95ffa94..dfccff2 100644
--- a/arch/sparc64/kernel/pci.c
+++ b/arch/sparc64/kernel/pci.c
@@ -656,6 +656,7 @@ int pci_mmap_page_range(struct pci_dev *
 	__pci_mmap_set_flags(dev, vma, mmap_state);
 	__pci_mmap_set_pgprot(dev, vma, mmap_state);
 
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	ret = io_remap_pfn_range(vma, vma->vm_start,
 				 vma->vm_pgoff,
 				 vma->vm_end - vma->vm_start,
@@ -663,7 +664,6 @@ int pci_mmap_page_range(struct pci_dev *
 	if (ret)
 		return ret;
 
-	vma->vm_flags |= VM_IO;
 	return 0;
 }
 
diff --git a/arch/sparc64/mm/generic.c b/arch/sparc64/mm/generic.c
index 5fc5c57..8cb0620 100644
--- a/arch/sparc64/mm/generic.c
+++ b/arch/sparc64/mm/generic.c
@@ -140,7 +140,6 @@ int io_remap_pfn_range(struct vm_area_st
 	vma->vm_flags |= VM_IO | VM_RESERVED | VM_PFNMAP;
 	vma->vm_pgoff = phys_base >> PAGE_SHIFT;
 
-	prot = __pgprot(pg_iobits);
 	offset -= from;
 	dir = pgd_offset(mm, from);
 	flush_cache_range(vma, beg, end);
diff --git a/drivers/char/drm/drm_vm.c b/drivers/char/drm/drm_vm.c
index 0291cd6..ffd0800 100644
--- a/drivers/char/drm/drm_vm.c
+++ b/drivers/char/drm/drm_vm.c
@@ -619,6 +619,7 @@ int drm_mmap(struct file *filp, struct v
 #endif
 		offset = dev->driver->get_reg_ofs(dev);
 #ifdef __sparc__
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 		if (io_remap_pfn_range(DRM_RPR_ARG(vma) vma->vm_start,
 				       (map->offset + offset) >> PAGE_SHIFT,
 				       vma->vm_end - vma->vm_start,
diff --git a/drivers/sbus/char/flash.c b/drivers/sbus/char/flash.c
index 6bdd768..2beb3dd 100644
--- a/drivers/sbus/char/flash.c
+++ b/drivers/sbus/char/flash.c
@@ -71,9 +71,8 @@ flash_mmap(struct file *file, struct vm_
 	if (vma->vm_end - (vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT)) > size)
 		size = vma->vm_end - (vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT));
 
-	pgprot_val(vma->vm_page_prot) &= ~(_PAGE_CACHE);
-	pgprot_val(vma->vm_page_prot) |= _PAGE_E;
 	vma->vm_flags |= (VM_SHM | VM_LOCKED);
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	if (io_remap_pfn_range(vma, vma->vm_start, addr, size, vma->vm_page_prot))
 		return -EAGAIN;
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 996c7b5..07d882b 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1169,11 +1169,6 @@ fb_mmap(struct file *file, struct vm_are
 	vma->vm_pgoff = off >> PAGE_SHIFT;
 	/* This is an IO map - tell maydump to skip this VMA */
 	vma->vm_flags |= VM_IO | VM_RESERVED;
-#if defined(__sparc_v9__)
-	if (io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT,
-				vma->vm_end - vma->vm_start, vma->vm_page_prot))
-		return -EAGAIN;
-#else
 #if defined(__mc68000__)
 #if defined(CONFIG_SUN3)
 	pgprot_val(vma->vm_page_prot) |= SUN3_PAGE_NOCACHE;
@@ -1195,7 +1190,7 @@ fb_mmap(struct file *file, struct vm_are
 #elif defined(__i386__) || defined(__x86_64__)
 	if (boot_cpu_data.x86 > 3)
 		pgprot_val(vma->vm_page_prot) |= _PAGE_PCD;
-#elif defined(__mips__)
+#elif defined(__mips__) || defined(__sparc_v9__)
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 #elif defined(__hppa__)
 	pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
@@ -1212,7 +1207,6 @@ fb_mmap(struct file *file, struct vm_are
 	if (io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT,
 			     vma->vm_end - vma->vm_start, vma->vm_page_prot))
 		return -EAGAIN;
-#endif /* !__sparc_v9__ */
 	return 0;
 #endif /* !sparc32 */
 }
diff --git a/drivers/video/sbuslib.c b/drivers/video/sbuslib.c
index a4d7cc5..34ef859 100644
--- a/drivers/video/sbuslib.c
+++ b/drivers/video/sbuslib.c
@@ -58,6 +58,8 @@ int sbusfb_mmap_helper(struct sbus_mmap_
 	/* To stop the swapper from even considering these pages */
 	vma->vm_flags |= (VM_IO | VM_RESERVED);
 	
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
 	/* Each page, see which map applies */
 	for (page = 0; page < size; ){
 		map_size = 0;
diff --git a/include/asm-sparc/pgtable.h b/include/asm-sparc/pgtable.h
index b33c354..9eea8f4 100644
--- a/include/asm-sparc/pgtable.h
+++ b/include/asm-sparc/pgtable.h
@@ -269,11 +269,14 @@ BTFIXUPDEF_CALL_CONST(pte_t, mk_pte, str
 
 BTFIXUPDEF_CALL_CONST(pte_t, mk_pte_phys, unsigned long, pgprot_t)
 BTFIXUPDEF_CALL_CONST(pte_t, mk_pte_io, unsigned long, pgprot_t, int)
+BTFIXUPDEF_CALL_CONST(pgprot_t, pgprot_noncached, pgprot_t)
 
 #define mk_pte(page,pgprot) BTFIXUP_CALL(mk_pte)(page,pgprot)
 #define mk_pte_phys(page,pgprot) BTFIXUP_CALL(mk_pte_phys)(page,pgprot)
 #define mk_pte_io(page,pgprot,space) BTFIXUP_CALL(mk_pte_io)(page,pgprot,space)
 
+#define pgprot_noncached(pgprot) BTFIXUP_CALL(pgprot_noncached)(pgprot)
+
 BTFIXUPDEF_INT(pte_modify_mask)
 
 static pte_t pte_modify(pte_t pte, pgprot_t newprot) __attribute_const__;
@@ -309,9 +312,6 @@ BTFIXUPDEF_CALL(pte_t *, pte_offset_kern
 #define pte_unmap(pte)		do{}while(0)
 #define pte_unmap_nested(pte)	do{}while(0)
 
-/* The permissions for pgprot_val to make a page mapped on the obio space */
-extern unsigned int pg_iobits;
-
 /* Certain architectures need to do special things when pte's
  * within a page table are directly modified.  Thus, the following
  * hook is made available.

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[Linus Torvalds]

On Fri, 23 Dec 2005, David S. Miller wrote:
>
> Something like this patch below.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/drivers/video/sbuslib.c b/drivers/video/sbuslib.c
> index 646c43f..ac937da 100644
> --- a/drivers/video/sbuslib.c
> +++ b/drivers/video/sbuslib.c
> @@ -46,6 +46,9 @@ int sbusfb_mmap_helper(struct sbus_mmap_
>  	unsigned long off;
>  	int i;
>                                          
> +	if (!(vma->vm_flags & VM_SHARED))
> +		return -EINVAL;
> +

Side note - as I explained to Nick the other week, VM_SHARED really means 
"shared _writable_" mapping, so you're now disallowing a shared read-only 
open too.

Which may be fine, of course. Especially if sbusfb always ends up giving a 
writable pfn-mapping. But I wanted to check that that was what you meant 
to do.

To test for MAP_SHARED, either do the is_cow_mapping() thing, or check 
the VM_MAYSHARE bit.

		Linus

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[Hugh Dickins]

On Sat, 24 Dec 2005, Linus Torvalds wrote:
> On Fri, 23 Dec 2005, David S. Miller wrote:
> >                                          
> > +	if (!(vma->vm_flags & VM_SHARED))
> > +		return -EINVAL;
> > +
> 
> Side note - as I explained to Nick the other week, VM_SHARED really means 
> "shared _writable_" mapping, so you're now disallowing a shared read-only 
> open too.
> 
> Which may be fine, of course. Especially if sbusfb always ends up giving a 
> writable pfn-mapping. But I wanted to check that that was what you meant 
> to do.
> 
> To test for MAP_SHARED, either do the is_cow_mapping() thing, or check 
> the VM_MAYSHARE bit.

I wondered over that too.  Concluded that at the moment it's right for
the test to be on VM_SHARED, because it's secretly giving away shared
write permission.  But in future, once that's been corrected, such checks
would do better to check for VM_MAYSHARE (i.e. reject all MAP_PRIVATE).

Hugh

Re: More info for DSM w/r/t sunffb on 2.6.15-rc6

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Sat, 24 Dec 2005 00:32:26 -0800 (PST)

> On Fri, 23 Dec 2005, David S. Miller wrote:
> > +	if (!(vma->vm_flags & VM_SHARED))
> > +		return -EINVAL;
> > +
> 
> Side note - as I explained to Nick the other week, VM_SHARED really means 
> "shared _writable_" mapping, so you're now disallowing a shared read-only 
> open too.
> 
> Which may be fine, of course. Especially if sbusfb always ends up giving a 
> writable pfn-mapping. But I wanted to check that that was what you meant 
> to do.
> 
> To test for MAP_SHARED, either do the is_cow_mapping() thing, or check 
> the VM_MAYSHARE bit.

I did mean to test for MAP_SHARED, thanks for the test.
I've made the check like this:

diff --git a/drivers/video/sbuslib.c b/drivers/video/sbuslib.c
index 646c43f..3a74a63 100644
--- a/drivers/video/sbuslib.c
+++ b/drivers/video/sbuslib.c
@@ -46,6 +46,9 @@ int sbusfb_mmap_helper(struct sbus_mmap_
 	unsigned long off;
 	int i;
                                         
+	if (!(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
+		return -EINVAL;
+
 	size = vma->vm_end - vma->vm_start;
 	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
 		return -EINVAL;

RE: More info for DSM w/r/t sunffb on 2.6.15-rc6

[Linus Torvalds]

On Fri, 23 Dec 2005, Michael Bishop wrote:
>
>  Linus - I did not try out the memory.c patch, as the subsequent patch 
> from David looked like the simpler solution.

Yes, absolutely.

> Hopefully no other 'insane users' such as myself run into the same 
> problem; I suspect that there would have been a chorus of complaints by 
> now if it was evident in more prominent places. :-)

Note that by "insane users" I didn't mean the end-user sitting in front of 
the machine, but the actual _program_ (ie X) that does uses the kernel in 
insane ways.

So no need to check in at the asylym.

And yes, as X seems to have a sane fallback, we can definitely avoid my 
"allow insanity" patch. Much better to return an error for insane uses 
than to try to support them when there's no need.

Thanks for testing,

		Linus