Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port

[Peter Hurley]

Hi Baolin,

On 11/16/2015 02:05 AM, Baolin Wang wrote:
> It dose not work when we want to use the usb-to-serial port based
> on one usb gadget as a console. Thus this patch adds the console
> initialization to support this request.

I have some high level concerns.

1. I would defer registering the console until the port has at least been
   allocated in gserial_alloc_line(), and unregister when the line is freed.
   That also reduces many of the conditions that you shouldn't need to
   check, like port number range and so on.

   Further, consider deferring the console registration until gserial_connect();
   that would further simplify things. In this case, unregistration would
   happen at disconnect.

2. Why are you using a kworker for actual device i/o when all of the i/o
   is done with interrupts disabled anyway?
   Getting rid of the work would eliminate using the 8K intermediate buffer
   as well.

3. If for some reason i/o deferral is really necessary, consider using a kthread
   kworker instead of the pooled kworker. The scheduler response will be _way_
   better.

4. If for some reason i/o deferral is really necessary, use a circular buffer
   for the intermediate buffer, preferably lockless since there is only
   one producer and one consumer.

Some other review comments below; please ignore anything other reviewers
have already noted.

Regards,
Peter Hurley

> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
> ---
>  drivers/usb/gadget/Kconfig             |    6 +
>  drivers/usb/gadget/function/u_serial.c |  238 ++++++++++++++++++++++++++++++++
>  2 files changed, 244 insertions(+)
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 33834aa..be5aab9 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
>  	   a module parameter as well.
>  	   If unsure, say 2.
>  
> +config U_SERIAL_CONSOLE
> +	bool "Serial gadget console support"
> +	depends on USB_G_SERIAL
> +	help
> +	   It supports the serial gadget can be used as a console.
> +
>  source "drivers/usb/gadget/udc/Kconfig"
>  
>  #
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index f7771d8..4ade527 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -27,6 +27,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
> +#include <linux/console.h>
>  
>  #include "u_serial.h"
>  
> @@ -79,6 +80,16 @@
>   */
>  #define QUEUE_SIZE		16
>  #define WRITE_BUF_SIZE		8192		/* TX only */
> +#define GS_BUFFER_SIZE		(4096)
> +#define GS_CONSOLE_BUF_SIZE	(2 * GS_BUFFER_SIZE)
> +
> +struct gscons_info {
> +	struct gs_port		*port;
> +	struct tty_driver	*tty_driver;
> +	struct work_struct	work;
> +	int			buf_tail;
> +	char			buf[GS_CONSOLE_BUF_SIZE];
> +};

Make struct gscons_info a static declaration instead of
allocating it.

