* Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping)
[not found] <200411252301.iAPN1mo4023046@hera.kernel.org>
@ 2004-11-27 15:54 ` Geert Uytterhoeven
2004-11-30 11:29 ` Marcelo Tosatti
0 siblings, 1 reply; 4+ messages in thread
From: Geert Uytterhoeven @ 2004-11-27 15:54 UTC (permalink / raw)
To: vince, Marcelo Tosatti
Cc: Linux Kernel Development, Linux Frame Buffer Device Development
On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote:
> ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk
>
> [PATCH] vga16fb: Fix frame buffer bad memory mapping
>
> The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to
> the vga card. This is wrong on architectures other than x86, every
> other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct
> memory mapping.
>
> All this patch does is add the mapping macro this has been tested and
> works on ARM systems The driver no longer maps parts of kernel
> workspace and modifies it.
This fix is not correct!
> diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> @@ -142,7 +142,7 @@
> memset(fix, 0, sizeof(struct fb_fix_screeninfo));
> strcpy(fix->id,"VGA16 VGA");
>
> - fix->smem_start = VGA_FB_PHYS;
> + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS);
> fix->smem_len = VGA_FB_PHYS_LEN;
> fix->type = FB_TYPE_VGA_PLANES;
> fix->visual = FB_VISUAL_PSEUDOCOLOR;
fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on
machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either,
because fix->smem_start is supposed to be a CPU _physical_ address, not a
virtual address.
However, this value isn't really used, except by (very rare) userspace that
wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an
incorrect value doesn't really harm.
> @@ -896,7 +896,7 @@
>
> /* XXX share VGA_FB_PHYS region with vgacon */
>
> - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
> + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN);
> if (!vga16fb.video_vbase) {
> printk(KERN_ERR "vga16fb: unable to map device\n");
> return -ENOMEM;
ioremap(): VGA_MAP_MEM() already a _virtual_ address:
| include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
| include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x))
| include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
| include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
| include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x))
| include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base)
| include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
| include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x)
| include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure...
BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because
__ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than
16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail
(before the machine itself died :-(.
Yes, ISA memory space is a mess to do right in a portable way...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping)
2004-11-27 15:54 ` Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) Geert Uytterhoeven
@ 2004-11-30 11:29 ` Marcelo Tosatti
2004-12-01 8:48 ` Geert Uytterhoeven
0 siblings, 1 reply; 4+ messages in thread
From: Marcelo Tosatti @ 2004-11-30 11:29 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: vince, Linux Kernel Development, Linux Frame Buffer Device Development
On Sat, Nov 27, 2004 at 04:54:12PM +0100, Geert Uytterhoeven wrote:
> On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote:
> > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk
> >
> > [PATCH] vga16fb: Fix frame buffer bad memory mapping
> >
> > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to
> > the vga card. This is wrong on architectures other than x86, every
> > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct
> > memory mapping.
> >
> > All this patch does is add the mapping macro this has been tested and
> > works on ARM systems The driver no longer maps parts of kernel
> > workspace and modifies it.
>
> This fix is not correct!
>
> > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> > @@ -142,7 +142,7 @@
> > memset(fix, 0, sizeof(struct fb_fix_screeninfo));
> > strcpy(fix->id,"VGA16 VGA");
> >
> > - fix->smem_start = VGA_FB_PHYS;
> > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS);
> > fix->smem_len = VGA_FB_PHYS_LEN;
> > fix->type = FB_TYPE_VGA_PLANES;
> > fix->visual = FB_VISUAL_PSEUDOCOLOR;
>
> fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on
> machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either,
> because fix->smem_start is supposed to be a CPU _physical_ address, not a
> virtual address.
>
> However, this value isn't really used, except by (very rare) userspace that
> wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an
> incorrect value doesn't really harm.
>
> > @@ -896,7 +896,7 @@
> >
> > /* XXX share VGA_FB_PHYS region with vgacon */
> >
> > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
> > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN);
> > if (!vga16fb.video_vbase) {
> > printk(KERN_ERR "vga16fb: unable to map device\n");
> > return -ENOMEM;
>
> ioremap(): VGA_MAP_MEM() already a _virtual_ address:
>
> | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x))
> | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
> | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x))
> | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base)
> | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x)
> | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
>
> Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure...
>
> BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because
> __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than
> 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail
> (before the machine itself died :-(.
>
> Yes, ISA memory space is a mess to do right in a portable way...
Geert,
I'll guest I'll just revert the whole change then... Do you have a
better suggestion?
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping)
2004-11-30 11:29 ` Marcelo Tosatti
@ 2004-12-01 8:48 ` Geert Uytterhoeven
2004-12-01 10:59 ` Marcelo Tosatti
0 siblings, 1 reply; 4+ messages in thread
From: Geert Uytterhoeven @ 2004-12-01 8:48 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: vince, Linux Kernel Development, Linux Frame Buffer Device Development
On Tue, 30 Nov 2004, Marcelo Tosatti wrote:
> On Sat, Nov 27, 2004 at 04:54:12PM +0100, Geert Uytterhoeven wrote:
> > On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote:
> > > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk
> > >
> > > [PATCH] vga16fb: Fix frame buffer bad memory mapping
> > >
> > > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to
> > > the vga card. This is wrong on architectures other than x86, every
> > > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct
> > > memory mapping.
> > >
> > > All this patch does is add the mapping macro this has been tested and
> > > works on ARM systems The driver no longer maps parts of kernel
> > > workspace and modifies it.
> >
> > This fix is not correct!
> >
> > > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> > > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> > > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> > > @@ -142,7 +142,7 @@
> > > memset(fix, 0, sizeof(struct fb_fix_screeninfo));
> > > strcpy(fix->id,"VGA16 VGA");
> > >
> > > - fix->smem_start = VGA_FB_PHYS;
> > > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS);
> > > fix->smem_len = VGA_FB_PHYS_LEN;
> > > fix->type = FB_TYPE_VGA_PLANES;
> > > fix->visual = FB_VISUAL_PSEUDOCOLOR;
> >
> > fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on
> > machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either,
> > because fix->smem_start is supposed to be a CPU _physical_ address, not a
> > virtual address.
> >
> > However, this value isn't really used, except by (very rare) userspace that
> > wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an
> > incorrect value doesn't really harm.
> >
> > > @@ -896,7 +896,7 @@
> > >
> > > /* XXX share VGA_FB_PHYS region with vgacon */
> > >
> > > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
> > > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN);
> > > if (!vga16fb.video_vbase) {
> > > printk(KERN_ERR "vga16fb: unable to map device\n");
> > > return -ENOMEM;
> >
> > ioremap(): VGA_MAP_MEM() already a _virtual_ address:
> >
> > | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> > | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x))
> > | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
> > | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> > | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x))
> > | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base)
> > | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> > | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x)
> > | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
> >
> > Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure...
> >
> > BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because
> > __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than
> > 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail
> > (before the machine itself died :-(.
> >
> > Yes, ISA memory space is a mess to do right in a portable way...
>
> Geert,
>
> I'll guest I'll just revert the whole change then... Do you have a
Yes, please.
> better suggestion?
Replacing
vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
by
vga16fb.video_vbase = VGA_MAP_MEM(VGA_FB_PHYS);
will probably work, since that's what vgacon does, but I'd like to see it
tested first anyway.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping)
2004-12-01 8:48 ` Geert Uytterhoeven
@ 2004-12-01 10:59 ` Marcelo Tosatti
0 siblings, 0 replies; 4+ messages in thread
From: Marcelo Tosatti @ 2004-12-01 10:59 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: vince, Linux Kernel Development, Linux Frame Buffer Device Development
On Wed, Dec 01, 2004 at 09:48:29AM +0100, Geert Uytterhoeven wrote:
> On Tue, 30 Nov 2004, Marcelo Tosatti wrote:
> > On Sat, Nov 27, 2004 at 04:54:12PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, 25 Nov 2004, Linux Kernel Mailing List wrote:
> > > > ChangeSet 1.1543, 2004/11/25 13:16:49-02:00, vince@arm.linux.org.uk
> > > >
> > > > [PATCH] vga16fb: Fix frame buffer bad memory mapping
> > > >
> > > > The vga16fb driver uses a direct ioremap on 0xa00000 to gain access to
> > > > the vga card. This is wrong on architectures other than x86, every
> > > > other driver uses VGA_MAP_MEM macro from vga.h to ensure the correct
> > > > memory mapping.
> > > >
> > > > All this patch does is add the mapping macro this has been tested and
> > > > works on ARM systems The driver no longer maps parts of kernel
> > > > workspace and modifies it.
> > >
> > > This fix is not correct!
> > >
> > > > diff -Nru a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> > > > --- a/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> > > > +++ b/drivers/video/vga16fb.c 2004-11-25 15:01:51 -08:00
> > > > @@ -142,7 +142,7 @@
> > > > memset(fix, 0, sizeof(struct fb_fix_screeninfo));
> > > > strcpy(fix->id,"VGA16 VGA");
> > > >
> > > > - fix->smem_start = VGA_FB_PHYS;
> > > > + fix->smem_start = VGA_MAP_MEM(VGA_FB_PHYS);
> > > > fix->smem_len = VGA_FB_PHYS_LEN;
> > > > fix->type = FB_TYPE_VGA_PLANES;
> > > > fix->visual = FB_VISUAL_PSEUDOCOLOR;
> > >
> > > fix->smem_start: Although I agree VGA_FB_PHYS is not the correct value on
> > > machines other than PC, VGA_MAP_MEM(VGA_FB_PHYS) is not appropriate either,
> > > because fix->smem_start is supposed to be a CPU _physical_ address, not a
> > > virtual address.
> > >
> > > However, this value isn't really used, except by (very rare) userspace that
> > > wants to mmap the frame buffer through /dev/mem instead of /dev/fb*, so an
> > > incorrect value doesn't really harm.
> > >
> > > > @@ -896,7 +896,7 @@
> > > >
> > > > /* XXX share VGA_FB_PHYS region with vgacon */
> > > >
> > > > - vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
> > > > + vga16fb.video_vbase = ioremap(VGA_MAP_MEM(VGA_FB_PHYS), VGA_FB_PHYS_LEN);
> > > > if (!vga16fb.video_vbase) {
> > > > printk(KERN_ERR "vga16fb: unable to map device\n");
> > > > return -ENOMEM;
> > >
> > > ioremap(): VGA_MAP_MEM() already a _virtual_ address:
> > >
> > > | include/asm-alpha/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> > > | include/asm-arm/vga.h:#define VGA_MAP_MEM(x) (PCIMEM_BASE + (x))
> > > | include/asm-i386/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
> > > | include/asm-ia64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> > > | include/asm-mips/vga.h:#define VGA_MAP_MEM(x) ((unsigned long)0xb0000000 + (unsigned long)(x))
> > > | include/asm-ppc/vga.h:#define VGA_MAP_MEM(x) (x + vgacon_remap_base)
> > > | include/asm-ppc64/vga.h:#define VGA_MAP_MEM(x) ((unsigned long) ioremap((x), 0))
> > > | include/asm-sparc64/vga.h:#define VGA_MAP_MEM(x) (x)
> > > | include/asm-x86_64/vga.h:#define VGA_MAP_MEM(x) (unsigned long)phys_to_virt(x)
> > >
> > > Doing a double ioremap(), or ioremap(phys_to_virt()) will break for sure...
> > >
> > > BTW, on (PReP/CHRP) PPC ioremap() on ISA memory space `just works' because
> > > __ioremap() adds _ISA_MEM_BASE to the passed pointer if it's smaller than
> > > 16*1024*1024. And yes, vga16fb used to work fine on my CHRP LongTrail
> > > (before the machine itself died :-(.
> > >
> > > Yes, ISA memory space is a mess to do right in a portable way...
> >
> > Geert,
> >
> > I'll guest I'll just revert the whole change then... Do you have a
>
> Yes, please.
Done.
>
> > better suggestion?
>
> Replacing
>
> vga16fb.video_vbase = ioremap(VGA_FB_PHYS, VGA_FB_PHYS_LEN);
>
> by
>
> vga16fb.video_vbase = VGA_MAP_MEM(VGA_FB_PHYS);
>
> will probably work, since that's what vgacon does, but I'd like to see it
> tested first anyway.
I think Andrew merged a similar patch into v2.6 months ago.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2004-12-01 17:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <200411252301.iAPN1mo4023046@hera.kernel.org>
2004-11-27 15:54 ` Linux 2.4.29-pre1 (was: Re: [PATCH] vga16fb: Fix frame buffer bad memory mapping) Geert Uytterhoeven
2004-11-30 11:29 ` Marcelo Tosatti
2004-12-01 8:48 ` Geert Uytterhoeven
2004-12-01 10:59 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).