8250_hp300: initialisation ordering bug

8250_hp300: initialisation ordering bug

[Russell King]

Hi,

I've noticed that 8250_hp300 is buggy wrt the ordering of hardware
initialisation to the visibility of devices to user space.  Namely,
8250_hp300 does the following:

        line = serial8250_register_port(&port);
...
        /* Enable board-interrupts */
        out_8(d->resource.start + DIO_VIRADDRBASE + DCA_IC, DCA_IC_IE);
        dio_set_drvdata(d, (void *)line);

        /* Reset the DCA */
        out_8(d->resource.start + DIO_VIRADDRBASE + DCA_ID, 0xff);
        udelay(100);

serial8250_register_port() makes the port visible to userspace, so
from that point on it could be opened.  However, if it's opened
prior to the remainder of the above completing, we will be missing
interrupts (and what effect does "reset the DCA" have?)

Surely this hardware fiddling should be completed before we register
the port?


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 8250_hp300: initialisation ordering bug

[Kars de Jong]

On zo, 2005-09-04 at 11:19 +0100, Russell King wrote:
> Hi,
> 
> I've noticed that 8250_hp300 is buggy wrt the ordering of hardware
> initialisation to the visibility of devices to user space.  Namely,
> 8250_hp300 does the following:
> 
> ...

> serial8250_register_port() makes the port visible to userspace, so
> from that point on it could be opened.  However, if it's opened
> prior to the remainder of the above completing, we will be missing
> interrupts (and what effect does "reset the DCA" have?)
> 
> Surely this hardware fiddling should be completed before we register
> the port?

Yes, you are right. I am working on rewriting the driver a bit to use a
platform device for the APCI driver, I'll take your bug report into
account as well.

On a related note: can I use the "serial8250" platform driver also for
non-ISA devices (like my APCI platform device)? The comments in
drivers/serial/8250.c suggest it's for ISA devices only, but I don't see
a particular reason for not using it for my APCI devices.


Kind regards,

Kars.

Re: 8250_hp300: initialisation ordering bug

[Russell King]

On Wed, Sep 07, 2005 at 10:17:49PM +0200, Kars de Jong wrote:
> Yes, you are right. I am working on rewriting the driver a bit to use a
> platform device for the APCI driver, I'll take your bug report into
> account as well.

Thanks.

> On a related note: can I use the "serial8250" platform driver also for
> non-ISA devices (like my APCI platform device)? The comments in
> drivers/serial/8250.c suggest it's for ISA devices only, but I don't see
> a particular reason for not using it for my APCI devices.

The legacy platform device (serial8250_isa_devs) is for the old
legacy ISA tables, found in include/asm-*/serial.h.

Other serial8250 platform devices can be used to register other
devices - preferably groups of platform specific serial ports.

However, if you're talking about registering a set of devices
found on a different bus type (eg, PCI) then look at how 8250_pci
handles that.  I'd prefer bus-specific device registration to be
done in a similar way to 8250_pci rather than creating extra
platform devices.

I hope that's clear.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 8250_hp300: initialisation ordering bug

[Kars de Jong]

On wo, 2005-09-07 at 21:33 +0100, Russell King wrote:
> On Wed, Sep 07, 2005 at 10:17:49PM +0200, Kars de Jong wrote:
> > On a related note: can I use the "serial8250" platform driver also for
> > non-ISA devices (like my APCI platform device)? The comments in
> > drivers/serial/8250.c suggest it's for ISA devices only, but I don't see
> > a particular reason for not using it for my APCI devices.
> 
> The legacy platform device (serial8250_isa_devs) is for the old
> legacy ISA tables, found in include/asm-*/serial.h.

Right. My machine has an ISA bus too, but no support for it yet :)

> Other serial8250 platform devices can be used to register other
> devices - preferably groups of platform specific serial ports.
> 
> However, if you're talking about registering a set of devices
> found on a different bus type (eg, PCI) then look at how 8250_pci
> handles that.  I'd prefer bus-specific device registration to be
> done in a similar way to 8250_pci rather than creating extra
> platform devices.

Yes, I do this for the DCA ports (which are on the DIO bus).

