Re: [PATCH] iuu_patch call for review comments

Am Donnerstag 16 August 2007 schrieb [email protected]:
> Sorry for the previous mail. A little modification was missing.
> 
> Now the code fits the Alan’s remarks concerning the tty flip call
> 
> Alain 
> 

+static int iuu_alloc_buf(struct iuu_private *priv)
+{
+	priv->buf = kzalloc(256, GFP_KERNEL);
+	priv->dbgbuf = kzalloc(256, GFP_KERNEL);
+	priv->writebuf = kzalloc(256, GFP_KERNEL);
+	if (!priv->buf || !priv->dbgbuf || !priv->writebuf) {
+		kfree(priv->buf);
+		kfree(priv->dbgbuf);
+		kfree(priv->writebuf);
+		dbg("%s problem allocation buffer", __FUNCTION__);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static void iuu_free_buf(struct iuu_private *priv)
+{
+	kfree(priv->buf);
+	kfree(priv->dbgbuf);
+	kfree(priv->writebuf);
+}

If you define such a function, use it.

+static int iuu_bulk_write(struct usb_serial_port *port)
+{
+	struct iuu_private *priv = usb_get_serial_port_data(port);
+	int result;
+	int i;
+	dbg("%s - enter", __FUNCTION__);
+
+	spin_lock_bh(&priv->lock);
+	if (port->write_urb_busy) {
+		spin_unlock_bh(&priv->lock);
+		dbg("%s - already writing", __FUNCTION__);
+		return (0);
+	}
+
+	spin_unlock_bh(&priv->lock);
+
+	memset(port->write_urb->transfer_buffer, IUU_UART_ESC, 1);
+	memset(port->write_urb->transfer_buffer + 1, IUU_UART_TX, 1);
+	memset(port->write_urb->transfer_buffer + 2, priv->writelen, 1);
+	memcpy(port->write_urb->transfer_buffer + 3, priv->writebuf,
+	       priv->writelen);
+	port->write_urb_busy = 1;

Against which bottom half are you guarding?
Secondly, this is a race. If write_urb_busy must be tested under
lock, it must also be set under the same lock.

+	/* we came from bulk write ! */
+	spin_lock_irqsave(&priv->lock, flags);
+	if (port->write_urb_busy) {
+		port->write_urb_busy = 0;
+		dbg("%s - came from bulk_write", __FUNCTION__);
+		usb_serial_port_softint(port);
+	}
+
+	spin_unlock_irqrestore(&priv->lock, flags);

Why do you test this? How do you prevent a reuse of that URB
if other users don't mark it busy?

+	status = bulk_immediate(port, buf, 4);
+	if (status != IUU_OPERATION_OK) {
+		dbg("%s - uart_on error", __FUNCTION__);
+		kfree(buf);
+		return status;
+	}

goto would be better

+static int
+iuu_uart_baud(struct usb_serial_port *port, u_int32_t baud, u_int32_t *actual,
+	      u_int8_t parity)
+{
+
+	/* SDK_STATUS sdk_status = SDK_SUCCESS; */
+	/* u8 dataout[10]; */
+	/* DWORD dwWritten = 0; */
+	/* unsigned void DataCount = 0; */
+
+	int status;
+	u8 *dataout;
+	u_int8_t DataCount = 0;
+	u8 T1Frekvens = 0;
+	u8 T1reload = 0;
+	unsigned int T1FrekvensHZ = 0;
+
+	dataout = kmalloc(sizeof(u8) * 5, GFP_KERNEL);
+
+	if (!dataout)
+		return -ENOMEM;

dataout is never freed. You have a memory leak.

+	memset(port->write_urb->transfer_buffer, IUU_UART_RX, 1);
+	usb_fill_bulk_urb(port->write_urb, port->serial->dev,
+			  usb_sndbulkpipe(port->serial->dev,
+					  port->bulk_out_endpointAddress),
+			  port->write_urb->transfer_buffer, 1,
+			  read_rxcmd_callback, port);
+	result = usb_submit_urb(port->write_urb, GFP_ATOMIC);

GFP_KERNEL will do.

	Regards
		Oliver

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel