preliminary version of major cleanup of mos7840
Oliver Neukum <[email protected]>
| Newsgroups | gmane.linux.usb.devel |
|---|---|
| Organization | Novell |
| Message-ID | <[email protected]> |
Hi,
this one is getting so large that I'd appreciate some early comments and
reviews. In fact, I don't like to make so many changes without a device
for testing. So please speak up if you can test.
This fixes:
- breaking DMA rules about buffers
- usage of _global_ variables to save a single device's attributes
- racy access to urb->status
- smp monotonity issue with statistics
- use of one buffer for many simultaneous URBs
- error handling introduced
- several instances of following NULL pointers
- use after free
- unnecessary GFP_ATOMIC
- GFP_KERNEL in interrupt
- various cleanups
- write room granularity issue that bit cdc-acm
I am not yet done.
Regards
Oliver
--- a/drivers/usb/serial/mos7840.c 2007-03-14 14:40:15.000000000 +0100
+++ b/drivers/usb/serial/mos7840.c 2007-03-15 14:47:54.000000000 +0100
@@ -176,9 +176,11 @@
int port_num; /*Actual port number in the device(1,2,etc) */
struct urb *write_urb; /* write URB for this port */
struct urb *read_urb; /* read URB for this port */
+ struct urb *int_urb;
__u8 shadowLCR; /* last LCR value received */
__u8 shadowMCR; /* last MCR value received */
char open;
+ char open_ports;
wait_queue_head_t wait_chase; /* for handling sleeping while waiting for chase to finish */
wait_queue_head_t delta_msr_wait; /* for handling sleeping while waiting for msr change to happen */
int delta_msr_cond;
@@ -191,17 +193,17 @@
__u8 DcrRegOffset;
//for processing control URBS in interrupt context
struct urb *control_urb;
+ struct usb_ctrlrequest *dr;
char *ctrl_buf;
int MsrLsr;
+ spinlock_t pool_lock;
struct urb *write_urb_pool[NUM_URBS];
+ char busy[NUM_URBS];
};
static int debug;
-static int mos7840_num_ports; //this says the number of ports in the device
-static int mos7840_num_open_ports;
-
/*
* mos7840_set_reg_sync
@@ -254,7 +256,7 @@
struct usb_device *dev = port->serial->dev;
val = val & 0x00ff;
// For the UART control registers, the application number need to be Or'ed
- if (mos7840_num_ports == 4) {
+ if (port->serial->num_ports == 4) {
val |=
(((__u16) port->number - (__u16) (port->serial->minor)) +
1) << 8;
@@ -294,7 +296,7 @@
//dbg("application number is %4x \n",(((__u16)port->number - (__u16)(port->serial->minor))+1)<<8);
/*Wval is same as application number */
- if (mos7840_num_ports == 4) {
+ if (port->serial->num_ports == 4) {
Wval =
(((__u16) port->number - (__u16) (port->serial->minor)) +
1) << 8;
@@ -366,15 +368,19 @@
/* update input line counters */
if (new_msr & MOS_MSR_DELTA_CTS) {
icount->cts++;
+ smp_wmb();
}
if (new_msr & MOS_MSR_DELTA_DSR) {
icount->dsr++;
+ smp_wmb();
}
if (new_msr & MOS_MSR_DELTA_CD) {
icount->dcd++;
+ smp_wmb();
}
if (new_msr & MOS_MSR_DELTA_RI) {
icount->rng++;
+ smp_wmb();
}
}
@@ -400,15 +406,19 @@
icount = &port->icount;
if (new_lsr & SERIAL_LSR_BI) {
icount->brk++;
+ smp_wmb();
}
if (new_lsr & SERIAL_LSR_OE) {
icount->overrun++;
+ smp_wmb();
}
if (new_lsr & SERIAL_LSR_PE) {
icount->parity++;
+ smp_wmb();
}
if (new_lsr & SERIAL_LSR_FE) {
icount->frame++;
+ smp_wmb();
}
return 0;
@@ -426,12 +436,15 @@
unsigned char *data;
struct moschip_port *mos7840_port;
__u8 regval = 0x0;
+ int result;
if (!urb) {
dbg("%s", "Invalid Pointer !!!!:\n");
return;
}
+ mos7840_port = (struct moschip_port *)urb->context;
+
switch (urb->status) {
case 0:
/* success */
@@ -449,8 +462,6 @@
goto exit;
}
- mos7840_port = (struct moschip_port *)urb->context;
-
dbg("%s urb buffer size is %d\n", __FUNCTION__, urb->actual_length);
dbg("%s mos7840_port->MsrLsr is %d port %d\n", __FUNCTION__,
mos7840_port->MsrLsr, mos7840_port->port_num);
@@ -463,20 +474,22 @@
mos7840_handle_new_lsr(mos7840_port, regval);
exit:
- return;
+ result = usb_submit_urb(mos7840_port->int_urb, GFP_ATOMIC);
+ if (result) {
+ dev_err(&urb->dev->dev,
+ "%s - Error %d submitting interrupt urb\n",
+ __FUNCTION__, result);
+ }
}
static int mos7840_get_reg(struct moschip_port *mcs, __u16 Wval, __u16 reg,
__u16 * val)
{
struct usb_device *dev = mcs->port->serial->dev;
- struct usb_ctrlrequest *dr = NULL;
- unsigned char *buffer = NULL;
- int ret = 0;
- buffer = (__u8 *) mcs->ctrl_buf;
+ struct usb_ctrlrequest *dr = mcs->dr;
+ unsigned char *buffer = mcs->ctrl_buf;
+ int ret;
-// dr=(struct usb_ctrlrequest *)(buffer);
- dr = (void *)(buffer + 2);
dr->bRequestType = MCS_RD_RTYPE;
dr->bRequest = MCS_RDREQ;
dr->wValue = cpu_to_le16(Wval); //0;
@@ -506,8 +519,8 @@
__u16 Data;
unsigned char *data;
__u8 sp[5], st;
- int i;
- __u16 wval;
+ int i, rv = 0;
+ __u16 wval, wreg = 0;
dbg("%s", " : Entering\n");
if (!urb) {
@@ -569,21 +582,20 @@
dbg("Serial Port %d: Receiver status error or ", i);
dbg("address bit detected in 9-bit mode\n");
mos7840_port->MsrLsr = 1;
- mos7840_get_reg(mos7840_port, wval,
- LINE_STATUS_REGISTER,
- &Data);
+ wreg = LINE_STATUS_REGISTER;
break;
case SERIAL_IIR_MS:
dbg("Serial Port %d: Modem status change\n", i);
mos7840_port->MsrLsr = 0;
- mos7840_get_reg(mos7840_port, wval,
- MODEM_STATUS_REGISTER,
- &Data);
+ wreg = MODEM_STATUS_REGISTER;
break;
}
+ rv = mos7840_get_reg(mos7840_port, wval, wreg, &Data);
}
}
}
+ if (!(rv < 0)) /* the completion handler for the control urb will resubmit */
+ return;
exit:
result = usb_submit_urb(urb, GFP_ATOMIC);
if (result) {
@@ -634,7 +646,8 @@
if (!port ||
mos7840_port_paranoia_check(port, function) ||
mos7840_serial_paranoia_check(port->serial, function)) {
- /* then say that we don't have a valid usb_serial thing, which will * end up genrating -ENODEV return values */
+ /* then say that we don't have a valid usb_serial thing, which will
+ * end up genrating -ENODEV return values */
return NULL;
}
@@ -699,6 +712,7 @@
tty_flip_buffer_push(tty);
}
mos7840_port->icount.rx += urb->actual_length;
+ smp_wmb();
dbg("mos7840_port->icount.rx is %d:\n",
mos7840_port->icount.rx);
}
@@ -708,15 +722,14 @@
return;
}
- if (mos7840_port->read_urb->status != -EINPROGRESS) {
- mos7840_port->read_urb->dev = serial->dev;
- status = usb_submit_urb(mos7840_port->read_urb, GFP_ATOMIC);
+ mos7840_port->read_urb->dev = serial->dev;
- if (status) {
- dbg(" usb_submit_urb(read bulk) failed, status = %d",
- status);
- }
+ status = usb_submit_urb(mos7840_port->read_urb, GFP_ATOMIC);
+
+ if (status) {
+ dbg(" usb_submit_urb(read bulk) failed, status = %d",
+ status);
}
}
@@ -730,17 +743,28 @@
{
struct moschip_port *mos7840_port;
struct tty_struct *tty;
+ int i;
+
if (!urb) {
dbg("%s", "Invalid Pointer !!!!:\n");
return;
}
+ mos7840_port = (struct moschip_port *)urb->context;
+ spin_lock(&mos7840_port->pool_lock);
+ for (i = 0; i < NUM_URBS; i++) {
+ if (urb == mos7840_port->write_urb_pool[i]) {
+ mos7840_port->busy[i] = 0;
+ break;
+ }
+ }
+ spin_unlock(&mos7840_port->pool_lock);
+
if (urb->status) {
dbg("nonzero write bulk status received:%d\n", urb->status);
return;
}
- mos7840_port = (struct moschip_port *)urb->context;
if (!mos7840_port) {
dbg("%s", "NULL mos7840_port pointer \n");
return;
@@ -792,13 +816,13 @@
__u16 Data;
int status;
struct moschip_port *mos7840_port;
+ struct moschip_port *port0;
if (mos7840_port_paranoia_check(port, __FUNCTION__)) {
dbg("%s", "Port Paranoia failed \n");
return -ENODEV;
}
- mos7840_num_open_ports++;
serial = port->serial;
if (mos7840_serial_paranoia_check(serial, __FUNCTION__)) {
@@ -807,16 +831,18 @@
}
mos7840_port = mos7840_get_port_private(port);
+ port0 = mos7840_get_port_private(serial->port[0]);
- if (mos7840_port == NULL)
+ if (mos7840_port == NULL || port0 == NULL)
return -ENODEV;
usb_clear_halt(serial->dev, port->write_urb->pipe);
usb_clear_halt(serial->dev, port->read_urb->pipe);
+ port0->open_ports++;
/* Initialising the write urb pool */
for (j = 0; j < NUM_URBS; ++j) {
- urb = usb_alloc_urb(0, GFP_ATOMIC);
+ urb = usb_alloc_urb(0, GFP_KERNEL);
mos7840_port->write_urb_pool[j] = urb;
if (urb == NULL) {
@@ -824,10 +850,10 @@
continue;
}
- urb->transfer_buffer = NULL;
- urb->transfer_buffer =
- kmalloc(URB_TRANSFER_BUFFER_SIZE, GFP_KERNEL);
+ urb->transfer_buffer = kmalloc(URB_TRANSFER_BUFFER_SIZE, GFP_KERNEL);
if (!urb->transfer_buffer) {
+ usb_free_urb(urb);
+ mos7840_port->write_urb_pool[j] = NULL;
err("%s-out of memory for urb buffers.", __FUNCTION__);
continue;
}
@@ -879,9 +905,7 @@
}
Data |= 0x08; //Driver done bit
Data |= 0x20; //rx_disable
- status = 0;
- status =
- mos7840_set_reg_sync(port, mos7840_port->ControlRegOffset, Data);
+ status = mos7840_set_reg_sync(port, mos7840_port->ControlRegOffset, Data);
if (status < 0) {
dbg("writing Controlreg failed\n");
return -1;
@@ -893,7 +917,6 @@
////////////////////////////////////
Data = 0x00;
- status = 0;
status = mos7840_set_uart_reg(port, INTERRUPT_ENABLE_REGISTER, Data);
if (status < 0) {
dbg("disableing interrupts failed\n");
@@ -901,7 +924,6 @@
}
// Set FIFO_CONTROL_REGISTER to the default value
Data = 0x00;
- status = 0;
status = mos7840_set_uart_reg(port, FIFO_CONTROL_REGISTER, Data);
if (status < 0) {
dbg("Writing FIFO_CONTROL_REGISTER failed\n");
@@ -909,7 +931,6 @@
}
Data = 0xcf;
- status = 0;
status = mos7840_set_uart_reg(port, FIFO_CONTROL_REGISTER, Data);
if (status < 0) {
dbg("Writing FIFO_CONTROL_REGISTER failed\n");
@@ -917,22 +938,18 @@
}
Data = 0x03;
- status = 0;
status = mos7840_set_uart_reg(port, LINE_CONTROL_REGISTER, Data);
mos7840_port->shadowLCR = Data;
Data = 0x0b;
- status = 0;
status = mos7840_set_uart_reg(port, MODEM_CONTROL_REGISTER, Data);
mos7840_port->shadowMCR = Data;
Data = 0x00;
- status = 0;
status = mos7840_get_uart_reg(port, LINE_CONTROL_REGISTER, &Data);
mos7840_port->shadowLCR = Data;
Data |= SERIAL_LCR_DLAB; //data latch enable in LCR 0x80
- status = 0;
status = mos7840_set_uart_reg(port, LINE_CONTROL_REGISTER, Data);
Data = 0x0c;
@@ -999,7 +1016,7 @@
/* Check to see if we've set up our endpoint info yet *
* (can't set it up in mos7840_startup as the structures *
* were not set up at that time.) */
- if (mos7840_num_open_ports == 1) {
+ if (port0->open_ports == 1) {
if (serial->port[0]->interrupt_in_buffer == NULL) {
/* set up interrupt urb */
@@ -1097,6 +1114,7 @@
{
int i;
int chars = 0;
+ unsigned long flags;
struct moschip_port *mos7840_port;
dbg("%s \n", " mos7840_chars_in_buffer:entering ...........");
@@ -1112,13 +1130,15 @@
return -1;
}
+ spin_lock_irqsave(&mos7840_port->pool_lock,flags);
for (i = 0; i < NUM_URBS; ++i) {
- if (mos7840_port->write_urb_pool[i]->status == -EINPROGRESS) {
+ if (mos7840_port->busy[i]) {
chars += URB_TRANSFER_BUFFER_SIZE;
}
}
+ spin_unlock_irqrestore(&mos7840_port->pool_lock,flags);
dbg("%s - returns %d", __FUNCTION__, chars);
- return (chars);
+ return chars;
}
@@ -1172,6 +1192,7 @@
{
struct usb_serial *serial;
struct moschip_port *mos7840_port;
+ struct moschip_port *port0;
int j;
__u16 Data;
@@ -1189,10 +1210,10 @@
}
mos7840_port = mos7840_get_port_private(port);
+ port0 = mos7840_get_port_private(serial->port[0]);
- if (mos7840_port == NULL) {
+ if (mos7840_port == NULL || port0 == NULL)
return;
- }
for (j = 0; j < NUM_URBS; ++j)
usb_kill_urb(mos7840_port->write_urb_pool[j]);
@@ -1234,12 +1255,13 @@
}
// if(mos7840_port->ctrl_buf != NULL)
// kfree(mos7840_port->ctrl_buf);
- mos7840_num_open_ports--;
+ port0->open_ports--;
dbg("mos7840_num_open_ports in close%d:in port%d\n",
- mos7840_num_open_ports, port->number);
- if (mos7840_num_open_ports == 0) {
+ port0->open_ports, port->number);
+ if (port0->open_ports == 0) {
if (serial->port[0]->interrupt_in_urb) {
dbg("%s", "Shutdown interrupt_in_urb\n");
+ usb_kill_urb(serial->port[0]->interrupt_in_urb);
}
}
@@ -1368,6 +1390,7 @@
{
int i;
int room = 0;
+ unsigned long flags;
struct moschip_port *mos7840_port;
dbg("%s \n", " mos7840_write_room:entering ...........");
@@ -1384,14 +1407,17 @@
return -1;
}
+ spin_lock_irqsave(&mos7840_port->pool_lock, flags);
for (i = 0; i < NUM_URBS; ++i) {
- if (mos7840_port->write_urb_pool[i]->status != -EINPROGRESS) {
+ if (!mos7840_port->busy[i]) {
room += URB_TRANSFER_BUFFER_SIZE;
}
}
+ spin_unlock_irqrestore(&mos7840_port->pool_lock, flags);
+ room = (room == 0) ? 0 : room - URB_TRANSFER_BUFFER_SIZE + 1;
dbg("%s - returns %d", __FUNCTION__, room);
- return (room);
+ return room;
}
@@ -1410,6 +1436,7 @@
int i;
int bytes_sent = 0;
int transfer_size;
+ unsigned long flags;
struct moschip_port *mos7840_port;
struct usb_serial *serial;
@@ -1476,13 +1503,16 @@
/* try to find a free urb in the list */
urb = NULL;
+ spin_lock_irqsave(&mos7840_port->pool_lock, flags);
for (i = 0; i < NUM_URBS; ++i) {
- if (mos7840_port->write_urb_pool[i]->status != -EINPROGRESS) {
+ if (!mos7840_port->busy[i]) {
+ mos7840_port->busy[i] = 1;
urb = mos7840_port->write_urb_pool[i];
dbg("\nURB:%d", i);
break;
}
}
+ spin_unlock_irqrestore(&mos7840_port->pool_lock, flags);
if (urb == NULL) {
dbg("%s - no more free urbs", __FUNCTION__);
@@ -1518,6 +1548,7 @@
status = usb_submit_urb(urb, GFP_ATOMIC);
if (status) {
+ mos7840_port->busy[i] = 0;
err("%s - usb_submit_urb(write bulk) failed with status = %d",
__FUNCTION__, status);
bytes_sent = status;
@@ -1525,6 +1556,7 @@
}
bytes_sent = transfer_size;
mos7840_port->icount.tx += transfer_size;
+ smp_wmb();
dbg("mos7840_port->icount.tx is %d:\n", mos7840_port->icount.tx);
exit:
@@ -2490,6 +2522,7 @@
if (signal_pending(current))
return -ERESTARTSYS;
cnow = mos7840_port->icount;
+ smp_rmb();
if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
return -EIO; /* no change => error */
@@ -2506,6 +2539,7 @@
case TIOCGICOUNT:
cnow = mos7840_port->icount;
+ smp_rmb();
icount.cts = cnow.cts;
icount.dsr = cnow.dsr;
icount.rng = cnow.rng;
@@ -2535,19 +2569,18 @@
static int mos7840_calc_num_ports(struct usb_serial *serial)
{
+ int mos7840_num_ports = 0;
dbg("numberofendpoints: %d \n",
(int)serial->interface->cur_altsetting->desc.bNumEndpoints);
dbg("numberofendpoints: %d \n",
(int)serial->interface->altsetting->desc.bNumEndpoints);
if (serial->interface->cur_altsetting->desc.bNumEndpoints == 5) {
- mos7840_num_ports = 2;
- serial->type->num_ports = 2;
+ mos7840_num_ports = serial->num_ports = 2;
} else if (serial->interface->cur_altsetting->desc.bNumEndpoints == 9) {
- mos7840_num_ports = 4;
- serial->type->num_bulk_in = 4;
- serial->type->num_bulk_out = 4;
- serial->type->num_ports = 4;
+ serial->num_bulk_in = 4;
+ serial->num_bulk_out = 4;
+ mos7840_num_ports = serial->num_ports = 4;
}
return mos7840_num_ports;
@@ -2583,7 +2616,9 @@
mos7840_port = kzalloc(sizeof(struct moschip_port), GFP_KERNEL);
if (mos7840_port == NULL) {
err("%s - Out of memory", __FUNCTION__);
- return -ENOMEM;
+ status = -ENOMEM;
+ i--; /* don't follow NULL pointer cleaning up */
+ goto error;
}
/* Initialize all port interrupt end point to port 0 int endpoint *
@@ -2591,6 +2626,7 @@
mos7840_port->port = serial->port[i];
mos7840_set_port_private(serial->port[i], mos7840_port);
+ spin_lock_init(&mos7840_port->pool_lock);
mos7840_port->port_num = ((serial->port[i]->number -
(serial->port[i]->serial->minor)) +
@@ -2601,22 +2637,22 @@
mos7840_port->ControlRegOffset = 0x1;
mos7840_port->DcrRegOffset = 0x4;
} else if ((mos7840_port->port_num == 2)
- && (mos7840_num_ports == 4)) {
+ && (serial->num_ports == 4)) {
mos7840_port->SpRegOffset = 0x8;
mos7840_port->ControlRegOffset = 0x9;
mos7840_port->DcrRegOffset = 0x16;
} else if ((mos7840_port->port_num == 2)
- && (mos7840_num_ports == 2)) {
+ && (serial->num_ports == 2)) {
mos7840_port->SpRegOffset = 0xa;
mos7840_port->ControlRegOffset = 0xb;
mos7840_port->DcrRegOffset = 0x19;
} else if ((mos7840_port->port_num == 3)
- && (mos7840_num_ports == 4)) {
+ && (serial->num_ports == 4)) {
mos7840_port->SpRegOffset = 0xa;
mos7840_port->ControlRegOffset = 0xb;
mos7840_port->DcrRegOffset = 0x19;
} else if ((mos7840_port->port_num == 4)
- && (mos7840_num_ports == 4)) {
+ && (serial->num_ports == 4)) {
mos7840_port->SpRegOffset = 0xc;
mos7840_port->ControlRegOffset = 0xd;
mos7840_port->DcrRegOffset = 0x1c;
@@ -2701,21 +2737,19 @@
dbg("CLK_START_VALUE_REGISTER Writing success status%d\n", status);
Data = 0x20;
- status = 0;
status =
mos7840_set_reg_sync(serial->port[i], CLK_MULTI_REGISTER,
Data);
if (status < 0) {
dbg("Writing CLK_MULTI_REGISTER failed status-0x%x\n",
status);
- break;
+ goto error;
} else
dbg("CLK_MULTI_REGISTER Writing success status%d\n",
status);
//write value 0x0 to scratchpad register
Data = 0x00;
- status = 0;
status =
mos7840_set_uart_reg(serial->port[i], SCRATCH_PAD_REGISTER,
Data);
@@ -2729,7 +2763,7 @@
//Zero Length flag register
if ((mos7840_port->port_num != 1)
- && (mos7840_num_ports == 2)) {
+ && (serial->num_ports == 2)) {
Data = 0xff;
status = 0;
@@ -2770,14 +2804,17 @@
i + 1, status);
}
- mos7840_port->control_urb = usb_alloc_urb(0, GFP_ATOMIC);
+ mos7840_port->control_urb = usb_alloc_urb(0, GFP_KERNEL);
mos7840_port->ctrl_buf = kmalloc(16, GFP_KERNEL);
-
+ mos7840_port->dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
+ if (!mos7840_port->control_urb || !mos7840_port->ctrl_buf || !mos7840_port->dr) {
+ status = -ENOMEM;
+ goto error;
+ }
}
//Zero Length flag enable
Data = 0x0f;
- status = 0;
status = mos7840_set_reg_sync(serial->port[0], ZLP_REG5, Data);
if (status < 0) {
dbg("Writing ZLP_REG5 failed status-0x%x\n", status);
@@ -2789,6 +2826,17 @@
usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
(__u8) 0x03, 0x00, 0x01, 0x00, NULL, 0x00, 5 * HZ);
return 0;
+error:
+ for (/* nothing */; i >= 0; i--) {
+ mos7840_port = mos7840_get_port_private(serial->port[i]);
+
+ kfree(mos7840_port->dr);
+ kfree(mos7840_port->ctrl_buf);
+ usb_free_urb(mos7840_port->control_urb);
+ kfree(mos7840_port);
+ serial->port[i] = NULL;
+ }
+ return status;
}
/****************************************************************************
@@ -2814,8 +2862,9 @@
for (i = 0; i < serial->num_ports; ++i) {
mos7840_port = mos7840_get_port_private(serial->port[i]);
- kfree(mos7840_port->ctrl_buf);
usb_kill_urb(mos7840_port->control_urb);
+ kfree(mos7840_port->ctrl_buf);
+ kfree(mos7840_port->dr);
kfree(mos7840_port);
mos7840_set_port_private(serial->port[i], NULL);
}
--
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)
This signature is a legal requirement
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel