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

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

[Baolin Wang]

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.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 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];
+};
 
 /* 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)
+{
+	struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+
+	if (!req)
+		return NULL;
+
+	/* now allocate buffers for the requests */
+	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)
+{
+	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;
+	}
+
+	req = port->console_req;
+	if (!req) {
+		req = gs_request_new(ep, GS_BUFFER_SIZE);
+		if (!req) {
+			pr_err("%s: request fail.\n", __func__);
+			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;
+	gscons_info->tty_driver = gs_tty_driver;
+	INIT_WORK(&gscons_info->work, gs_console_work);
+	gscons_info->buf_tail = 0;
+	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;
+
+	p = &info->buf[info->buf_tail];
+	memcpy(p, buf, xfer);
+	info->buf_tail += xfer;
+
+	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;
-- 
1.7.9.5

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

[Andy Shevchenko]

On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> 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.


> @@ -79,6 +80,16 @@
>   */
>  #define QUEUE_SIZE             16
>  #define WRITE_BUF_SIZE         8192            /* TX only */
> +#define GS_BUFFER_SIZE         (4096)

Redundant parens

> +#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];

Can't be malloced once?


> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
> +{
> +       struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +
> +       if (!req)

For sake of readability it's better to have assignment explicitly before 'if'.

> +               return NULL;
> +
> +       /* now allocate buffers for the requests */
> +       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) {

if (!req)
 return;

?

> +               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;

Something missed here. Currently it's no-op.

> +}
> +
> +static struct console gserial_cons;
> +static int gs_console_connect(void)
> +{
> +       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__);

Starting from here could it be dev_err and so on?

> +               return -ENODEV;
> +       }
> +
> +       ep = port->port_usb->in;
> +       if (!ep) {
> +               pr_err("%s: no usb endpoint.\n", __func__);
> +               return -ENXIO;
> +       }
> +
> +       req = port->console_req;
> +       if (!req) {
> +               req = gs_request_new(ep, GS_BUFFER_SIZE);
> +               if (!req) {
> +                       pr_err("%s: request fail.\n", __func__);
> +                       return -ENOMEM;
> +               }
> +               req->complete = gs_complete_out;
> +       }
> +
> +       info->port = port;
> +
> +       pr_debug("%s: port[%d] console connect!\n", __func__, port_num);

Dynamic debug will add function name if asked.

> +       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;

xfer = min_t(…, count, GS_BUFFER_SIZE);

> +
> +               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;
> +       gscons_info->tty_driver = gs_tty_driver;
> +       INIT_WORK(&gscons_info->work, gs_console_work);
> +       gscons_info->buf_tail = 0;
> +       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;

Ditto.

> +
> +       p = &info->buf[info->buf_tail];
> +       memcpy(p, buf, xfer);
> +       info->buf_tail += xfer;
> +
> +       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;
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

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

[Baolin Wang]

On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> 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.
>
>
>> @@ -79,6 +80,16 @@
>>   */
>>  #define QUEUE_SIZE             16
>>  #define WRITE_BUF_SIZE         8192            /* TX only */
>> +#define GS_BUFFER_SIZE         (4096)
>
> Redundant parens
>

OK. I'll remove it.

>> +#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];
>
> Can't be malloced once?
>

The 'gscons_info' structure is malloced once.

>
>> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
>> +{
>> +       struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>> +
>> +       if (!req)
>
> For sake of readability it's better to have assignment explicitly before 'if'.

But I think it is very easy to understand the assignment here with
saving code lines.

>
>> +               return NULL;
>> +
>> +       /* now allocate buffers for the requests */
>> +       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) {
>
> if (!req)
>  return;
>
> ?

Make sense.

>
>> +               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;
>
> Something missed here. Currently it's no-op.
>

Yeah. I didn't realize what need to do in the callback here, so just
leave a callback without anything. But maybe something will be added
if there are some requirements in future.

>> +}
>> +
>> +static struct console gserial_cons;
>> +static int gs_console_connect(void)
>> +{
>> +       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__);
>
> Starting from here could it be dev_err and so on?

There are no dev_err things and device things in this file, so pr_xxx
is more reasonable.

>
>> +               return -ENODEV;
>> +       }
>> +
>> +       ep = port->port_usb->in;
>> +       if (!ep) {
>> +               pr_err("%s: no usb endpoint.\n", __func__);
>> +               return -ENXIO;
>> +       }
>> +
>> +       req = port->console_req;
>> +       if (!req) {
>> +               req = gs_request_new(ep, GS_BUFFER_SIZE);
>> +               if (!req) {
>> +                       pr_err("%s: request fail.\n", __func__);
>> +                       return -ENOMEM;
>> +               }
>> +               req->complete = gs_complete_out;
>> +       }
>> +
>> +       info->port = port;
>> +
>> +       pr_debug("%s: port[%d] console connect!\n", __func__, port_num);
>
> Dynamic debug will add function name if asked.

