2004-04-20 22:20:39

by Tony Breeds

[permalink] [raw]
Subject: [PATCH] Kconfig dependancy update for drivers/misc/ibmasm


Hello,
Some weeks ago I saw this compile error posted to lkml:

---
> LD .tmp_vmlinux1
> drivers/built-in.o(.text+0x435e1): In function `ibmasm_register_uart':
> : undefined reference to `register_serial'
> drivers/built-in.o(.text+0x43649): In function `ibmasm_unregister_uart':
> : undefined reference to `unregister_serial'
> make: *** [.tmp_vmlinux1] Error 1
> summer@Dolphin:~/pebble/kernel/linux-2.6.4$
---

This was created because ibmasm was set to yes BUT the 8250 was a
module. I believe the correct (tested) fix is below.

################################################################################
--- 2.6.4.clean/drivers/misc/Kconfig 2004-03-11 17:57:23.000000000 +1100
+++ 2.6.4.noconfig/drivers/misc/Kconfig 2004-03-30 09:32:07.000000000 +1000
@@ -6,7 +6,7 @@

config IBM_ASM
tristate "Device driver for IBM RSA service processor"
- depends on X86
+ depends on X86 && SERIAL_8250
default n
---help---
This option enables device driver support for in-band access to the
################################################################################

Yours Tony

linux.conf.au http://lca2005.linux.org.au/
Apr 18-23 2005 The Australian Linux Technical Conference!


2004-04-20 22:27:49

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] Kconfig dependancy update for drivers/misc/ibmasm

Tony Breeds <[email protected]> wrote:
>
>
> Hello,
> Some weeks ago I saw this compile error posted to lkml:
>
> ---
> > LD .tmp_vmlinux1
> > drivers/built-in.o(.text+0x435e1): In function `ibmasm_register_uart':
> > : undefined reference to `register_serial'
> > drivers/built-in.o(.text+0x43649): In function `ibmasm_unregister_uart':
> > : undefined reference to `unregister_serial'
> > make: *** [.tmp_vmlinux1] Error 1
> > summer@Dolphin:~/pebble/kernel/linux-2.6.4$
> ---
>
> This was created because ibmasm was set to yes BUT the 8250 was a
> module. I believe the correct (tested) fix is below.

Seems sane to me, but I'm not sure why this wasn't done originally. ie, this:

+#ifdef CONFIG_SERIAL_8250
extern void ibmasm_register_uart(struct service_processor *sp);
extern void ibmasm_unregister_uart(struct service_processor *sp);
+#else
+#define ibmasm_register_uart(sp) do { } while(0)
+#define ibmasm_unregister_uart(sp) do { } while(0)
+#endif

becomes unnecessary with your patch.

Max, any preferences?


> ################################################################################
> --- 2.6.4.clean/drivers/misc/Kconfig 2004-03-11 17:57:23.000000000 +1100
> +++ 2.6.4.noconfig/drivers/misc/Kconfig 2004-03-30 09:32:07.000000000 +1000
> @@ -6,7 +6,7 @@
>
> config IBM_ASM
> tristate "Device driver for IBM RSA service processor"
> - depends on X86
> + depends on X86 && SERIAL_8250
> default n
> ---help---
> This option enables device driver support for in-band access to the
> ################################################################################
>
> Yours Tony
>
> linux.conf.au http://lca2005.linux.org.au/
> Apr 18-23 2005 The Australian Linux Technical Conference!
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
> ----- End forwarded message -----
>
> Yours Tony
>
> linux.conf.au http://lca2005.linux.org.au/
> Apr 18-23 2005 The Australian Linux Technical Conference!

2004-04-20 22:53:29

by Max Asbock

[permalink] [raw]
Subject: Re: [PATCH] Kconfig dependancy update for drivers/misc/ibmasm

On Tue, 2004-04-20 at 14:34, Andrew Morton wrote:
> Tony Breeds <[email protected]> wrote:
>
> Seems sane to me, but I'm not sure why this wasn't done originally. ie, this:
>
> +#ifdef CONFIG_SERIAL_8250
> extern void ibmasm_register_uart(struct service_processor *sp);
> extern void ibmasm_unregister_uart(struct service_processor *sp);
> +#else
> +#define ibmasm_register_uart(sp) do { } while(0)
> +#define ibmasm_unregister_uart(sp) do { } while(0)
> +#endif
>
> becomes unnecessary with your patch.
>
> Max, any preferences?
>

