Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754054AbbKRCPJ (ORCPT ); Tue, 17 Nov 2015 21:15:09 -0500 Received: from mail-yk0-f179.google.com ([209.85.160.179]:35385 "EHLO mail-yk0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752009AbbKRCPG convert rfc822-to-8bit (ORCPT ); Tue, 17 Nov 2015 21:15:06 -0500 MIME-Version: 1.0 In-Reply-To: References: Date: Wed, 18 Nov 2015 10:15:04 +0800 Message-ID: Subject: Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port From: Baolin Wang To: Andy Shevchenko Cc: Felipe Balbi , Greg Kroah-Hartman , r.baldyga@samsung.com, fabio.estevam@freescale.com, Philip Oberstaller , Peter Hurley , scottwood@freescale.com, Mark Brown , USB , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9757 Lines: 357 On 17 November 2015 at 21:34, Andy Shevchenko wrote: > On Mon, Nov 16, 2015 at 9: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. > > >> @@ -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 -- 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/