However, that doesn't seem to be appropriate for the APCI devices which
are on the motherboard. So I'll use the platform driver for those. Do
you have any suggestion for the id value I should use in my platform
device? I looked in the existing drivers and ids -1 and 1-5 are taken,
any reason id 0 was not used?


Kind regards,

Kars.

Re: 8250_hp300: initialisation ordering bug

[Russell King]

On Thu, Sep 08, 2005 at 09:42:12PM +0200, Kars de Jong wrote:
> However, that doesn't seem to be appropriate for the APCI devices which
> are on the motherboard. So I'll use the platform driver for those. Do
> you have any suggestion for the id value I should use in my platform
> device? I looked in the existing drivers and ids -1 and 1-5 are taken,
> any reason id 0 was not used?

You might be interested in this patch which sorts out the numeric
ID allocation.

diff --git a/arch/arm/mach-clps7500/core.c b/arch/arm/mach-clps7500/core.c
--- a/arch/arm/mach-clps7500/core.c
+++ b/arch/arm/mach-clps7500/core.c
@@ -354,7 +354,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-ebsa110/core.c b/arch/arm/mach-ebsa110/core.c
--- a/arch/arm/mach-ebsa110/core.c
+++ b/arch/arm/mach-ebsa110/core.c
@@ -219,7 +219,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-epxa10db/arch.c b/arch/arm/mach-epxa10db/arch.c
--- a/arch/arm/mach-epxa10db/arch.c
+++ b/arch/arm/mach-epxa10db/arch.c
@@ -52,7 +52,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-footbridge/isa.c b/arch/arm/mach-footbridge/isa.c
--- a/arch/arm/mach-footbridge/isa.c
+++ b/arch/arm/mach-footbridge/isa.c
@@ -34,7 +34,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-h720x/cpu-h7202.c b/arch/arm/mach-h720x/cpu-h7202.c
--- a/arch/arm/mach-h720x/cpu-h7202.c
+++ b/arch/arm/mach-h720x/cpu-h7202.c
@@ -90,7 +90,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-ixp2000/core.c b/arch/arm/mach-ixp2000/core.c
--- a/arch/arm/mach-ixp2000/core.c
+++ b/arch/arm/mach-ixp2000/core.c
@@ -174,7 +174,7 @@ static struct resource ixp2000_uart_reso
 
 static struct platform_device ixp2000_serial_device = {
 	.name		= "serial8250",
-	.id		= 0,
+	.id		= PLAT8250_DEV_PLATFORM,
 	.dev		= {
 		.platform_data		= ixp2000_serial_port,
 	},
diff --git a/arch/arm/mach-ixp4xx/coyote-setup.c b/arch/arm/mach-ixp4xx/coyote-setup.c
--- a/arch/arm/mach-ixp4xx/coyote-setup.c
+++ b/arch/arm/mach-ixp4xx/coyote-setup.c
@@ -66,7 +66,7 @@ static struct plat_serial8250_port coyot
 
 static struct platform_device coyote_uart = {
 	.name		= "serial8250",
-	.id		= 0,
+	.id		= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= coyote_uart_data,
 	},
diff --git a/arch/arm/mach-ixp4xx/gtwx5715-setup.c b/arch/arm/mach-ixp4xx/gtwx5715-setup.c
--- a/arch/arm/mach-ixp4xx/gtwx5715-setup.c
+++ b/arch/arm/mach-ixp4xx/gtwx5715-setup.c
@@ -93,7 +93,7 @@ static struct plat_serial8250_port gtwx5
 
 static struct platform_device gtwx5715_uart_device = {
 	.name		= "serial8250",
-	.id		= 0,
+	.id		= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= gtwx5715_uart_platform_data,
 	},
diff --git a/arch/arm/mach-ixp4xx/ixdp425-setup.c b/arch/arm/mach-ixp4xx/ixdp425-setup.c
--- a/arch/arm/mach-ixp4xx/ixdp425-setup.c
+++ b/arch/arm/mach-ixp4xx/ixdp425-setup.c
@@ -96,7 +96,7 @@ static struct plat_serial8250_port ixdp4
 
 static struct platform_device ixdp425_uart = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev.platform_data	= ixdp425_uart_data,
 	.num_resources		= 2,
 	.resource		= ixdp425_uart_resources
diff --git a/arch/arm/mach-omap1/board-voiceblue.c b/arch/arm/mach-omap1/board-voiceblue.c
--- a/arch/arm/mach-omap1/board-voiceblue.c
+++ b/arch/arm/mach-omap1/board-voiceblue.c
@@ -74,7 +74,7 @@ static struct plat_serial8250_port voice
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 1,
+	.id			= PLAT8250_DEV_PLATFORM1,
 	.dev			= {
 		.platform_data	= voiceblue_ports,
 	},
diff --git a/arch/arm/mach-omap1/serial.c b/arch/arm/mach-omap1/serial.c
--- a/arch/arm/mach-omap1/serial.c
+++ b/arch/arm/mach-omap1/serial.c
@@ -94,7 +94,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-rpc/riscpc.c b/arch/arm/mach-rpc/riscpc.c
--- a/arch/arm/mach-rpc/riscpc.c
+++ b/arch/arm/mach-rpc/riscpc.c
@@ -140,7 +140,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-s3c2410/mach-bast.c b/arch/arm/mach-s3c2410/mach-bast.c
--- a/arch/arm/mach-s3c2410/mach-bast.c
+++ b/arch/arm/mach-s3c2410/mach-bast.c
@@ -381,7 +381,7 @@ static struct plat_serial8250_port bast_
 
 static struct platform_device bast_sio = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= &bast_sio_data,
 	},