The above allows the driver to be built without serial line support. It
still functions that way. uart support is only part of the driver's
functions. Therefore it makes sense to not make the whole driver depend
on SERIAL_8250 and instead only configure away the uart support when
SERIAL_8250 is not defined.

regards,
max


>
> > ################################################################################
> > --- 2.6.4.clean/drivers/misc/Kconfig 2004-03-11 17:57:23.000000000 +1100
> > +++ 2.6.4.noconfig/drivers/misc/Kconfig 2004-03-30 09:32:07.000000000 +1000
> > @@ -6,7 +6,7 @@
> >
> > config IBM_ASM
> > tristate "Device driver for IBM RSA service processor"
> > - depends on X86
> > + depends on X86 && SERIAL_8250
> > default n
> > ---help---
> > This option enables device driver support for in-band access to the


2004-04-20 22:16:53

by Tony Breeds

[permalink] [raw]
Subject: Re: [PATCH] Kconfig dependancy update for drivers/misc/ibmasm

On Tue, Apr 20, 2004 at 02:34:18PM -0700, Andrew Morton wrote:

> Seems sane to me, but I'm not sure why this wasn't done originally. ie, this:
>
> +#ifdef CONFIG_SERIAL_8250
> extern void ibmasm_register_uart(struct service_processor *sp);
> extern void ibmasm_unregister_uart(struct service_processor *sp);
> +#else
> +#define ibmasm_register_uart(sp) do { } while(0)
> +#define ibmasm_unregister_uart(sp) do { } while(0)
> +#endif
>
> becomes unnecessary with your patch.
>
> Max, any preferences?

If I read this correctly the above patch would mean that ibmasm can be
built regardless of the value of SERIAL_8250 BUT my patch means it can
only be built if SERIAL_8250 is also being built (regardless of state).

Can the device operate correctly without the uart? If so then my patch
is bogus.

Yours Tony

linux.conf.au http://lca2005.linux.org.au/
Apr 18-23 2005 The Australian Linux Technical Conference!

2004-04-20 23:41:13

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] Kconfig dependancy update for drivers/misc/ibmasm

Max Asbock <[email protected]> wrote:
>
> On Tue, 2004-04-20 at 14:34, Andrew Morton wrote:
> > Tony Breeds <[email protected]> wrote:
> >
> > Seems sane to me, but I'm not sure why this wasn't done originally. ie, this:
> >
> > +#ifdef CONFIG_SERIAL_8250
> > extern void ibmasm_register_uart(struct service_processor *sp);
> > extern void ibmasm_unregister_uart(struct service_processor *sp);
> > +#else
> > +#define ibmasm_register_uart(sp) do { } while(0)
> > +#define ibmasm_unregister_uart(sp) do { } while(0)
> > +#endif
> >
> > becomes unnecessary with your patch.
> >
> > Max, any preferences?
> >
>
> The above allows the driver to be built without serial line support. It
> still functions that way. uart support is only part of the driver's
> functions. Therefore it makes sense to not make the whole driver depend
> on SERIAL_8250 and instead only configure away the uart support when
> SERIAL_8250 is not defined.

So I think you'll be needing something liek this?


25-akpm/drivers/misc/ibmasm/uart.c | 4 ++++
1 files changed, 4 insertions(+)

diff -puN drivers/misc/ibmasm/uart.c~a drivers/misc/ibmasm/uart.c
--- 25/drivers/misc/ibmasm/uart.c~a Tue Apr 20 16:41:32 2004
+++ 25-akpm/drivers/misc/ibmasm/uart.c Tue Apr 20 16:41:56 2004
@@ -54,12 +54,14 @@ void ibmasm_register_uart(struct service
serial.io_type = UPIO_MEM;
serial.iomem_base = iomem_base;

+#ifdef CONFIG_SERIAL_8250
sp->serial_line = register_serial(&serial);
if (sp->serial_line < 0) {
dev_err(sp->dev, "Failed to register serial port\n");
return;
}
enable_uart_interrupts(sp->base_address);
+#endif
}

void ibmasm_unregister_uart(struct service_processor *sp)
@@ -68,5 +70,7 @@ void ibmasm_unregister_uart(struct servi
return;

disable_uart_interrupts(sp->base_address);
+#ifdef CONFIG_SERIAL_8250
unregister_serial(sp->serial_line);
+#endif
}

_

2004-04-21 00:18:02

by Max Asbock

[permalink] [raw]
Subject: Re: [PATCH] Kconfig dependancy update for drivers/misc/ibmasm

