lkml.org 
[lkml]   [2003]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Altix console driver
On Wed, Sep 17, 2003 at 03:24:14PM -0700, Jesse Barnes wrote:
> + * 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..

> +#include <linux/console.h>
> +#include <linux/module.h>
> +#ifdef CONFIG_MAGIC_SYSRQ
> +#include <linux/sysrq.h>
> +#endif

Please include it unconditionally.

> +#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?

> +#ifdef CONFIG_KDB
> +#include <linux/kdb.h>
> +#include <linux/serial_reg.h>

Keith doesn't like kdb code hitting mainline...

> +static char *serial_revdate = "2003-09-10";

This gets out of sync easily if someone external modifies the driver,
don't do it.

> +
> +#define snia_get_console_nasid get_console_nasid

Another level of obsfuction?

> +/* event types for our task queue -- so far just one */
> +#define SN_SAL_EVENT_WRITE_WAKEUP 0

So kill this layher of indirection?

> +#define CONSOLE_RESTART 0

When will this be enabled?


> +
> +/* 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?

> +
> +#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];
> +
> +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.

> +
> +/* 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?

> +/*********************************************************************
> + * Interrupt handling routines.
> + */

Please fix the comment format.

> +
> +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.

> +/* 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);
> +
> + 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?

> +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.

> +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.

> +
> + 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 *..

> +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.

> +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",
> + 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.

> +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..

> + 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.

> + 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.

> + 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.

> +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..

> + synch_flush_xmit();
> + sn_func->sal_puts(s, count);
> + }
> + else if (in_interrupt()) {

} else if (in_interrupt()) {
> + spin_lock(&sn_sal_lock);
> + synch_flush_xmit();
> + spin_unlock(&sn_sal_lock);
> + sn_func->sal_puts(s, count);
> + }
> + else

} else
-
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/

\
 
 \ /
  Last update: 2005-03-22 13:48    [from the cache]
©2003-2011 Jasper Spaans