Altix console driver
PATCH 1/1 Altix console driver
Just a simple addition to drivers/char for the Altix serial console.
MAINTAINERS | 6 drivers/char/Kconfig | 16 drivers/char/Makefile | 1 drivers/char/sn_serial.c | 1189 +++++++++++++++++++++++++++++++++++++++++++++++
Thanks,
Jesse
MAINTAINERS +6 -0
@@ -1727,6 +1727,12 @@ W: http://www.nsa.gov/selinux S: Supported +SGI SN-IA64 (Altix) SERIAL CONSOLE DRIVER +P: Pat Gefre +M: [email protected] +L: [email protected] +S: Supported + SGI VISUAL WORKSTATION 320 AND 540 P: Andrey Panin M: [email protected]
drivers/char/Kconfig +16 -0
@@ -393,6 +393,22 @@ If you have an Alchemy AU1000 processor (MIPS based) and you want to use a console on a serial port, say Y. Otherwise, say N. +config SGI_L1_SERIAL + bool "SGI Altix L1 serial support" + depends on SERIAL_NONSTANDARD && IA64 + help + If you have an SGI Altix and you want to use the serial port + connected to the system controller (you want this!), say Y. + Otherwise, say N. + +config SGI_L1_SERIAL_CONSOLE + bool "SGI Altix L1 serial console support" + depends on SGI_L1_SERIAL + help + If you have an SGI Altix and you would like to use the system + controller serial port as your console (you want this!), + say Y. Otherwise, say N. + config QTRONIX_KEYBOARD bool "Enable Qtronix 990P Keyboard Support" depends on IT8712
drivers/char/Makefile +1 -0
@@ -41,6 +41,7 @@ obj-$(CONFIG_SERIAL_TX3912) += generic_serial.o serial_tx3912.o obj-$(CONFIG_HVC_CONSOLE) += hvc_console.o obj-$(CONFIG_RAW_DRIVER) += raw.o +obj-$(CONFIG_SGI_L1_SERIAL) += sn_serial.o obj-$(CONFIG_PRINTER) += lp.o obj-$(CONFIG_TIPAR) += tipar.o
drivers/char/sn_serial.c new file +1189 -0
1,189 lines not on this page · raw message
Quoted 7 lines · [email protected] (Jesse Barnes) wrote:
Just a simple addition to
drivers/charfor the Altix serial console.MAINTAINERS | 6 drivers/char/Kconfig | 16 drivers/char/Makefile | 1 drivers/char/sn_serial.c | 1189 +++++++++++++++++++++++++++++++++++++++++++++++
Would it be more appropriate to place this under arch/ia64?
Quoted 10 lines · Andrew Morton
Quoted 7 lines · [email protected] (Jesse Barnes) wrote:
Just a simple addition to
drivers/charfor the Altix serial console.MAINTAINERS | 6 drivers/char/Kconfig | 16 drivers/char/Makefile | 1 drivers/char/sn_serial.c | 1189 +++++++++++++++++++++++++++++++++++++++++++++++Would it be more appropriate to place this under
arch/ia64?
I've always been told not to do that. Christoph? ;)
Jesse
Quoted 1 line · Andrew Morton
Would it be more appropriate to place this under
arch/ia64?
arch/ drivers are evil. Also the driver could possibly used on mips if someone does the port to this hardware..
Quoted 1 line · Andrew Morton
Would it be more appropriate to place this under
arch/ia64?
Unless they clarify the GPL violating license clause I'd suggest strongly it goes into /dev/null. The GPL states:
6. Each time you redistribute the Program (or any work based on the
Program), the recipient automatically receives a license from the original licensor to copy, distribute or modify the Program subject to these terms and conditions. You may not impose any further restrictions on the recipients' exercise of the rights granted herein. You are not responsible for enforcing compliance by third parties to this License.
I am perfectly permitted by the GPL to modify Linux and add other code to it, or to create a new product based on it that is GPL licensed.
SGI need to fix their boilerplate. I can kind of guess what they are trying to say but it needs tweaking.
Well, according to the FSF our extra clauses are compatible with the GPL and LGPL. See http://oss.sgi.com/projects/GenInfo/NoticeExplan/. If you still disagree then we'll have to try to find another solution.
Thanks,
Jesse
Quoted 22 lines · Alan Cox
Quoted 1 line · Andrew Morton
Would it be more appropriate to place this under
arch/ia64?Unless they clarify the GPL violating license clause I'd suggest strongly it goes into
/dev/null. The GPL states:6. Each time you redistribute the Program (or any work based on the
Program), the recipient automatically receives a license from the original licensor to copy, distribute or modify the Program subject to these terms and conditions. You may not impose any further restrictions on the recipients' exercise of the rights granted herein. You are not responsible for enforcing compliance by third parties to this License.I am perfectly permitted by the GPL to modify Linux and add other code to it, or to create a new product based on it that is GPL licensed.
SGI need to fix their boilerplate. I can kind of guess what they are trying to say but it needs tweaking.
Quoted 3 lines · Jesse Barnes
Well, according to the FSF our extra clauses are compatible with the GPL and LGPL. See http://oss.sgi.com/projects/GenInfo/NoticeExplan/. If you still disagree then we'll have to try to find another solution.
I think I need to discuss this with Eben, it certainly seems a problem to me but he's the legal expert so I want to find out his view first.
Quoted 7 lines · Alan Cox
Quoted 3 lines · Jesse Barnes
Well, according to the FSF our extra clauses are compatible with the GPL and LGPL. See http://oss.sgi.com/projects/GenInfo/NoticeExplan/. If you still disagree then we'll have to try to find another solution.
I think I need to discuss this with Eben, it certainly seems a problem to me but he's the legal expert so I want to find out his view first.
My reading of the boilerplate is that I can't use their code in another GPL program, because their patent grant doesn't extend to other programs.
Even if that's permitted by the GPL, it doesn't mean you have to accept it into the kernel like that.
If that's just a misunderstanding of the legalese on my part, then IMHO the text needs to be clarified to ensure that everyone knows they may take the code and use it in other GPL projects.
-- Jamie
Quoted 3 lines · Jamie Lokier
If that's just a misunderstanding of the legalese on my part, then IMHO the text needs to be clarified to ensure that everyone knows they may take the code and use it in other GPL projects.
I'll let folks know when I hear back from the FSF folks or if SGI decide to tweak it anyway.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines
Quoted 6 lines
Just a simple addition to
drivers/charfor the Altix serial console.MAINTAINERS | 6 drivers/char/Kconfig | 16 drivers/char/Makefile | 1 drivers/char/sn_serial.c | 1189 +++++++++++++++++++++++++++++++++++++++++++++++Would it be more appropriate to place this under
arch/ia64?
It was discussed on other occasions, and it "was decided" (sprinkled with holy penguin pee) that such things go into drivers/, e.g. drivers/serial/sunsu.c is Sun-specific, but it helps to keep it near drivers/serial/8250.c, for diffing. If an additional API is involved, e.g. sbus_alloc_consistent, drivers get a subdir like drivers/sbus or drivers/s390. Also, you never know when sharing hits you across the head, e.g. sparc, sparc64, and m68k (for sun3) share some drivers.
-- Pete
Quoted 6 lines · Jesse Barnes
+ * Further, this software is distributed without any warranty that it is
+ * free of the rightful claim of any third person regarding infringement
+ * or the like. Any license provided herein, whether implied or
+ * otherwise, applies only to this software file. Patent licenses, if
+ * any, provided herein do not apply to combinations of this program with
+ * other software, or any other product whatsoever.
This seems to be a restriction not compatible with the GPL. And it looks like many SGI files in the tree have it aswell..
drivers/char/sn_serial.c line 49
Quoted 5 lines
drivers/char/sn_serial.c new file +5
-0+#include <linux/console.h> +#include <linux/module.h> +#ifdef CONFIG_MAGIC_SYSRQ +#include <linux/sysrq.h> +#endif
Please include it unconditionally.
drivers/char/sn_serial.c line 62
Quoted 7 lines
drivers/char/sn_serial.c new file +7
-0+#include <asm/sn/sn_sal.h> +#include <asm/sn/pci/pciio.h> /* this is needed for snia_get_console_nasid */ +#include <asm/sn/nodepda.h> +#include <asm/sn/simulator.h> +#include <asm/sn/sn2/intr.h> +#include <asm/sn/sn2/sn_private.h> +#include <asm/sn/clksupport.h>
Yuck. Do you really need all these crufty includes?
drivers/char/sn_serial.c line 66
Quoted 3 lines
drivers/char/sn_serial.c new file +3
-0+#ifdef CONFIG_KDB +#include <linux/kdb.h> +#include <linux/serial_reg.h>
Keith doesn't like kdb code hitting mainline...
Quoted 1 line
+static char
*serial_revdate= "2003-09-10";
This gets out of sync easily if someone external modifies the driver, don't do it.
Review
Quoted 2 lines
(quoted diff) +2
-0+ +#define snia_get_console_nasid get_console_nasid
Another level of obsfuction?
drivers/char/sn_serial.c line 109
Quoted 2 lines
drivers/char/sn_serial.c new file +2
-0+/* event types for our task queue -- so far just one */ +#define SN_SAL_EVENT_WRITE_WAKEUP 0
So kill this layher of indirection?
Quoted 1 line
+#define
CONSOLE_RESTART0
When will this be enabled?
drivers/char/sn_serial.c line 119
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+ +/* 64K, when we're asynch, it must be at least printk's LOG_BUF_LEN to +* avoid losing chars, (always has to be a power of 2) */ +#if 1 +#define SN_SAL_BUFFER_SIZE (64 * (1 << 10)) +#else +#define SN_SAL_BUFFER_SIZE (64) +#endif
Why keep the #if 0?
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+ +#define SN_SAL_UART_FIFO_DEPTH 16 +#define SN_SAL_UART_FIFO_SPEED_CPS 9600/10 + +/* we don't kmalloc/get_free_page these as we want them available +* before either of those are initialized */ +static volatile char sn_xmit_buff_mem[SN_SAL_BUFFER_SIZE ]; +
drivers/char/sn_serial.c line 137
Quoted 10 lines
drivers/char/sn_serial.c new file +10
-0+struct volatile_circ_buf { +volatile char *cb_buf; +int cb_head; +int cb_tail; +}; + +static volatile struct volatile_circ_buf xmit = { .cb_buf = sn_xmit_buff_mem }; +static char sn_tmp_buffer[SN_SAL_BUFFER_SIZE ]; + +static volatile struct tty_struct *sn_sal_tty;
Please do proper locking instread of volatile abuse.
drivers/char/sn_serial.c line 308
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+ +/* The early printk (possible setup) and function call */ + +void +early_printk_sn_sal (const char *s,unsigned count) +{ +extern void early_sn_setup (void ); +
there is not early printk in mainline, only in the ia64 tree, isn't it?
Quoted 3 lines
+/*********************************************************************
+ * Interrupt handling routines.
+ */
Please fix the comment format.
drivers/char/sn_serial.c line 566
Quoted 10 lines
drivers/char/sn_serial.c new file +10
-0+ +static irqreturn_t +sn_sal_interrupt (int irq,void *dev_id,struct pt_regs *regs) +{ +/* this call is necessary to pass the interrupt back to the +* SAL, since it doesn't intercept the UART interrupts +* itself */ +int status =ia64_sn_console_intr_status (); + +spin_lock (&sn_sal_lock);
This is registered with SA_INTERRUPT, so the plain spin_lock probably is not enough.
Quoted 15 lines
drivers/char/sn_serial.c new file +15
-0+/* returns the console irq if interrupt is successfully registered, +* else 0 */ +static int +sn_sal_connect_interrupt (void ) +{ +cpuid_t intr_cpuid; +unsigned int intr_cpuloc; +nasid_t console_nasid; +unsigned int console_irq; +int result; + + console_nasid =ia64_sn_get_console_nasid (); + intr_cpuid =NODEPDA (NASID_TO_COMPACT_NODEID (console_nasid))->node_first_cpu; + intr_cpuloc =cpu_physical_id (intr_cpuid); + console_irq =CPU_VECTOR_TO_IRQ (intr_cpuloc,SGI_UART_VECTOR );
drivers/char/sn_serial.c line 600
Quoted 10 lines
drivers/char/sn_serial.c new file +10
-0+ + result =intr_connect_level (intr_cpuid,SGI_UART_VECTOR ,0 /*not used*/ ,0 /*not used*/ ); +BUG_ON (result !=SGI_UART_VECTOR ); + + result =request_irq (console_irq, sn_sal_interrupt,SA_INTERRUPT ,"SAL console driver" , &sn_sal_tty); +if (result >=0 ) +return console_irq; + +printk (KERN_INFO "sn_serial: console proceeding in polled mode\n" ); +return 0 ;
Yikes! WTF is this? Okay, looking through arch/ia64/sn I see that request_irq is basically a noop on SN2 and you must do intr_connect_level instead. Could you _please_fix this up by registering a struct hw_interrupt_type for the hub-level interrupts and getting rid of all this intr_connect_level madness?
drivers/char/sn_serial.c line 620
Quoted 13 lines
drivers/char/sn_serial.c new file +13
-0+static void +sn_sal_tasklet_action (unsigned long data) +{ +unsigned long flags; + +if (sn_sal_tty) { +spin_lock_irqsave (&sn_sal_lock, flags); +if (sn_sal_tty) { +if (test_and_clear_bit (SN_SAL_EVENT_WRITE_WAKEUP , &sn_sal_event)) { +if ((sn_sal_tty->flags & (1 <<TTY_DO_WRITE_WAKEUP )) && sn_sal_tty->ldisc.write_wakeup) + (sn_sal_tty->ldisc.write_wakeup)((struct tty_struct *)sn_sal_tty); +wake_up_interruptible ((wait_queue_head_t *)&sn_sal_tty->write_wait); + }
Please linebreak after 80 chars.
drivers/char/sn_serial.c line 672
Quoted 11 lines
drivers/char/sn_serial.c new file +11
-0+static int +sn_sal_open (struct tty_struct *tty,struct file *filp) +{ +unsigned long flags; + +DPRINTF ("sn_sal_open: sn_sal_tty = %p, tty = %p, filp = %p\n" , + sn_sal_tty, tty, filp); + +spin_lock_irqsave (&sn_sal_lock, flags); +if (!sn_sal_tty) + sn_sal_tty = tty;
Eeek. Either you allow opening just one tty and you don't need this lock or you fix up the driver to pass the tty_struct to where it is used properly.
drivers/char/sn_serial.c line 734
Quoted 2 lines
drivers/char/sn_serial.c new file +2
-0+ +memcpy ((char *)xmit.cb_buf + xmit.cb_head, sn_tmp_buffer, c);
No need to cast the first ar to memcpy. And even if you did it it would be void *..
drivers/char/sn_serial.c line 838
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+static int +sn_sal_ioctl (struct tty_struct *tty,struct file *file, +unsigned int cmd,unsigned long arg) +{ +/* nothing supported */ + +return -ENOIOCTLCMD ; +}
So don't implement this function at all. The upper layers will deal with this nicely.
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+static int +sn_sal_read_proc (char *page,char **start,off_t off,int count, +int *eof,void *data) +{ +int len =0 ; +off_t begin =0 ; + + len += sprintf(page, "sn_serial: revision:%s nasid:%d irq:%d tx:%d rx:%d\n",
drivers/char/sn_serial.c line 896
Quoted 10 lines
drivers/char/sn_serial.c new file +10
-0+ serial_revdate,snia_get_console_nasid (), sn_sal_irq, + sn_total_tx_count, sn_total_rx_count); + *eof =1 ; + +if (off >= len+begin) +return 0 ; + *start = page + (off-begin); + +return count < begin+len-off ? count : begin+len-off; +}
Either convert to seq_file or better kill it off completly. I don't look very useful.
drivers/char/sn_serial.c line 902
Quoted 4 lines
drivers/char/sn_serial.c new file +4
-0+static struct tty_operations sn_sal_driver_ops = { + .open = sn_sal_open, + .close = sn_sal_close, + .write = sn_sal_write,
.open = sn_sal_open, .close = sn_sal_close,
etc..
drivers/char/sn_serial.c line 1021
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+ sn_sal_driver->owner =THIS_MODULE ; + sn_sal_driver->driver_name ="sn_serial" ; + sn_sal_driver->name ="ttyS" ; + sn_sal_driver->major =TTY_MAJOR ; + sn_sal_driver->minor_start =SN_SAL_MINOR ; + sn_sal_driver->type =TTY_DRIVER_TYPE_SERIAL ; + sn_sal_driver->subtype =SERIAL_TYPE_NORMAL ; + sn_sal_driver->flags =TTY_DRIVER_REAL_RAW |TTY_DRIVER_NO_DEVFS ;
So you're using the normal TTY_MAJOR in this driver? And use a different name for 64 minors offsets. So what happens if someone puts a multiport pci serial card into an SN2?
Also you really want to use drivers/serial/serial_core.c for this driver and move it to drivers/serial. rmk has a 2.4 version of that file aswell so the 2.4 altix driver can use it, too.
drivers/char/sn_serial.c line 1047
Quoted 10 lines
drivers/char/sn_serial.c new file +10
-0+if ((retval =tty_register_driver (sn_sal_driver))) { +printk (KERN_ERR "sn_serial: Unable to register tty driver\n" ); +return retval; + } +#ifdef CONFIG_SGI_L1_SERIAL_CONSOLE + { +void __init sn_sal_serial_console_init (void ); +sn_sal_serial_console_init (); + } +#endif /* CONFIG_SGI_L1_SERIAL_CONSOLE */
Eeek. sn_sal_serial_console_init should be static, shouldn't it. And have a proper prototype. Then you could also get rid of the ugly scoping mess.
drivers/char/sn_serial.c line 1063
Quoted 6 lines
drivers/char/sn_serial.c new file +6
-0+del_timer_sync (&sn_sal_timer); +spin_lock_irqsave (&sn_sal_lock, flags); +if ((tmp =tty_unregister_driver (sn_sal_driver))) +printk (KERN_ERR "sn_serial: failed to unregister driver (%d)\n" , tmp); + +spin_unlock_irqrestore (&sn_sal_lock, flags);
tty_unregister_driver under a look looks strange. Also where's the put_tty_driver? And the retval from tty_unregister_driver is meaningless, your module is gone now anyway.
drivers/char/sn_serial.c line 1090
Quoted 8 lines
drivers/char/sn_serial.c new file +8
-0+static void +sn_sal_console_write (struct console *co,const char *s,unsigned count) +{ +BUG_ON (!sn_sal_is_asynch); + +/* somebody really wants this output, might be an +* oops, kdb, panic, etc. make sure they get it. */ +if (spin_is_locked (&sn_sal_lock)) {
spin_is_locked doesn't work on !SMP && !PREEMPT..
drivers/char/sn_serial.c line 1094
Quoted 4 lines
drivers/char/sn_serial.c new file +4
-0+synch_flush_xmit (); + sn_func->sal_puts (s, count); + } +else if (in_interrupt ()) {
} else if (in_interrupt()) {drivers/char/sn_serial.c line 1100
Quoted 6 lines
drivers/char/sn_serial.c new file +6
-0+spin_lock (&sn_sal_lock); +synch_flush_xmit (); +spin_unlock (&sn_sal_lock); + sn_func->sal_puts (s, count); + } +else
} else