Sorry, I didn't get your point, you mean print the function name is
redundant here?

>
>> +       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;
>
> xfer = min_t(…, count, GS_BUFFER_SIZE);

That's right and I'll fix that.

>
>> +
>> +               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;
>> +       gscons_info->tty_driver = gs_tty_driver;
>> +       INIT_WORK(&gscons_info->work, gs_console_work);
>> +       gscons_info->buf_tail = 0;
>> +       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;
>
> Ditto.

OK.

>
>> +
>> +       p = &info->buf[info->buf_tail];
>> +       memcpy(p, buf, xfer);
>> +       info->buf_tail += xfer;
>> +
>> +       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;
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
Baolin.wang
Best Regards

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

[Andy Shevchenko]

On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> 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.
>>

>>> +#define GS_BUFFER_SIZE         (4096)
>> Redundant parens
> OK. I'll remove it.
>
>>> +#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];
>>
>> Can't be malloced once?
> The 'gscons_info' structure is malloced once.

In state of high fragmentation is quite hard to find big memory chunks.
I would split it to two allocations, though if maintainers are okay
with your code, then I'm also okay.

>>> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
>>> +{
>>> +       struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>> +
>>> +       if (!req)
>>
>> For sake of readability it's better to have assignment explicitly before 'if'.
>
> But I think it is very easy to understand the assignment here with
> saving code lines.

It's not a function of couple of lines, so, for me makes sense to
explicitly put the assignment here. Especially that one that does
allocations (for pointer arithmetic I could agree to place the
assignment in the definition block).

>>> +static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
>>> +{
>>> +       if (req->status != 0 && req->status != -ECONNRESET)
>>> +               return;
>>
>> Something missed here. Currently it's no-op.
>>
>
> Yeah. I didn't realize what need to do in the callback here, so just
> leave a callback without anything. But maybe something will be added
> if there are some requirements in future.

if ()
..

will be optimized away, why not to remove it?

>>> +       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__);
>>
>> Starting from here could it be dev_err and so on?
>
> There are no dev_err things and device things in this file, so pr_xxx
> is more reasonable.

This is understandable, but if in case you have device in place why
not to use its name?

>>> +       pr_debug("%s: port[%d] console connect!\n", __func__, port_num);
>>
>> Dynamic debug will add function name if asked.
>
> Sorry, I didn't get your point, you mean print the function name is
> redundant here?

Right.

Just pr_debug("port[%d] …", …);

-- 
With Best Regards,
Andy Shevchenko

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

[Baolin Wang]

On 18 November 2015 at 17:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> 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.
>>>
>
>>>> +#define GS_BUFFER_SIZE         (4096)
>>> Redundant parens
>> OK. I'll remove it.
>>
>>>> +#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];
>>>
>>> Can't be malloced once?
>> The 'gscons_info' structure is malloced once.
>
> In state of high fragmentation is quite hard to find big memory chunks.
> I would split it to two allocations, though if maintainers are okay
> with your code, then I'm also okay.
>

Make sense. But I think the major memory of the 'struct gscons_info'
is for the 'buf' member, so I still think no need to allocate it 2
times.

>>>> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
>>>> +{
>>>> +       struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>>> +
>>>> +       if (!req)
>>>
>>> For sake of readability it's better to have assignment explicitly before 'if'.
>>
>> But I think it is very easy to understand the assignment here with
>> saving code lines.
>
> It's not a function of couple of lines, so, for me makes sense to
> explicitly put the assignment here. Especially that one that does
> allocations (for pointer arithmetic I could agree to place the
> assignment in the definition block).
>

OK. Sounds reasonable.

>>>> +static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
>>>> +{
>>>> +       if (req->status != 0 && req->status != -ECONNRESET)
>>>> +               return;
>>>
>>> Something missed here. Currently it's no-op.
>>>
>>
>> Yeah. I didn't realize what need to do in the callback here, so just
>> leave a callback without anything. But maybe something will be added
>> if there are some requirements in future.
>
> if ()
> ..
>
> will be optimized away, why not to remove it?

OK. I'll remove it.

>
>>>> +       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__);
>>>
>>> Starting from here could it be dev_err and so on?
>>
>> There are no dev_err things and device things in this file, so pr_xxx
>> is more reasonable.
>
> This is understandable, but if in case you have device in place why
> not to use its name?

Yes, that's right.

>
>>>> +       pr_debug("%s: port[%d] console connect!\n", __func__, port_num);
>>>
>>> Dynamic debug will add function name if asked.
>>
>> Sorry, I didn't get your point, you mean print the function name is
>> redundant here?
>
> Right.
>
> Just pr_debug("port[%d] …", …);
>

OK. Very thanks for your suggestions.

> --
> With Best Regards,
> Andy Shevchenko



-- 
Baolin.wang
Best Regards

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

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1821 bytes --]

From: Baolin Wang
> Sent: 18 November 2015 10:45
> On 18 November 2015 at 17:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> >> On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> 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.
> >>>
> >
> >>>> +#define GS_BUFFER_SIZE         (4096)
> >>> Redundant parens
> >> OK. I'll remove it.
> >>
> >>>> +#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];
> >>>
> >>> Can't be malloced once?
> >> The 'gscons_info' structure is malloced once.
> >
> > In state of high fragmentation is quite hard to find big memory chunks.
> > I would split it to two allocations, though if maintainers are okay
> > with your code, then I'm also okay.
> >
> 
> Make sense. But I think the major memory of the 'struct gscons_info'
> is for the 'buf' member, so I still think no need to allocate it 2
> times.

It may be worth just reducing GS_BUFFER_SIZE slightly so that the gscons_info
structure itself is less than 8k.
If you can't get 2 adjacent pages then a lot of things will fail.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Baolin Wang]

On 18 November 2015 at 20:05, David Laight <David.Laight@aculab.com> wrote:
> From: Baolin Wang
>> Sent: 18 November 2015 10:45
>> On 18 November 2015 at 17:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> > On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> >> On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> >>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> 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.
>> >>>
>> >
>> >>>> +#define GS_BUFFER_SIZE         (4096)
>> >>> Redundant parens
>> >> OK. I'll remove it.
>> >>
>> >>>> +#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];
>> >>>
>> >>> Can't be malloced once?
>> >> The 'gscons_info' structure is malloced once.
>> >
>> > In state of high fragmentation is quite hard to find big memory chunks.
>> > I would split it to two allocations, though if maintainers are okay
>> > with your code, then I'm also okay.
>> >
>>
>> Make sense. But I think the major memory of the 'struct gscons_info'
>> is for the 'buf' member, so I still think no need to allocate it 2
>> times.
>
> It may be worth just reducing GS_BUFFER_SIZE slightly so that the gscons_info
> structure itself is less than 8k.
> If you can't get 2 adjacent pages then a lot of things will fail.
>

But its allocation is called in early booting time, I think there are
not many memory fragments now.

>         David
>



-- 
Baolin.wang
Best Regards

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@linaro.org>
> ---
>  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;
> 

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

[Baolin Wang]

On 18 November 2015 at 23:32, Peter Hurley <peter@hurleysoftware.com> wrote:
> 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.

The 'setup' callback of 'struct console' is just do some memory
allocation and member initialization, that no need to defer
registering the console in gserial_alloc_line(). But the
'gs_console_connect()' which will use the port need to be called in
gserial_connect().

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

That sounds reasonable. I would try.

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

If remove the kworker, there are some deadlocks to call the printk()
when in the process of transferring data with usb endpoint. So we need
a async kworker to complete the io or it can not work.

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

OK, make sense.

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

Yeah, the circular buffer is better but more tricky. I would try.

> Some other review comments below; please ignore anything other reviewers
> have already noted.
>
> Regards,
> Peter Hurley
>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  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.

But I think the dynamic allocation is more reasonable with reducing
some global variables.

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

Yeah, I'll remove the 'buffer_size' parameter.

>
>> +{
>> +     struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>> +
>
> Remove this newline.

OK.

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

OK.

>
>> +     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.

Make sense.

>
>> +{
>> +     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.

That's right. I'll remove these checks.

>
>
>> +
>> +     req = port->console_req;
>> +     if (!req) {
>> +             req = gs_request_new(ep, GS_BUFFER_SIZE);
>
> Where is port->console_req assigned to?

The connect may be called several times, if the req is allocated at
one time, there is no need to assign it.

>
>> +             if (!req) {
>> +                     pr_err("%s: request fail.\n", __func__);
>
> Remove redundant error message; the allocator has already done so.

OK.

>
>> +                     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.

Will remove it.

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

Will remove it.

>
>> +     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);

Yeah, that's right.

>
>> +
>> +     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?

Like I said above, it will meet deadlocks if running the work
directly, then introduce the kworker.

>
>> +
>> +     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;
>>
>



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

>
>> +{
>> +     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.
>

I re-look the code and do some tests, I found the checking is
necessary. Cause we get the port number from the console->index, if
the cmdline is not set the ttyGS0 as the console, the console->index
will be -1 that is a wrong value. Also the serial.c file will create 1
usb-to-seial port as default (default n_ports = 1), so we need to
check the port and the endpoint of the port. So I think here checking
is necessary and I have tested it.



-- 
Baolin.wang
Best Regards

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

[Peter Hurley]

On 11/19/2015 01:48 AM, Baolin Wang wrote:
>>
>>> +{
>>> +     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.
>>
> 
> I re-look the code and do some tests, I found the checking is
> necessary. Cause we get the port number from the console->index, if
> the cmdline is not set the ttyGS0 as the console, the console->index
> will be -1 that is a wrong value. Also the serial.c file will create 1
> usb-to-seial port as default (default n_ports = 1), so we need to
> check the port and the endpoint of the port. So I think here checking
> is necessary and I have tested it.

static void gs_console_connect(int port_num)
{
	.
	.
	if (port_num != gserial_cons.index)
		return;
	.
	.


@@ -1219,6 +1453,7 @@ int gserial_connect(struct gserial *gser, u8 port_num)
 			gser->disconnect(gser);
 	}
 
+	status = gs_console_connect(port_num);
 	spin_unlock_irqrestore(&port->port_lock, flags);
 
 	return status;

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

[Baolin Wang]

On 19 November 2015 at 17:36, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 11/19/2015 01:48 AM, Baolin Wang wrote:
>>>
>>>> +{
>>>> +     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.
>>>
>>
>> I re-look the code and do some tests, I found the checking is
>> necessary. Cause we get the port number from the console->index, if
>> the cmdline is not set the ttyGS0 as the console, the console->index
>> will be -1 that is a wrong value. Also the serial.c file will create 1
>> usb-to-seial port as default (default n_ports = 1), so we need to
>> check the port and the endpoint of the port. So I think here checking
>> is necessary and I have tested it.
>
> static void gs_console_connect(int port_num)
> {
>         .
>         .
>         if (port_num != gserial_cons.index)
>                 return;
>         .

OK. Thanks.

>         .
>
>
> @@ -1219,6 +1453,7 @@ int gserial_connect(struct gserial *gser, u8 port_num)
>                         gser->disconnect(gser);
>         }
>
> +       status = gs_console_connect(port_num);
>         spin_unlock_irqrestore(&port->port_lock, flags);
>
>         return status;
>
>
>
>
>
>
>



-- 
Baolin.wang
Best Regards

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

[Peter Hurley]

On 11/18/2015 09:35 PM, Baolin Wang wrote:
> On 18 November 2015 at 23:32, Peter Hurley <peter@hurleysoftware.com> wrote:
>> 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.
> 
> The 'setup' callback of 'struct console' is just do some memory
> allocation and member initialization, that no need to defer
> registering the console in gserial_alloc_line(). But the
> 'gs_console_connect()' which will use the port need to be called in
> gserial_connect().

My point here was why are you registering the console before the port
table is even allocated and initialized?  The console can't possibly
perform i/o that early because the port doesn't even exist.
Which is why I suggested waiting until gserial_alloc_line() to
register the console.

A typical console setup() performs the cross-reference linking between
the console data structure and the port table. The reason for that
is the console needs to be cleaned up if the port is being torn down.

For example, in gserial_disconnect() the port->port_usb is reset to NULL,
and later in gserial_console_exit():

	if (port && port->port_usb) {
		....
		gs_request_free(req, ep);
	}

which means your leaking the request allocation when the port has been
disconnected.


>>    Further, consider deferring the console registration until gserial_connect();
>>    that would further simplify things. In this case, unregistration would
>>    happen at disconnect.
> 
> That sounds reasonable. I would try.
> 
>>
>> 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.
> 
> If remove the kworker, there are some deadlocks to call the printk()
> when in the process of transferring data with usb endpoint. So we need
> a async kworker to complete the io or it can not work.

The commit log should detail the major design choices, including why you
used the kworker (because of re-entrancy issues with usb endpoint).



>> 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.
>>
> 
> OK, make sense.
> 
>> 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.
>>
> 
> Yeah, the circular buffer is better but more tricky. I would try.
> 
>> Some other review comments below; please ignore anything other reviewers
>> have already noted.
>>
>> Regards,
>> Peter Hurley
>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>>  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.
> 
> But I think the dynamic allocation is more reasonable with reducing
> some global variables.

But introduces a failure mode that doesn't exist with the
static definition.


>>>  /* 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?
>>
> 
> Yeah, I'll remove the 'buffer_size' parameter.
> 
>>
>>> +{
>>> +     struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>> +
>>
>> Remove this newline.
> 
> OK.
> 
>>
>>> +     if (!req)
>>> +             return NULL;
>>> +
>>> +     /* now allocate buffers for the requests */
>>
>> Unnecessary comment.
> 
> OK.
> 
>>
>>> +     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.
> 
> Make sense.
> 
>>
>>> +{
>>> +     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.
> 
> That's right. I'll remove these checks.
> 
>>
>>
>>> +
>>> +     req = port->console_req;
>>> +     if (!req) {
>>> +             req = gs_request_new(ep, GS_BUFFER_SIZE);
>>
>> Where is port->console_req assigned to?
> 
> The connect may be called several times, if the req is allocated at
> one time, there is no need to assign it.

You've missed my point: where is

		port->console_req = ?????


>>> +             if (!req) {
>>> +                     pr_err("%s: request fail.\n", __func__);
>>
>> Remove redundant error message; the allocator has already done so.
> 
> OK.
> 
>>
>>> +                     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;

I'm not seeing how info->buf_tail is ever reset to 0.

	count = info->buf_tail

	while (count > 0) {
		....
		count -= xfer;
	}

At loop exit count == 0, so

	info->buf_tail -= count;

never decreases buf_tail?


>>> +     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.
> 
> Will remove it.
> 
>>
>>> +     gscons_info->tty_driver = gs_tty_driver;
>>> +     INIT_WORK(&gscons_info->work, gs_console_work);
>>> +     gscons_info->buf_tail = 0;
>>
>> Redundant.
> 
> Will remove it.
> 
>>
>>> +     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);
> 
> Yeah, that's right.
> 
>>
>>> +
>>> +     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?
> 
> Like I said above, it will meet deadlocks if running the work
> directly, then introduce the kworker.

You've missed my point here:

CPU 0                                 CPU 1
--------------------------------      -------------------------------
gs_console_write()                    gs_console_work()

   info->buf_tail += xfer                info->buf_tail -= count;


Neither of these operations are atomic so what will the value of
info->buf_tail be? For example:

   A <= LOAD(info->buf_tail)
                                         B <= LOAD(info->buf_tail)
   A <= A + xfer                         B <= B - count
   STORE(A, info->buf_tail)
                                         STORE(B, info->buf_tail)

The result is as if info->buf_tail += xfer never happened.


>>> +
>>> +     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;
>>>
>>
> 
> 
> 

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

[Baolin Wang]

On 19 November 2015 at 18:28, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 11/18/2015 09:35 PM, Baolin Wang wrote:
>> On 18 November 2015 at 23:32, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> 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.
>>
>> The 'setup' callback of 'struct console' is just do some memory
>> allocation and member initialization, that no need to defer
>> registering the console in gserial_alloc_line(). But the
>> 'gs_console_connect()' which will use the port need to be called in
>> gserial_connect().
>
> My point here was why are you registering the console before the port
> table is even allocated and initialized?  The console can't possibly
> perform i/o that early because the port doesn't even exist.
> Which is why I suggested waiting until gserial_alloc_line() to
> register the console.
>
> A typical console setup() performs the cross-reference linking between
> the console data structure and the port table. The reason for that
> is the console needs to be cleaned up if the port is being torn down.
>
> For example, in gserial_disconnect() the port->port_usb is reset to NULL,
> and later in gserial_console_exit():
>
>         if (port && port->port_usb) {
>                 ....
>                 gs_request_free(req, ep);
>         }
>
> which means your leaking the request allocation when the port has been
> disconnected.
>

Yeah, that's right. I'll defer the console registration until
gserial_connect() and unregistration in disconnect.

>
>>>    Further, consider deferring the console registration until gserial_connect();
>>>    that would further simplify things. In this case, unregistration would
>>>    happen at disconnect.
>>
>> That sounds reasonable. I would try.
>>
>>>
>>> 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.
>>
>> If remove the kworker, there are some deadlocks to call the printk()
>> when in the process of transferring data with usb endpoint. So we need
>> a async kworker to complete the io or it can not work.
>
> The commit log should detail the major design choices, including why you
> used the kworker (because of re-entrancy issues with usb endpoint).
>

OK.

>
>
>>> 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.
>>>
>>
>> OK, make sense.
>>
>>> 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.
>>>
>>
>> Yeah, the circular buffer is better but more tricky. I would try.
>>
>>> Some other review comments below; please ignore anything other reviewers
>>> have already noted.
>>>
>>> Regards,
>>> Peter Hurley
>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>>  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.
>>
>> But I think the dynamic allocation is more reasonable with reducing
>> some global variables.
>
> But introduces a failure mode that doesn't exist with the
> static definition.
>

OK. I'll consider that.

>
>>>>  /* 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?
>>>
>>
>> Yeah, I'll remove the 'buffer_size' parameter.
>>
>>>
>>>> +{
>>>> +     struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>>> +
>>>
>>> Remove this newline.
>>
>> OK.
>>
>>>
>>>> +     if (!req)
>>>> +             return NULL;
>>>> +
>>>> +     /* now allocate buffers for the requests */
>>>
>>> Unnecessary comment.
>>
>> OK.
>>
>>>
>>>> +     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.
>>
>> Make sense.
>>
>>>
>>>> +{
>>>> +     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.
>>
>> That's right. I'll remove these checks.
>>
>>>
>>>
>>>> +
>>>> +     req = port->console_req;
>>>> +     if (!req) {
>>>> +             req = gs_request_new(ep, GS_BUFFER_SIZE);
>>>
>>> Where is port->console_req assigned to?
>>
>> The connect may be called several times, if the req is allocated at
>> one time, there is no need to assign it.
>
> You've missed my point: where is
>
>                 port->console_req = ?????

Sorry, I missed here and will fix it.

>
>
>>>> +             if (!req) {
>>>> +                     pr_err("%s: request fail.\n", __func__);
>>>
>>> Remove redundant error message; the allocator has already done so.
>>
>> OK.
>>
>>>
>>>> +                     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;
>
> I'm not seeing how info->buf_tail is ever reset to 0.
>
>         count = info->buf_tail
>
>         while (count > 0) {
>                 ....
>                 count -= xfer;
>         }
>
> At loop exit count == 0, so
>
>         info->buf_tail -= count;
>
> never decreases buf_tail?
>

Sorry, that's a mistake and I'll fix that.

>
>>>> +     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.
>>
>> Will remove it.
>>
>>>
>>>> +     gscons_info->tty_driver = gs_tty_driver;
>>>> +     INIT_WORK(&gscons_info->work, gs_console_work);
>>>> +     gscons_info->buf_tail = 0;
>>>
>>> Redundant.
>>
>> Will remove it.
>>
>>>
>>>> +     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);
>>
>> Yeah, that's right.
>>
>>>
>>>> +
>>>> +     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?
>>
>> Like I said above, it will meet deadlocks if running the work
>> directly, then introduce the kworker.
>
> You've missed my point here:
>
> CPU 0                                 CPU 1
> --------------------------------      -------------------------------
> gs_console_write()                    gs_console_work()
>
>    info->buf_tail += xfer                info->buf_tail -= count;
>
>
> Neither of these operations are atomic so what will the value of
> info->buf_tail be? For example:
>
>    A <= LOAD(info->buf_tail)
>                                          B <= LOAD(info->buf_tail)
>    A <= A + xfer                         B <= B - count
>    STORE(A, info->buf_tail)
>                                          STORE(B, info->buf_tail)
>
> The result is as if info->buf_tail += xfer never happened.
>

Make sense. I would remove the buffer counter and replace with a
circular buffer. Very thanks for your good suggestions.

>
>>>> +
>>>> +     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;
>>>>
>>>
>>
>>
>>
>



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

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.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/gadget/Kconfig             |    6 +
 drivers/usb/gadget/function/u_serial.c |  239 ++++++++++++++++++++++++++++++++
 2 files changed, 245 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 9cc6a13..343d530 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -27,6 +27,8 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <linux/module.h>
+#include <linux/console.h>
+#include <linux/kthread.h>
 
 #include "u_serial.h"
 
@@ -79,6 +81,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];
+};
 
 /* circular buffer */
 struct gs_buf {
@@ -117,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 {
@@ -1052,6 +1065,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:
@@ -1141,6 +1155,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)
+{
+	struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+
+	if (!req)
+		return NULL;
+
+	/* now allocate buffers for the requests */
+	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)
+{
+	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;
+	}
+
+	req = port->console_req;
+	if (!req) {
+		req = gs_request_new(ep, GS_BUFFER_SIZE);
+		if (!req) {
+			pr_err("%s: request fail.\n", __func__);
+			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;
+	gscons_info->tty_driver = gs_tty_driver;
+	INIT_WORK(&gscons_info->work, gs_console_work);
+	gscons_info->buf_tail = 0;
+	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;
+
+	p = &info->buf[info->buf_tail];
+	memcpy(p, buf, xfer);
+	info->buf_tail += xfer;
+
+	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
@@ -1217,6 +1452,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;
@@ -1318,6 +1554,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");
@@ -1332,6 +1570,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;
-- 
1.7.9.5