Re: [PATCH 7/7] ALPHA: support graphics on non-zero PCI domains

[Ivan Kokshaysky <ink@jurassic.park.msu.ru>]

On Wed, Apr 11, 2007 at 01:32:19PM -0400, Jay Estabrook wrote:
> Yes, it does leak, and yes, it should be kmalloced. Something like this?
> 
>         struct resource *new_vga;
> 
>         new_vga = kmalloc(sizeof(struct resource), GFP_ATOMIC);
>         if (new_vga) {
>             *new_vga = alpha_vga;
>             new_vga->start += pci_vga_hose->io_space->start;
>             new_vga->end   += pci_vga_hose->io_space->start;
>             request_resource(&ioport_resource, new_vga);
>         }

No. After closer inspection this looks like a dead code.
This request_resource() always fails because the parent resource
passed to it is incorrect - it must be pci_vga_hose->io_space,
not ioport_resource.
On the other hand, I don't see why we should request VGA IO from
VGA_MAP_MEM, which might be called multiple times. Wouldn't it be
much cleaner to do it once right after VGA discovery?
I'd suggest the appended patch on top of your original one.

Ivan.

--- orig/arch/alpha/kernel/console.c	Tue Apr 10 12:26:12 2007
+++ linux/arch/alpha/kernel/console.c	Wed Apr 11 22:09:56 2007
@@ -18,6 +18,11 @@
 #ifdef CONFIG_VGA_HOSE
 
 struct pci_controller *pci_vga_hose = NULL;
+static struct resource alpha_vga = {
+	.name	= "alpha-vga+",
+	.start	= 0x3C0,
+	.end	= 0x3DF
+};
 
 static struct pci_controller * __init 
 default_vga_hose_select(struct pci_controller *h1, struct pci_controller *h2)
@@ -48,6 +53,11 @@ locate_and_init_vga(void *(*sel_func)(vo
 	/* Did we already initialize the correct one? Is there one? */
 	if (!hose || (conswitchp == &vga_con && pci_vga_hose == hose))
 		return;
+
+	/* Create a new VGA ioport resource WRT the hose it is on. */
+	alpha_vga.start += hose->io_space->start;
+	alpha_vga.end += hose->io_space->start;
+	request_resource(hose->io_space, &alpha_vga);
 
 	/* Set the VGA hose and init the new console. */
 	pci_vga_hose = hose;
--- orig/include/asm-alpha/vga.h	Tue Apr 10 12:26:12 2007
+++ linux/include/asm-alpha/vga.h	Wed Apr 11 22:07:17 2007
@@ -52,24 +52,6 @@ extern void scr_memcpyw(u16 *d, const u1
 
 extern struct pci_controller *pci_vga_hose;
  
-static inline unsigned long
-VGA_MAP_MEM(unsigned long xaddr, unsigned int size)
-{
-	/* Create a new VGA ioport resource WRT the hose it is on. */
-	if (pci_vga_hose && pci_vga_hose->index) {
-		static struct resource alpha_vga =
-		  { .name = "alpha-vga+", .start = 0x3C0, .end = 0x3DF };
-		struct resource new_vga = alpha_vga;
-
-		new_vga.start += pci_vga_hose->io_space->start;
-		new_vga.end   += pci_vga_hose->io_space->start;
-		request_resource(&ioport_resource, &new_vga);
-	}
-
-	/*  Return the ioremap. */
-	return (unsigned long) ioremap(xaddr, size);
-}
-
 # define __is_port_vga(a)       \
 	(((a) >= 0x3b0) && ((a) < 0x3e0) && \
 	 ((a) != 0x3b3) && ((a) != 0x3d3))
@@ -88,10 +70,11 @@ VGA_MAP_MEM(unsigned long xaddr, unsigne
  } while(0)
 
 #else /* CONFIG_VGA_HOSE */
-# define VGA_MAP_MEM(x,s)	((unsigned long) ioremap(x, s))
 # define pci_vga_hose 0
 # define FIXUP_IOADDR_VGA(a)
 # define FIXUP_MEMADDR_VGA(a)
 #endif /* CONFIG_VGA_HOSE */
+
+#define VGA_MAP_MEM(x,s)	((unsigned long) ioremap(x, s))
 
 #endif
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/