diff --git a/arch/arm/mach-s3c2410/mach-vr1000.c b/arch/arm/mach-s3c2410/mach-vr1000.c
--- a/arch/arm/mach-s3c2410/mach-vr1000.c
+++ b/arch/arm/mach-s3c2410/mach-vr1000.c
@@ -221,7 +221,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-shark/core.c b/arch/arm/mach-shark/core.c
--- a/arch/arm/mach-shark/core.c
+++ b/arch/arm/mach-shark/core.c
@@ -41,7 +41,7 @@ static struct plat_serial8250_port seria
 
 static struct platform_device serial_device = {
 	.name			= "serial8250",
-	.id			= 0,
+	.id			= PLAT8250_DEV_PLATFORM,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/ppc/syslib/mpc10x_common.c b/arch/ppc/syslib/mpc10x_common.c
--- a/arch/ppc/syslib/mpc10x_common.c
+++ b/arch/ppc/syslib/mpc10x_common.c
@@ -140,12 +140,12 @@ struct platform_device ppc_sys_platform_
 	},
 	[MPC10X_UART0] = {
 		.name = "serial8250",
-		.id	= 0,
+		.id	= PLAT8250_DEV_PLATFORM,
 		.dev.platform_data = serial_plat_uart0,
 	},
 	[MPC10X_UART1] = {
 		.name = "serial8250",
-		.id	= 1,
+		.id	= PLAT8250_DEV_PLATFORM1,
 		.dev.platform_data = serial_plat_uart1,
 	},
 
