[PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[Alon Ziv]

Any module using usb_serial_generic_write (which is already exported)
had better use usb_serial_generic_write_room as well.

Signed-off-by: Alon Ziv <alon-git@nolaviz.org>
---
 drivers/usb/serial/generic.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index e6833e2..c663369 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -288,6 +288,7 @@ int usb_serial_generic_write_room(struct tty_struct *tty)
 	dbg("%s - returns %d", __func__, room);
 	return room;
 }
+EXPORT_SYMBOL_GPL(usb_serial_generic_write_room);
 
 int usb_serial_generic_chars_in_buffer(struct tty_struct *tty)
 {
-- 
1.7.0.4

Re: [PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[Greg KH]

On Sat, Oct 23, 2010 at 06:20:17PM +0200, Alon Ziv wrote:
> Any module using usb_serial_generic_write (which is already exported)
> had better use usb_serial_generic_write_room as well.

Are you implying that the one driver using this function today is
broken?  Care to provide a patch for it as well?

thanks,

greg k-h

Re: [PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[Alon Ziv]

On 10/23/2010 06:37 PM, Greg KH wrote:
> On Sat, Oct 23, 2010 at 06:20:17PM +0200, Alon Ziv wrote:
>   
>> Any module using usb_serial_generic_write (which is already exported)
>> had better use usb_serial_generic_write_room as well.
>>     
> Are you implying that the one driver using this function today is
> broken?  Care to provide a patch for it as well?
>
>   
Well...
The one driver using it now is usb_debug, which only supports BREAK
signaling.
The generic code does not invoke the write_room callback before invoking
break_ctl, so there is no way to use this function in usb_serial.

At worst, with the current code, usb_serial's break_ctl may become a
no-op if
another BREAK signal is outstanding--which should not be an issue (assuming
regular serial protocol semantics, that is, "BREAK" has indeterminate
length).

-a

Re: [PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[Alan Cox]

> The generic code does not invoke the write_room callback before invoking
> break_ctl, so there is no way to use this function in usb_serial.

Why would it ?

Alan

Re: [PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[Alon Ziv]

On 10/24/2010 10:37 AM, Alan Cox wrote:

>> The generic code does not invoke the write_room callback before invoking
>> break_ctl, so there is no way to use this function in usb_serial.
>>     
> Why would it ?
>
>   
Oops, typo...  Last sentence was supposed to end with "in usb_debug".
usb_debug invokes usb_serial_generic_write from break_ctl and ignores
the result; therefore, if there is no room in the generic write FIFO,
the write will fail.  However--usb_debug _only_ uses
usb_serial_generic_write for break_ctl; so if there is no room, it is
because of a previous (uncompleted) break_ctl, and according to standard
UART semantics there is no difference between one break and two...

-a

Re: [PATCH 1/2] Export usb_serial_generic_write_room for use in other modules

[Johan Hovold]

On Sat, Oct 23, 2010 at 09:37:47AM -0700, Greg KH wrote:
> On Sat, Oct 23, 2010 at 06:20:17PM +0200, Alon Ziv wrote:
> > Any module using usb_serial_generic_write (which is already exported)
> > had better use usb_serial_generic_write_room as well.
> 
> Are you implying that the one driver using this function today is
> broken?  Care to provide a patch for it as well?

Actually, most usb serial drivers use usb_serial_generic_write as well
as usb_serial_generic_write_room. The generic implementations are used
unless drivers explicitly define their own. So far there has been no
need to export usb_serial_generic_write_room but perhaps Alon's patch
does require this.

Either way the patch description is thus actually wrong (or rather
misleading) as it stands.

I'll try to have a look at the other patch tomorrow.

Johan

[PATCH 2/2] OPN2001: use generic code where possible

[Alon Ziv]

Replace custom read/write code with calls to usb_serial_generic_XXX.

Signed-off-by: Alon Ziv <alon-git@nolaviz.org>
---
 drivers/usb/serial/opticon.c |  392 +++++-------------------------------------
 1 files changed, 45 insertions(+), 347 deletions(-)

diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index eda1f92..5bbed97 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -31,54 +31,19 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 /* This structure holds all of the individual device information */
 struct opticon_private {
-	struct usb_device *udev;
-	struct usb_serial *serial;
-	struct usb_serial_port *port;
-	unsigned char *bulk_in_buffer;
-	struct urb *bulk_read_urb;
-	int buffer_size;
-	u8 bulk_address;
-	spinlock_t lock;	/* protects the following flags */
-	bool throttled;
-	bool actually_throttled;
 	bool rts;
-	int outstanding_urbs;
 };
 
 /* max number of write urbs in flight */
 #define URB_UPPER_LIMIT	4
 
-static void opticon_bulk_callback(struct urb *urb)
+static void opticon_process_read_urb(struct urb *urb)
 {
-	struct opticon_private *priv = urb->context;
+	struct usb_serial_port *port = urb->context;
+	struct opticon_private *priv = usb_get_serial_data(port->serial);
 	unsigned char *data = urb->transfer_buffer;
-	struct usb_serial_port *port = priv->port;
-	int status = urb->status;
-	struct tty_struct *tty;
-	int result;
 	int data_length;
-
-	dbg("%s - port %d", __func__, port->number);
-
-	switch (status) {
-	case 0:
-		/* success */
-		break;
-	case -ECONNRESET:
-	case -ENOENT:
-	case -ESHUTDOWN:
-		/* this urb is terminated, clean up */
-		dbg("%s - urb shutting down with status: %d",
-		    __func__, status);
-		return;
-	default:
-		dbg("%s - nonzero urb status received: %d",
-		    __func__, status);
-		goto exit;
-	}
-
-	usb_serial_debug_data(debug, &port->dev, __func__, urb->actual_length,
-			      data);
+	struct tty_struct *tty;
 
 	if (urb->actual_length > 2) {
 		data_length = urb->actual_length - 2;
@@ -87,10 +52,10 @@ static void opticon_bulk_callback(struct urb *urb)
 		 * Data from the device comes with a 2 byte header:
 		 *
 		 * <0x00><0x00>data...
-		 * 	This is real data to be sent to the tty layer
-		 * <0x00><0x01)level
-		 * 	This is a RTS level change, the third byte is the RTS
-		 * 	value (0 for low, 1 for high).
+		 *	This is real data to be sent to the tty layer
+		 * <0x00><0x01>level
+		 *	This is a RTS level change, the third byte is the RTS
+		 *	value (0 for low, 1 for high).
 		 */
 		if ((data[0] == 0x00) && (data[1] == 0x00)) {
 			/* real data, send it to the tty layer */
@@ -108,269 +73,83 @@ static void opticon_bulk_callback(struct urb *urb)
 				else
 					priv->rts = true;
 			} else {
-				dev_dbg(&priv->udev->dev,
+				dev_dbg(&port->dev,
 					"Unknown data packet received from the device:"
 					" %2x %2x\n",
 					data[0], data[1]);
 			}
 		}
 	} else {
-		dev_dbg(&priv->udev->dev,
+		dev_dbg(&port->dev,
 			"Improper amount of data received from the device, "
 			"%d bytes", urb->actual_length);
 	}
-
-exit:
-	spin_lock(&priv->lock);
-
-	/* Continue trying to always read if we should */
-	if (!priv->throttled) {
-		usb_fill_bulk_urb(priv->bulk_read_urb, priv->udev,
-				  usb_rcvbulkpipe(priv->udev,
-						  priv->bulk_address),
-				  priv->bulk_in_buffer, priv->buffer_size,
-				  opticon_bulk_callback, priv);
-		result = usb_submit_urb(priv->bulk_read_urb, GFP_ATOMIC);
-		if (result)
-			dev_err(&port->dev,
-			    "%s - failed resubmitting read urb, error %d\n",
-							__func__, result);
-	} else
-		priv->actually_throttled = true;
-	spin_unlock(&priv->lock);
 }
 
 static int opticon_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
-	unsigned long flags;
-	int result = 0;
-
 	dbg("%s - port %d", __func__, port->number);
-
-	spin_lock_irqsave(&priv->lock, flags);
-	priv->throttled = false;
-	priv->actually_throttled = false;
-	priv->port = port;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	/* Start reading from the device */
-	usb_fill_bulk_urb(priv->bulk_read_urb, priv->udev,
-			  usb_rcvbulkpipe(priv->udev,
-					  priv->bulk_address),
-			  priv->bulk_in_buffer, priv->buffer_size,
-			  opticon_bulk_callback, priv);
-	result = usb_submit_urb(priv->bulk_read_urb, GFP_KERNEL);
-	if (result)
-		dev_err(&port->dev,
-			"%s - failed resubmitting read urb, error %d\n",
-			__func__, result);
-	return result;
+	return usb_serial_generic_open(tty, port);
 }
 
 static void opticon_close(struct usb_serial_port *port)
 {
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
-
 	dbg("%s - port %d", __func__, port->number);
-
-	/* shutdown our urbs */
-	usb_kill_urb(priv->bulk_read_urb);
-}
-
-static void opticon_write_bulk_callback(struct urb *urb)
-{
-	struct opticon_private *priv = urb->context;
-	int status = urb->status;
-	unsigned long flags;
-
-	/* free up the transfer buffer, as usb_free_urb() does not do this */
-	kfree(urb->transfer_buffer);
-
-	/* setup packet may be set if we're using it for writing */
-	kfree(urb->setup_packet);
-
-	if (status)
-		dbg("%s - nonzero write bulk status received: %d",
-		    __func__, status);
-
-	spin_lock_irqsave(&priv->lock, flags);
-	--priv->outstanding_urbs;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	usb_serial_port_softint(priv->port);
+	usb_serial_generic_close(port);
 }
 
 static int opticon_write(struct tty_struct *tty, struct usb_serial_port *port,
 			 const unsigned char *buf, int count)
 {
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
-	struct usb_serial *serial = port->serial;
-	struct urb *urb;
-	unsigned char *buffer;
-	unsigned long flags;
-	int status;
-
 	dbg("%s - port %d", __func__, port->number);
 
-	spin_lock_irqsave(&priv->lock, flags);
-	if (priv->outstanding_urbs > URB_UPPER_LIMIT) {
-		spin_unlock_irqrestore(&priv->lock, flags);
-		dbg("%s - write limit hit", __func__);
-		return 0;
-	}
-	priv->outstanding_urbs++;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	buffer = kmalloc(count, GFP_ATOMIC);
-	if (!buffer) {
-		dev_err(&port->dev, "out of memory\n");
-		count = -ENOMEM;
-		goto error_no_buffer;
-	}
-
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		dev_err(&port->dev, "no more free urbs\n");
-		count = -ENOMEM;
-		goto error_no_urb;
-	}
-
-	memcpy(buffer, buf, count);
-
-	usb_serial_debug_data(debug, &port->dev, __func__, count, buffer);
-
-	if (port->bulk_out_endpointAddress) {
-		usb_fill_bulk_urb(urb, serial->dev,
-				  usb_sndbulkpipe(serial->dev,
-						  port->bulk_out_endpointAddress),
-				  buffer, count, opticon_write_bulk_callback, priv);
-	} else {
-		struct usb_ctrlrequest *dr;
-
-		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
-		if (!dr)
-			return -ENOMEM;
-
-		dr->bRequestType = USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT;
-		dr->bRequest = 0x01;
-		dr->wValue = 0;
-		dr->wIndex = 0;
-		dr->wLength = cpu_to_le16(count);
-
-		usb_fill_control_urb(urb, serial->dev,
-			usb_sndctrlpipe(serial->dev, 0),
-			(unsigned char *)dr, buffer, count,
-			opticon_write_bulk_callback, priv);
-	}
-
-	/* send it down the pipe */
-	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status) {
-		dev_err(&port->dev,
-		   "%s - usb_submit_urb(write bulk) failed with status = %d\n",
-							__func__, status);
-		count = status;
-		goto error;
-	}
-
-	/* we are done with this urb, so let the host driver
-	 * really free it when it is finished with it */
-	usb_free_urb(urb);
-
-	return count;
-error:
-	usb_free_urb(urb);
-error_no_urb:
-	kfree(buffer);
-error_no_buffer:
-	spin_lock_irqsave(&priv->lock, flags);
-	--priv->outstanding_urbs;
-	spin_unlock_irqrestore(&priv->lock, flags);
-	return count;
+	if (port->bulk_out_endpointAddress)
+		return usb_serial_generic_write(tty, port, buf, count);
+	else
+		return usb_control_msg(port->serial->dev,
+				usb_sndctrlpipe(port->serial->dev, 0),
+				0x01,
+				USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT,
+				0, 0,
+				(void *)buf, count, 100);
 }
 
 static int opticon_write_room(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
-	unsigned long flags;
-
-	dbg("%s - port %d", __func__, port->number);
-
-	/*
-	 * We really can take almost anything the user throws at us
-	 * but let's pick a nice big number to tell the tty
-	 * layer that we have lots of free space, unless we don't.
-	 */
-	spin_lock_irqsave(&priv->lock, flags);
-	if (priv->outstanding_urbs > URB_UPPER_LIMIT * 2 / 3) {
-		spin_unlock_irqrestore(&priv->lock, flags);
-		dbg("%s - write limit hit", __func__);
-		return 0;
-	}
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	return 2048;
+	if (port->bulk_out_endpointAddress)
+		return usb_serial_generic_write_room(tty);
+	else
+		return 64;
 }
 
 static void opticon_throttle(struct tty_struct *tty)
 {
-	struct usb_serial_port *port = tty->driver_data;
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
-	unsigned long flags;
-
-	dbg("%s - port %d", __func__, port->number);
-	spin_lock_irqsave(&priv->lock, flags);
-	priv->throttled = true;
-	spin_unlock_irqrestore(&priv->lock, flags);
+	usb_serial_generic_throttle(tty);
 }
 
 
 static void opticon_unthrottle(struct tty_struct *tty)
 {
-	struct usb_serial_port *port = tty->driver_data;
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
-	unsigned long flags;
-	int result, was_throttled;
-
-	dbg("%s - port %d", __func__, port->number);
-
-	spin_lock_irqsave(&priv->lock, flags);
-	priv->throttled = false;
-	was_throttled = priv->actually_throttled;
-	priv->actually_throttled = false;
-	spin_unlock_irqrestore(&priv->lock, flags);
-
-	priv->bulk_read_urb->dev = port->serial->dev;
-	if (was_throttled) {
-		result = usb_submit_urb(priv->bulk_read_urb, GFP_ATOMIC);
-		if (result)
-			dev_err(&port->dev,
-				"%s - failed submitting read urb, error %d\n",
-							__func__, result);
-	}
+	usb_serial_generic_unthrottle(tty);
 }
 
 static int opticon_tiocmget(struct tty_struct *tty, struct file *file)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct opticon_private *priv = usb_get_serial_data(port->serial);
-	unsigned long flags;
 	int result = 0;
 
 	dbg("%s - port %d", __func__, port->number);
 
-	spin_lock_irqsave(&priv->lock, flags);
 	if (priv->rts)
 		result = TIOCM_RTS;
-	spin_unlock_irqrestore(&priv->lock, flags);
 
 	dbg("%s - %x", __func__, result);
 	return result;
 }
 
-static int get_serial_info(struct opticon_private *priv,
+static int get_serial_info(struct usb_serial_port *port,
 			   struct serial_struct __user *serial)
 {
 	struct serial_struct tmp;
@@ -382,9 +161,7 @@ static int get_serial_info(struct opticon_private *priv,
 
 	/* fake emulate a 16550 uart to make userspace code happy */
 	tmp.type		= PORT_16550A;
-	tmp.line		= priv->serial->minor;
-	tmp.port		= 0;
-	tmp.irq			= 0;
+	tmp.line		= port->serial->minor;
 	tmp.flags		= ASYNC_SKIP_TEST | ASYNC_AUTO_IRQ;
 	tmp.xmit_fifo_size	= 1024;
 	tmp.baud_base		= 9600;
@@ -400,13 +177,12 @@ static int opticon_ioctl(struct tty_struct *tty, struct file *file,
 			 unsigned int cmd, unsigned long arg)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct opticon_private *priv = usb_get_serial_data(port->serial);
 
 	dbg("%s - port %d, cmd = 0x%x", __func__, port->number, cmd);
 
 	switch (cmd) {
 	case TIOCGSERIAL:
-		return get_serial_info(priv,
+		return get_serial_info(port,
 				       (struct serial_struct __user *)arg);
 	}
 
@@ -416,10 +192,12 @@ static int opticon_ioctl(struct tty_struct *tty, struct file *file,
 static int opticon_startup(struct usb_serial *serial)
 {
 	struct opticon_private *priv;
-	struct usb_host_interface *intf;
-	int i;
-	int retval = -ENOMEM;
-	bool bulk_in_found = false;
+
+	if (serial->num_bulk_in != 1 || serial->num_bulk_out > 1) {
+		dev_err(&serial->dev->dev,
+			"Error - the proper endpoints were not found!\n");
+		return -EINVAL;
+	}
 
 	/* create our private serial structure */
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -427,70 +205,15 @@ static int opticon_startup(struct usb_serial *serial)
 		dev_err(&serial->dev->dev, "%s - Out of memory\n", __func__);
 		return -ENOMEM;
 	}
-	spin_lock_init(&priv->lock);
-	priv->serial = serial;
-	priv->port = serial->port[0];
-	priv->udev = serial->dev;
-
-	/* find our bulk endpoint */
-	intf = serial->interface->altsetting;
-	for (i = 0; i < intf->desc.bNumEndpoints; ++i) {
-		struct usb_endpoint_descriptor *endpoint;
-
-		endpoint = &intf->endpoint[i].desc;
-		if (!usb_endpoint_is_bulk_in(endpoint))
-			continue;
-
-		priv->bulk_read_urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!priv->bulk_read_urb) {
-			dev_err(&priv->udev->dev, "out of memory\n");
-			goto error;
-		}
-
-		priv->buffer_size = le16_to_cpu(endpoint->wMaxPacketSize) * 2;
-		priv->bulk_in_buffer = kmalloc(priv->buffer_size, GFP_KERNEL);
-		if (!priv->bulk_in_buffer) {
-			dev_err(&priv->udev->dev, "out of memory\n");
-			goto error;
-		}
-
-		priv->bulk_address = endpoint->bEndpointAddress;
-
-		/* set up our bulk urb */
-		usb_fill_bulk_urb(priv->bulk_read_urb, priv->udev,
-				  usb_rcvbulkpipe(priv->udev,
-						  endpoint->bEndpointAddress),
-				  priv->bulk_in_buffer, priv->buffer_size,
-				  opticon_bulk_callback, priv);
-
-		bulk_in_found = true;
-		break;
-		}
-
-	if (!bulk_in_found) {
-		dev_err(&priv->udev->dev,
-			"Error - the proper endpoints were not found!\n");
-		goto error;
-	}
 
 	usb_set_serial_data(serial, priv);
 	return 0;
-
-error:
-	usb_free_urb(priv->bulk_read_urb);
-	kfree(priv->bulk_in_buffer);
-	kfree(priv);
-	return retval;
 }
 
 static void opticon_disconnect(struct usb_serial *serial)
 {
-	struct opticon_private *priv = usb_get_serial_data(serial);
-
 	dbg("%s", __func__);
-
-	usb_kill_urb(priv->bulk_read_urb);
-	usb_free_urb(priv->bulk_read_urb);
+	usb_serial_generic_disconnect(serial);
 }
 
 static void opticon_release(struct usb_serial *serial)
@@ -499,44 +222,17 @@ static void opticon_release(struct usb_serial *serial)
 
 	dbg("%s", __func__);
 
-	kfree(priv->bulk_in_buffer);
 	kfree(priv);
 }
 
-static int opticon_suspend(struct usb_interface *intf, pm_message_t message)
-{
-	struct usb_serial *serial = usb_get_intfdata(intf);
-	struct opticon_private *priv = usb_get_serial_data(serial);
-
-	usb_kill_urb(priv->bulk_read_urb);
-	return 0;
-}
-
-static int opticon_resume(struct usb_interface *intf)
-{
-	struct usb_serial *serial = usb_get_intfdata(intf);
-	struct opticon_private *priv = usb_get_serial_data(serial);
-	struct usb_serial_port *port = serial->port[0];
-	int result;
-
-	mutex_lock(&port->port.mutex);
-	/* This is protected by the port mutex against close/open */
-	if (test_bit(ASYNCB_INITIALIZED, &port->port.flags))
-		result = usb_submit_urb(priv->bulk_read_urb, GFP_NOIO);
-	else
-		result = 0;
-	mutex_unlock(&port->port.mutex);
-	return result;
-}
-
 static struct usb_driver opticon_driver = {
 	.name =		"opticon",
 	.probe =	usb_serial_probe,
 	.disconnect =	usb_serial_disconnect,
-	.suspend =	opticon_suspend,
-	.resume =	opticon_resume,
+	.suspend =	usb_serial_suspend,
+	.resume =	usb_serial_resume,
 	.id_table =	id_table,
-	.no_dynamic_id = 	1,
+	.no_dynamic_id =	1,
 };
 
 static struct usb_serial_driver opticon_device = {
@@ -545,16 +241,18 @@ static struct usb_serial_driver opticon_device = {
 		.name =		"opticon",
 	},
 	.id_table =		id_table,
-	.usb_driver = 		&opticon_driver,
+	.usb_driver =		&opticon_driver,
 	.num_ports =		1,
+	.bulk_out_size =	1024,
 	.attach =		opticon_startup,
 	.open =			opticon_open,
 	.close =		opticon_close,
 	.write =		opticon_write,
-	.write_room = 		opticon_write_room,
+	.write_room =		opticon_write_room,
+	.process_read_urb =	opticon_process_read_urb,
 	.disconnect =		opticon_disconnect,
 	.release =		opticon_release,
-	.throttle = 		opticon_throttle,
+	.throttle =		opticon_throttle,
 	.unthrottle =		opticon_unthrottle,
 	.ioctl =		opticon_ioctl,
 	.tiocmget =		opticon_tiocmget,
-- 
1.7.0.4