Re: New driver proposal: iuu_phoenix

Am Mittwoch, 11. Juli 2007 schrieb Alain Degreffe:
> sequence, all is running well. I join the (hugly ?) code for a review. I'm a

1. you violate the kernel coding style (look into the Documentation directory)
2. you should use a .h file for your defines

3.:

static int
iuu_startup (struct usb_serial *serial)
{
  struct iuu_private *priv;
  int i;

  for (i = 0; i < serial->num_ports; ++i) {
    priv = kmalloc (sizeof (struct iuu_private), GFP_KERNEL);
    if (!priv)
      return -ENOMEM;

You are setting a trap here. You have only one port, but you are using
a loop. Yet, should you ever need it, you have a memory leak in the error
case.

4.:

  if ((set == 0) && priv->TIOSTATUS == TIOCM_RTS) {
    dbg ("%s TIOCMSET RESET called !!!", __FUNCTION__);
    priv->TIOSTATUS = 0;
    iuu_reset (port, 0x0C);
    current->state = TASK_INTERRUPTIBLE;
    schedule_timeout (1 + 500 * HZ / 1000);
    iuu_uart_read_and_store (port);
  }

If you need a delay, do it explicitely. You might be woken up prematurely.

5.:

static int
iuu_ioctl (struct usb_serial_port *port, struct file *file, unsigned int cmd, unsigned long arg)
{

  int mask;



  dbg ("%s (%d) cmd = 0x%04x", __FUNCTION__, port->number, cmd);

  get_user (mask, (unsigned long *) arg);

This may fail. You need to handle errors.

6.:

static int
iuu_reset (struct usb_serial_port *port, u_int8_t wt)
{
  int status;
  u_int8_t buf[4];

  status = iuu_uart_flush (port);
  if (status != IUU_OPERATION_OK) {
    dbg ("%s - reset (flush) error ", __FUNCTION__);
    return status;
  }

  buf[0] = IUU_RST_SET;
  buf[1] = IUU_DELAY_MS;
  buf[2] = wt;			/* miliseconds */
  buf[3] = IUU_RST_CLEAR;

  status = bulk_immediate (port, buf, 4);

DMA on the stack is not allowed. buf[4] must be kmalloced.

7.:

  /* send the data out the bulk port */
  priv->im_write_busy = 1;

This variable is never read.

8.:

static int
read_immediate (struct usb_serial_port *port, unsigned char *buf, int count)
{
  int status;
  struct usb_serial *serial = port->serial;

Do you real want to throw away actual?

9.:

int
iuu_led (struct usb_serial_port *port, u_int16_t R, u_int16_t G, u_int16_t B, u_int8_t f)
{
  int status;
  u_int8_t buf[8];

No DMA on the stack. And you might define a structure for that.

10.:

  if (priv->write_busy)
    return -1;

  /*
     if ( priv->loop > 2000) {
     dbg("%s - loop detected ", __FUNCTION__);
     return 0;
     }
   */

  priv->write_busy = 1;

This is a race condition right out of the textbook.

And there are other similar errors. I'll be happy to do another review after
you'll have fixed the issues I've pointed out.

	HTH
		Oliver

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel