lkml.org 
[lkml]   [2011]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] misc: add real function open/read/write/ioctl/close for moxa_serial_io driver
    > Add real function and GPL license.

    We really don't need an entire extra copy of the GPL. There is one in the
    source tree already to which you can refer!

    In code terms:
    - We have an expected interface for RS485/RS232 switching which
    is in the serial drivers. So the functionality is good but it
    really wants connecting to the serial driver.



    > -/* write function called when to /dev/mxsio is written */
    > -static ssize_t io_write (struct file *file, const char *buf,
    > - size_t count, loff_t *ppos) {
    > - int err;
    > - err = copy_from_user(string,buf,count);
    > -
    > - if(count < 3)
    > - return -EINVAL;
    > - if (err != 0)
    > - return -EFAULT;
    > -
    > - outb((unsigned char)string[2], (((unsigned short)string[0])<<8)|((unsigned short)string[1]));

    This can access anything so is entirely unsafe and unsuitable. Also you
    have no locking but use static buffers.

    > - outb(do_state_keep,BASEPORT+5);

    BASEPORT appears to be a magic constant - where does it come from given
    0x8000 is usually in PCI space ?

    > - outb(do_state_keep, BASEPORT+5);
    > - outb(0x00, BASEPORT);
    > -
    > - /* set default serial mode to RS232 */
    > - outb(0x88, BASEPORT+4);

    No checks to see if the relevant hardware is really present - so it will
    crash other systems

    > -void superio_enter_config(void) {
    > -#if defined(DA681)
    > - outb (0x87, SUPERIO_CONFIG_PORT);
    > - outb(0x80, 0xeb); // a Small delay
    > - outb (0x87, SUPERIO_CONFIG_PORT);
    > - outb(0x80, 0xeb); // a Small delay
    > -#elif defined(V21XX)
    > - outb (0x87, SUPERIO_CONFIG_PORT);
    > - outb (0x01, SUPERIO_CONFIG_PORT);
    > - outb (0x55, SUPERIO_CONFIG_PORT);
    > - outb (0x55, SUPERIO_CONFIG_PORT);
    > -#endif

    And what if a kernel is being built for two different configs at once ?

    > -}
    > -
    > -void superio_exit_config(void) {
    > - outb(0x02, SUPERIO_CONFIG_PORT);
    > - outb( 0x80, 0xeb); // a Small delay

    Use outb_p()

    the kernel then will figure out the right way to do the delay. Poking
    random I/O ports won't do what you want on some systems and may even
    crash them.


    \
     
     \ /
      Last update: 2011-04-25 13:41    [W:0.026 / U:57.832 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site