Re: [PATCH 2/2] misc: add real function open/read/write/ioctl/closefor moxa_serial_io driver

[Alan Cox]

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/