Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1271187 > unrolled thread
| Started by | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| First post | 2015-11-17 14:40 +0100 |
| Last post | 2015-11-18 13:50 +0100 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-17 14:40 +0100
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port Baolin Wang <baolin.wang@linaro.org> - 2015-11-18 03:20 +0100
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-18 10:40 +0100
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port Baolin Wang <baolin.wang@linaro.org> - 2015-11-18 11:50 +0100
RE: [PATCH] usb: gadget: Add the console support for usb-to-serial port David Laight <David.Laight@ACULAB.COM> - 2015-11-18 13:10 +0100
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port Baolin Wang <baolin.wang@linaro.org> - 2015-11-18 13:50 +0100
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2015-11-17 14:40 +0100 |
| Subject | Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port |
| Message-ID | <qvOEi-5DR-11@gated-at.bofh.it> |
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
--
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/
[toc] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2015-11-18 03:20 +0100 |
| Message-ID | <qw0vL-4VG-5@gated-at.bofh.it> |
| In reply to | #1271187 |
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
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2015-11-18 10:40 +0100 |
| Message-ID | <qw7nB-13M-29@gated-at.bofh.it> |
| In reply to | #1271816 |
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
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2015-11-18 11:50 +0100 |
| Message-ID | <qw8tj-1Ir-1@gated-at.bofh.it> |
| In reply to | #1272048 |
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
--
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/
[toc] | [prev] | [next] | [standalone]
| From | David Laight <David.Laight@ACULAB.COM> |
|---|---|
| Date | 2015-11-18 13:10 +0100 |
| Subject | RE: [PATCH] usb: gadget: Add the console support for usb-to-serial port |
| Message-ID | <qw9IJ-2FA-11@gated-at.bofh.it> |
| In reply to | #1272087 |
RnJvbTogQmFvbGluIFdhbmcNCj4gU2VudDogMTggTm92ZW1iZXIgMjAxNSAxMDo0NQ0KPiBPbiAx OCBOb3ZlbWJlciAyMDE1IGF0IDE3OjMyLCBBbmR5IFNoZXZjaGVua28gPGFuZHkuc2hldmNoZW5r b0BnbWFpbC5jb20+IHdyb3RlOg0KPiA+IE9uIFdlZCwgTm92IDE4LCAyMDE1IGF0IDQ6MTUgQU0s IEJhb2xpbiBXYW5nIDxiYW9saW4ud2FuZ0BsaW5hcm8ub3JnPiB3cm90ZToNCj4gPj4gT24gMTcg Tm92ZW1iZXIgMjAxNSBhdCAyMTozNCwgQW5keSBTaGV2Y2hlbmtvIDxhbmR5LnNoZXZjaGVua29A Z21haWwuY29tPiB3cm90ZToNCj4gPj4+IE9uIE1vbiwgTm92IDE2LCAyMDE1IGF0IDk6MDUgQU0s IEJhb2xpbiBXYW5nIDxiYW9saW4ud2FuZ0BsaW5hcm8ub3JnPiB3cm90ZToNCj4gPj4+PiBJdCBk b3NlIG5vdCB3b3JrIHdoZW4gd2Ugd2FudCB0byB1c2UgdGhlIHVzYi10by1zZXJpYWwgcG9ydCBi YXNlZA0KPiA+Pj4+IG9uIG9uZSB1c2IgZ2FkZ2V0IGFzIGEgY29uc29sZS4gVGh1cyB0aGlzIHBh dGNoIGFkZHMgdGhlIGNvbnNvbGUNCj4gPj4+PiBpbml0aWFsaXphdGlvbiB0byBzdXBwb3J0IHRo aXMgcmVxdWVzdC4NCj4gPj4+DQo+ID4NCj4gPj4+PiArI2RlZmluZSBHU19CVUZGRVJfU0laRSAg ICAgICAgICg0MDk2KQ0KPiA+Pj4gUmVkdW5kYW50IHBhcmVucw0KPiA+PiBPSy4gSSdsbCByZW1v dmUgaXQuDQo+ID4+DQo+ID4+Pj4gKyNkZWZpbmUgR1NfQ09OU09MRV9CVUZfU0laRSAgICAoMiAq IEdTX0JVRkZFUl9TSVpFKQ0KPiA+Pj4+ICsNCj4gPj4+PiArc3RydWN0IGdzY29uc19pbmZvIHsN Cj4gPj4+PiArICAgICAgIHN0cnVjdCBnc19wb3J0ICAgICAgICAgICpwb3J0Ow0KPiA+Pj4+ICsg ICAgICAgc3RydWN0IHR0eV9kcml2ZXIgICAgICAgKnR0eV9kcml2ZXI7DQo+ID4+Pj4gKyAgICAg ICBzdHJ1Y3Qgd29ya19zdHJ1Y3QgICAgICB3b3JrOw0KPiA+Pj4+ICsgICAgICAgaW50ICAgICAg ICAgICAgICAgICAgICAgYnVmX3RhaWw7DQo+ID4+Pj4gKyAgICAgICBjaGFyICAgICAgICAgICAg ICAgICAgICBidWZbR1NfQ09OU09MRV9CVUZfU0laRV07DQo+ID4+Pg0KPiA+Pj4gQ2FuJ3QgYmUg bWFsbG9jZWQgb25jZT8NCj4gPj4gVGhlICdnc2NvbnNfaW5mbycgc3RydWN0dXJlIGlzIG1hbGxv Y2VkIG9uY2UuDQo+ID4NCj4gPiBJbiBzdGF0ZSBvZiBoaWdoIGZyYWdtZW50YXRpb24gaXMgcXVp dGUgaGFyZCB0byBmaW5kIGJpZyBtZW1vcnkgY2h1bmtzLg0KPiA+IEkgd291bGQgc3BsaXQgaXQg dG8gdHdvIGFsbG9jYXRpb25zLCB0aG91Z2ggaWYgbWFpbnRhaW5lcnMgYXJlIG9rYXkNCj4gPiB3 aXRoIHlvdXIgY29kZSwgdGhlbiBJJ20gYWxzbyBva2F5Lg0KPiA+DQo+IA0KPiBNYWtlIHNlbnNl LiBCdXQgSSB0aGluayB0aGUgbWFqb3IgbWVtb3J5IG9mIHRoZSAnc3RydWN0IGdzY29uc19pbmZv Jw0KPiBpcyBmb3IgdGhlICdidWYnIG1lbWJlciwgc28gSSBzdGlsbCB0aGluayBubyBuZWVkIHRv IGFsbG9jYXRlIGl0IDINCj4gdGltZXMuDQoNCkl0IG1heSBiZSB3b3J0aCBqdXN0IHJlZHVjaW5n IEdTX0JVRkZFUl9TSVpFIHNsaWdodGx5IHNvIHRoYXQgdGhlIGdzY29uc19pbmZvDQpzdHJ1Y3R1 cmUgaXRzZWxmIGlzIGxlc3MgdGhhbiA4ay4NCklmIHlvdSBjYW4ndCBnZXQgMiBhZGphY2VudCBw YWdlcyB0aGVuIGEgbG90IG9mIHRoaW5ncyB3aWxsIGZhaWwuDQoNCglEYXZpZA0KDQo= -- 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/
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2015-11-18 13:50 +0100 |
| Message-ID | <qwalr-2Vg-1@gated-at.bofh.it> |
| In reply to | #1272111 |
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
--
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/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web