diff --git a/arch/ppc/syslib/mpc83xx_devices.c b/arch/ppc/syslib/mpc83xx_devices.c
--- a/arch/ppc/syslib/mpc83xx_devices.c
+++ b/arch/ppc/syslib/mpc83xx_devices.c
@@ -165,7 +165,7 @@ struct platform_device ppc_sys_platform_
 	},
 	[MPC83xx_DUART] = {
 		.name = "serial8250",
-		.id	= 0,
+		.id	= PLAT8250_DEV_PLATFORM,
 		.dev.platform_data = serial_platform_data,
 	},
 	[MPC83xx_SEC2] = {
diff --git a/arch/ppc/syslib/mpc85xx_devices.c b/arch/ppc/syslib/mpc85xx_devices.c
--- a/arch/ppc/syslib/mpc85xx_devices.c
+++ b/arch/ppc/syslib/mpc85xx_devices.c
@@ -282,7 +282,7 @@ struct platform_device ppc_sys_platform_
 	},
 	[MPC85xx_DUART] = {
 		.name = "serial8250",
-		.id	= 0,
+		.id	= PLAT8250_DEV_PLATFORM,
 		.dev.platform_data = serial_platform_data,
 	},
 	[MPC85xx_PERFMON] = {
diff --git a/arch/ppc64/kernel/setup.c b/arch/ppc64/kernel/setup.c
--- a/arch/ppc64/kernel/setup.c
+++ b/arch/ppc64/kernel/setup.c
@@ -1283,7 +1283,7 @@ void __init generic_find_legacy_serial_p
 
 static struct platform_device serial_device = {
 	.name	= "serial8250",
-	.id	= 0,
+	.id	= PLAT8250_DEV_PLATFORM,
 	.dev	= {
 		.platform_data = serial_ports,
 	},
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2536,7 +2536,7 @@ static int __init serial8250_init(void)
 		goto out;
 
 	serial8250_isa_devs = platform_device_register_simple("serial8250",
-							      -1, NULL, 0);
+					 PLAT8250_DEV_LEGACY, NULL, 0);
 	if (IS_ERR(serial8250_isa_devs)) {
 		ret = PTR_ERR(serial8250_isa_devs);
 		goto unreg;
diff --git a/drivers/serial/8250_accent.c b/drivers/serial/8250_accent.c
--- a/drivers/serial/8250_accent.c
+++ b/drivers/serial/8250_accent.c
@@ -29,7 +29,7 @@ static struct plat_serial8250_port accen
 
 static struct platform_device accent_device = {
 	.name			= "serial8250",
-	.id			= 2,
+	.id			= PLAT8250_DEV_ACCENT,
 	.dev			= {
 		.platform_data	= accent_data,
 	},
diff --git a/drivers/serial/8250_boca.c b/drivers/serial/8250_boca.c
--- a/drivers/serial/8250_boca.c
+++ b/drivers/serial/8250_boca.c
@@ -43,7 +43,7 @@ static struct plat_serial8250_port boca_
 
 static struct platform_device boca_device = {
 	.name			= "serial8250",
-	.id			= 3,
+	.id			= PLAT8250_DEV_BOCA,
 	.dev			= {
 		.platform_data	= boca_data,
 	},
diff --git a/drivers/serial/8250_fourport.c b/drivers/serial/8250_fourport.c
--- a/drivers/serial/8250_fourport.c
+++ b/drivers/serial/8250_fourport.c
@@ -35,7 +35,7 @@ static struct plat_serial8250_port fourp
 
 static struct platform_device fourport_device = {
 	.name			= "serial8250",
-	.id			= 1,
+	.id			= PLAT8250_DEV_FOURPORT,
 	.dev			= {
 		.platform_data	= fourport_data,
 	},
diff --git a/drivers/serial/8250_hub6.c b/drivers/serial/8250_hub6.c
--- a/drivers/serial/8250_hub6.c
+++ b/drivers/serial/8250_hub6.c
@@ -40,7 +40,7 @@ static struct plat_serial8250_port hub6_
 
 static struct platform_device hub6_device = {
 	.name			= "serial8250",
-	.id			= 4,
+	.id			= PLAT8250_DEV_HUB6,
 	.dev			= {
 		.platform_data	= hub6_data,
 	},
diff --git a/drivers/serial/8250_mca.c b/drivers/serial/8250_mca.c
--- a/drivers/serial/8250_mca.c
+++ b/drivers/serial/8250_mca.c
@@ -44,7 +44,7 @@ static struct plat_serial8250_port mca_d
 
 static struct platform_device mca_device = {
 	.name			= "serial8250",
-	.id			= 5,
+	.id			= PLAT8250_DEV_MCA,
 	.dev			= {
 		.platform_data	= mca_data,
 	},
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -30,6 +30,21 @@ struct plat_serial8250_port {
 };
 
 /*
+ * Allocate 8250 platform device IDs.  Nothing is implied by
+ * the numbering here, except for the legacy entry being -1.
+ */
+enum {
+	PLAT8250_DEV_LEGACY = -1,
+	PLAT8250_DEV_PLATFORM,
+	PLAT8250_DEV_PLATFORM1,
+	PLAT8250_DEV_FOURPORT,
+	PLAT8250_DEV_ACCENT,
+	PLAT8250_DEV_BOCA,
+	PLAT8250_DEV_HUB6,
+	PLAT8250_DEV_MCA,
+};
+
+/*
  * This should be used by drivers which want to register
  * their own 8250 ports without registering their own
  * platform device.  Using these will make your driver


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core