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@xxxxxxxxx> wrote:
> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/