| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMz4ku+JYFLt6H-Lvm_KbEX3iHNqzdBynUp2mAZhdaB9dsv0CA@mail.gmail.com>
Date: Wed, 18 Nov 2015 10:15:04 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Felipe Balbi <balbi@...com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
r.baldyga@...sung.com, fabio.estevam@...escale.com,
Philip Oberstaller <Philip.Oberstaller@...tentrio.com>,
Peter Hurley <peter@...leysoftware.com>,
scottwood@...escale.com, Mark Brown <broonie@...nel.org>,
USB <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@...aro.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@...r.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists