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    [W:0.048 / U:30.696 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site