>  
>  /* circular buffer */
>  struct gs_buf {
> @@ -118,6 +129,7 @@ struct gs_port {
>  
>  	/* REVISIT this state ... */
>  	struct usb_cdc_line_coding port_line_coding;	/* 8-N-1 etc */
> +	struct usb_request	*console_req;
>  };
>  
>  static struct portmaster {
> @@ -1054,6 +1066,7 @@ gs_port_alloc(unsigned port_num, struct usb_cdc_line_coding *coding)
>  
>  	port->port_num = port_num;
>  	port->port_line_coding = *coding;
> +	port->console_req = NULL;
>  
>  	ports[port_num].port = port;
>  out:
> @@ -1143,6 +1156,227 @@ err:
>  }
>  EXPORT_SYMBOL_GPL(gserial_alloc_line);
>  
> +#ifdef CONFIG_U_SERIAL_CONSOLE
> +
> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
                                                                ^^^^^^^^^^^^^^^
With only a single caller that uses a symbolic constant, is the
'buffer_size' parameter necessary?


> +{
> +	struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +

Remove this newline.

> +	if (!req)
> +		return NULL;
> +
> +	/* now allocate buffers for the requests */

Unnecessary comment.

> +	req->buf = kmalloc(buffer_size, GFP_ATOMIC);
> +	if (!req->buf) {
> +		usb_ep_free_request(ep, req);
> +		return NULL;
> +	}
> +
> +	return req;
> +}
> +
> +static void gs_request_free(struct usb_request *req, struct usb_ep *ep)
> +{
> +	if (req) {
> +		kfree(req->buf);
> +		usb_ep_free_request(ep, req);
> +	}
> +}
> +
> +static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
> +{
> +	if (req->status != 0 && req->status != -ECONNRESET)
> +		return;
> +}
> +
> +static struct console gserial_cons;
> +static int gs_console_connect(void)

Pass the console * as parameter, instead of forward declaring the console.
Or initialize info directly from the static struct gscons_info address.

> +{
> +	struct gscons_info *info = gserial_cons.data;
> +	int port_num = gserial_cons.index;
> +	struct usb_request *req;
> +	struct gs_port *port;
> +	struct usb_ep *ep;
> +
> +	if (port_num >= MAX_U_SERIAL_PORTS || port_num < 0) {
> +		pr_err("%s: port num [%d] exceeds the range.\n",
> +		       __func__, port_num);
> +		return -ENXIO;
> +	}
> +
> +	port = ports[port_num].port;
> +	if (!port) {
> +		pr_err("%s: serial line [%d] not allocated.\n",
> +		       __func__, port_num);
> +		return -ENODEV;
> +	}
> +
> +	if (!port->port_usb) {
> +		pr_err("%s: no port usb.\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	ep = port->port_usb->in;
> +	if (!ep) {
> +		pr_err("%s: no usb endpoint.\n", __func__);
> +		return -ENXIO;
> +	}

Looking at the caller, gserial_connect(), none of the error
conditions above look possible.


> +
> +	req = port->console_req;
> +	if (!req) {
> +		req = gs_request_new(ep, GS_BUFFER_SIZE);

Where is port->console_req assigned to?

> +		if (!req) {
> +			pr_err("%s: request fail.\n", __func__);

Remove redundant error message; the allocator has already done so.

> +			return -ENOMEM;
> +		}
> +		req->complete = gs_complete_out;
> +	}
> +
> +	info->port = port;
> +
> +	pr_debug("%s: port[%d] console connect!\n", __func__, port_num);
> +	return 0;
> +}
> +
> +static void gs_console_work(struct work_struct *work)
> +{
> +	struct gscons_info *info = container_of(work, struct gscons_info, work);
> +	struct gs_port *port = info->port;
> +	struct usb_request *req;
> +	struct usb_ep *ep;
> +	int xfer, ret, count;
> +	char *p;
> +
> +	if (!port || !port->port_usb)
> +		return;
> +
> +	req = port->console_req;
> +	ep = port->port_usb->in;
> +	if (!req || !ep)
> +		return;
> +
> +	spin_lock_irq(&port->port_lock);
> +	count = info->buf_tail;
> +	p = info->buf;
> +
> +	while (count > 0 && !port->write_busy) {
> +		if (count > GS_BUFFER_SIZE)
> +			xfer = GS_BUFFER_SIZE;
> +		else
> +			xfer = count;
> +
> +		memcpy(req->buf, p, xfer);
> +		req->length = xfer;
> +
> +		port->write_busy = true;
> +		spin_unlock(&port->port_lock);
> +		ret = usb_ep_queue(ep, req, GFP_ATOMIC);
> +		spin_lock(&port->port_lock);
> +		port->write_busy = false;
> +		if (ret < 0)
> +			break;
> +
> +		p += xfer;
> +		count -= xfer;
> +	}
> +
> +	info->buf_tail -= count;
> +	spin_unlock_irq(&port->port_lock);
> +}
> +
> +static int gs_console_setup(struct console *co, char *options)
> +{
> +	struct gscons_info *gscons_info;
> +
> +	gscons_info = kzalloc(sizeof(struct gscons_info), GFP_KERNEL);
> +	if (!gscons_info)
> +		return -ENOMEM;
> +
> +	gscons_info->port = NULL;

Redundant.

> +	gscons_info->tty_driver = gs_tty_driver;
> +	INIT_WORK(&gscons_info->work, gs_console_work);
> +	gscons_info->buf_tail = 0;

Redundant.

> +	co->data = gscons_info;
> +
> +	return 0;
> +}
> +
> +static void gs_console_write(struct console *co,
> +			     const char *buf, unsigned count)
> +{
> +	struct gscons_info *info = co->data;
> +	int avail, xfer;
> +	char *p;
> +
> +	avail = GS_CONSOLE_BUF_SIZE - info->buf_tail;
> +	if (count > avail)
> +		xfer = avail;
> +	else
> +		xfer = count;

	xfer = min(count, GS_CONSOLE_BUF_SIZE - info->buf_tail);

> +
> +	p = &info->buf[info->buf_tail];
> +	memcpy(p, buf, xfer);
> +	info->buf_tail += xfer;

What is preventing concurrently running work and this from
using/modifying the info->buf and info->buf_tail simultaneously?

> +
> +	schedule_work(&info->work);
> +}
> +
> +static struct tty_driver *gs_console_device(struct console *co, int *index)
> +{
> +	struct gscons_info *info = co->data;
> +
> +	*index = co->index;
> +	return info->tty_driver;
> +}
> +
> +static struct console gserial_cons = {
> +	.name =		"ttyGS",
> +	.write =	gs_console_write,
> +	.device =	gs_console_device,
> +	.setup =	gs_console_setup,
> +	.flags =	CON_PRINTBUFFER,
> +	.index =	-1,
> +};
> +
> +static void gserial_console_init(void)
> +{
> +	register_console(&gserial_cons);
> +}
> +
> +static void gserial_console_exit(void)
> +{
> +	struct gscons_info *info = gserial_cons.data;
> +	struct gs_port *port = info->port;
> +	struct usb_request *req;
> +	struct usb_ep *ep;
> +
> +	if (port && port->port_usb) {
> +		req = port->console_req;
> +		ep = port->port_usb->in;
> +		gs_request_free(req, ep);
> +	}
> +
> +	kfree(info);
> +	unregister_console(&gserial_cons);
> +}
> +
> +#else
> +
> +static int gs_console_connect(void)
> +{
> +	return 0;
> +}
> +
> +static void gserial_console_init(void)
> +{
> +}
> +
> +static void gserial_console_exit(void)
> +{
> +}
> +
> +#endif
> +
>  /**
>   * gserial_connect - notify TTY I/O glue that USB link is active
>   * @gser: the function, set up with endpoints and descriptors
> @@ -1219,6 +1453,7 @@ int gserial_connect(struct gserial *gser, u8 port_num)
>  			gser->disconnect(gser);
>  	}
>  
> +	status = gs_console_connect();
>  	spin_unlock_irqrestore(&port->port_lock, flags);
>  
>  	return status;
> @@ -1320,6 +1555,8 @@ static int userial_init(void)
>  		goto fail;
>  	}
>  
> +	gserial_console_init();
> +
>  	pr_debug("%s: registered %d ttyGS* device%s\n", __func__,
>  			MAX_U_SERIAL_PORTS,
>  			(MAX_U_SERIAL_PORTS == 1) ? "" : "s");
> @@ -1334,6 +1571,7 @@ module_init(userial_init);
>  
>  static void userial_cleanup(void)
>  {
> +	gserial_console_exit();
>  	tty_unregister_driver(gs_tty_driver);
>  	put_tty_driver(gs_tty_driver);
>  	gs_tty_driver = NULL;
> 

--
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/