On Tue, 2004-04-20 at 16:42, Andrew Morton wrote:

> Max Asbock <[email protected]> wrote:

> > ... allows the driver to be built without serial line support. It
> > still functions that way. uart support is only part of the driver's
> > functions. Therefore it makes sense to not make the whole driver depend
> > on SERIAL_8250 and instead only configure away the uart support when
> > SERIAL_8250 is not defined.


> So I think you'll be needing something liek this?
>
>
> 25-akpm/drivers/misc/ibmasm/uart.c | 4 ++++
> 1 files changed, 4 insertions(+)
>
> diff -puN drivers/misc/ibmasm/uart.c~a drivers/misc/ibmasm/uart.c
> --- 25/drivers/misc/ibmasm/uart.c~a Tue Apr 20 16:41:32 2004
> +++ 25-akpm/drivers/misc/ibmasm/uart.c Tue Apr 20 16:41:56 2004
> @@ -54,12 +54,14 @@ void ibmasm_register_uart(struct service
> serial.io_type = UPIO_MEM;
> serial.iomem_base = iomem_base;
>
> +#ifdef CONFIG_SERIAL_8250
> sp->serial_line = register_serial(&serial);
> if (sp->serial_line < 0) {
> dev_err(sp->dev, "Failed to register serial port\n");
> return;
> }
> enable_uart_interrupts(sp->base_address);
> +#endif
> }
>
> void ibmasm_unregister_uart(struct service_processor *sp)
> @@ -68,5 +70,7 @@ void ibmasm_unregister_uart(struct servi
> return;
>
> disable_uart_interrupts(sp->base_address);
> +#ifdef CONFIG_SERIAL_8250
> unregister_serial(sp->serial_line);
> +#endif
> }
>

I posted the following patch a little while ago. I arranged it that way
with the intention to avoid #ifdefs in the .c file. This patch has been
applied in 2.6.6-rc2. So it is all good.

regards,
max

diff -urN linux-2.6.5/drivers/misc/ibmasm/ibmasm.h linux-2.6.5-ibmasm/drivers/misc/ibmasm/ibmasm.h
--- linux-2.6.5/drivers/misc/ibmasm/ibmasm.h 2004-04-03 19:36:18.000000000 -0800
+++ linux-2.6.5-ibmasm/drivers/misc/ibmasm/ibmasm.h 2004-04-06 10:56:31.000000000 -0700
@@ -220,5 +220,10 @@
extern void ibmasmfs_add_sp(struct service_processor *sp);

/* uart */
+#ifdef CONFIG_SERIAL_8250
extern void ibmasm_register_uart(struct service_processor *sp);
extern void ibmasm_unregister_uart(struct service_processor *sp);
+#else
+#define ibmasm_register_uart(sp) do { } while(0)
+#define ibmasm_unregister_uart(sp) do { } while(0)
+#endif
diff -urN linux-2.6.5/drivers/misc/ibmasm/Makefile linux-2.6.5-ibmasm/drivers/misc/ibmasm/Makefile
--- linux-2.6.5/drivers/misc/ibmasm/Makefile 2004-04-03 19:37:37.000000000 -0800
+++ linux-2.6.5-ibmasm/drivers/misc/ibmasm/Makefile 2004-04-06 13:07:54.000000000 -0700
@@ -1,7 +1,7 @@

obj-$(CONFIG_IBM_ASM) := ibmasm.o

-ibmasm-objs := module.o \
+ibmasm-y := module.o \
ibmasmfs.o \
event.o \
command.o \
@@ -9,5 +9,7 @@
heartbeat.o \
r_heartbeat.o \
dot_command.o \
- lowlevel.o \
- uart.o
+ lowlevel.o
+
+ibmasm-$(CONFIG_SERIAL_8250) += uart.o
+
diff -urN linux-2.6.5/drivers/misc/Kconfig linux-2.6.5-ibmasm/drivers/misc/Kconfig
--- linux-2.6.5/drivers/misc/Kconfig 2004-04-03 19:36:26.000000000 -0800
+++ linux-2.6.5-ibmasm/drivers/misc/Kconfig 2004-04-06 13:50:49.924254952 -0700
@@ -16,7 +16,9 @@
processor. The driver is meant to be used in conjunction with
a user space API.
The ibmasm driver also enables the OS to use the UART on the
- service processor board as a regular serial port.
+ service processor board as a regular serial port. To make use of
+ this feature serial driver support (CONFIG_SERIAL_8250) must be
+ enabled.


If unsure, say N.