Hi Michal,
On Mon, 22 Jul 2019 at 23:26, Michał Mirosław <mirq-linux@xxxxxxxxxxxx> wrote:
>
> Rewrite console support to fix a few shortcomings of the old code
> preventing its use with multiple ports. This removes some duplicated
> code and replaces a custom kthread with simpler workqueue item.
Could you elaborate on why changing kthread to a workqueue? The
purpose of using kthread here is considering that the kthead has a
better scheduler response than pooled kworker.
>
> Only port ttyGS0 gets to be a console for now.
>
> Signed-off-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Tested-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
>
> ---
> v5: no changes
> v4: cosmetic change to __gs_console_push()
> v3: no changes
> v2: no changes
>
> ---
> drivers/usb/gadget/function/u_serial.c | 351 ++++++++++++-------------
> 1 file changed, 164 insertions(+), 187 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index bb1e2e1d0076..94f6999e8262 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -82,14 +82,12 @@
> #define GS_CONSOLE_BUF_SIZE 8192
>
> /* console info */
> -struct gscons_info {
> - struct gs_port *port;
> - struct task_struct *console_thread;
> - struct kfifo con_buf;
> - /* protect the buf and busy flag */
> - spinlock_t con_lock;
> - int req_busy;
> - struct usb_request *console_req;
> +struct gs_console {
> + struct console console;
> + struct work_struct work;
> + spinlock_t lock;
> + struct usb_request *req;
> + struct kfifo buf;
> };
>
> /*
> @@ -101,6 +99,9 @@ struct gs_port {
> spinlock_t port_lock; /* guard port_* access */
>
> struct gserial *port_usb;
> +#ifdef CONFIG_U_SERIAL_CONSOLE
> + struct gs_console *console;
> +#endif
>
> bool openclose; /* open/close in progress */
> u8 port_num;
> @@ -889,36 +890,9 @@ static struct tty_driver *gs_tty_driver;
>
> #ifdef CONFIG_U_SERIAL_CONSOLE
>
> -static struct gscons_info gscons_info;
> -static struct console gserial_cons;
> -
> -static struct usb_request *gs_request_new(struct usb_ep *ep)
> +static void gs_console_complete_out(struct usb_ep *ep, struct usb_request *req)
> {
> - struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> - if (!req)
> - return NULL;
> -
> - req->buf = kmalloc(ep->maxpacket, 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)
> - return;
> -
> - kfree(req->buf);
> - usb_ep_free_request(ep, req);
> -}
> -
> -static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
> -{
> - struct gscons_info *info = &gscons_info;
> + struct gs_console *cons = req->context;
>
> switch (req->status) {
> default:
> @@ -927,12 +901,12 @@ static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
> /* fall through */
> case 0:
> /* normal completion */
> - spin_lock(&info->con_lock);
> - info->req_busy = 0;
> - spin_unlock(&info->con_lock);
> -
> - wake_up_process(info->console_thread);
> + spin_lock(&cons->lock);
> + req->length = 0;
> + schedule_work(&cons->work);
> + spin_unlock(&cons->lock);
> break;
> + case -ECONNRESET:
> case -ESHUTDOWN:
> /* disconnect */
> pr_vdebug("%s: %s shutdown\n", __func__, ep->name);
> @@ -940,190 +914,190 @@ static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
> }
> }
>
> -static int gs_console_connect(int port_num)
> +static void __gs_console_push(struct gs_console *cons)
> {
> - struct gscons_info *info = &gscons_info;
> - struct gs_port *port;
> + struct usb_request *req = cons->req;
> struct usb_ep *ep;
> + size_t size;
>
> - if (port_num != gserial_cons.index) {
> - pr_err("%s: port num [%d] is not support console\n",
> - __func__, port_num);
> - return -ENXIO;
> - }
> + if (!req)
> + return; /* disconnected */
>
> - port = ports[port_num].port;
> - ep = port->port_usb->in;
> - if (!info->console_req) {
> - info->console_req = gs_request_new(ep);
> - if (!info->console_req)
> - return -ENOMEM;
> - info->console_req->complete = gs_complete_out;
> - }
> + if (req->length)
> + return; /* busy */
>
> - info->port = port;
> - spin_lock(&info->con_lock);
> - info->req_busy = 0;
> - spin_unlock(&info->con_lock);
> - pr_vdebug("port[%d] console connect!\n", port_num);
> - return 0;
> -}
> -
> -static void gs_console_disconnect(struct usb_ep *ep)
> -{
> - struct gscons_info *info = &gscons_info;
> - struct usb_request *req = info->console_req;
> -
> - gs_request_free(req, ep);
> - info->console_req = NULL;
> -}
> -
> -static int gs_console_thread(void *data)
> -{
> - struct gscons_info *info = &gscons_info;
> - struct gs_port *port;
> - struct usb_request *req;
> - struct usb_ep *ep;
> - int xfer, ret, count, size;
> + ep = cons->console.data;
> + size = kfifo_out(&cons->buf, req->buf, ep->maxpacket);
> + if (!size)
> + return;
>
> - do {
> - port = info->port;
> - set_current_state(TASK_INTERRUPTIBLE);
> - if (!port || !port->port_usb
> - || !port->port_usb->in || !info->console_req)
> - goto sched;
> -
> - req = info->console_req;
> - ep = port->port_usb->in;
> -
> - spin_lock_irq(&info->con_lock);
> - count = kfifo_len(&info->con_buf);
> - size = ep->maxpacket;
> -
> - if (count > 0 && !info->req_busy) {
> - set_current_state(TASK_RUNNING);
> - if (count < size)
> - size = count;
> -
> - xfer = kfifo_out(&info->con_buf, req->buf, size);
> - req->length = xfer;
> -
> - spin_unlock(&info->con_lock);
> - ret = usb_ep_queue(ep, req, GFP_ATOMIC);
> - spin_lock(&info->con_lock);
> - if (ret < 0)
> - info->req_busy = 0;
> - else
> - info->req_busy = 1;
> -
> - spin_unlock_irq(&info->con_lock);
> - } else {
> - spin_unlock_irq(&info->con_lock);
> -sched:
> - if (kthread_should_stop()) {
> - set_current_state(TASK_RUNNING);
> - break;
> - }
> - schedule();
> - }
> - } while (1);
> -
> - return 0;
> + req->length = size;
> + if (usb_ep_queue(ep, req, GFP_ATOMIC))
> + req->length = 0;
> }
>
> -static int gs_console_setup(struct console *co, char *options)
> +static void gs_console_work(struct work_struct *work)
> {
> - struct gscons_info *info = &gscons_info;
> - int status;
> -
> - info->port = NULL;
> - info->console_req = NULL;
> - info->req_busy = 0;
> - spin_lock_init(&info->con_lock);
> + struct gs_console *cons = container_of(work, struct gs_console, work);
>
> - status = kfifo_alloc(&info->con_buf, GS_CONSOLE_BUF_SIZE, GFP_KERNEL);
> - if (status) {
> - pr_err("%s: allocate console buffer failed\n", __func__);
> - return status;
> - }
> + spin_lock_irq(&cons->lock);
>
> - info->console_thread = kthread_create(gs_console_thread,
> - co, "gs_console");
> - if (IS_ERR(info->console_thread)) {
> - pr_err("%s: cannot create console thread\n", __func__);
> - kfifo_free(&info->con_buf);
> - return PTR_ERR(info->console_thread);
> - }
> - wake_up_process(info->console_thread);
> + __gs_console_push(cons);
>
> - return 0;
> + spin_unlock_irq(&cons->lock);
> }
>
> static void gs_console_write(struct console *co,
> const char *buf, unsigned count)
> {
> - struct gscons_info *info = &gscons_info;
> + struct gs_console *cons = container_of(co, struct gs_console, console);
> unsigned long flags;
>
> - spin_lock_irqsave(&info->con_lock, flags);
> - kfifo_in(&info->con_buf, buf, count);
> - spin_unlock_irqrestore(&info->con_lock, flags);
> + spin_lock_irqsave(&cons->lock, flags);
>
> - wake_up_process(info->console_thread);
> + kfifo_in(&cons->buf, buf, count);
> +
> + if (cons->req && !cons->req->length)
> + schedule_work(&cons->work);
> +
> + spin_unlock_irqrestore(&cons->lock, flags);
> }
>
> static struct tty_driver *gs_console_device(struct console *co, int *index)
> {
> - struct tty_driver **p = (struct tty_driver **)co->data;
> -
> - if (!*p)
> - return NULL;
> -
> *index = co->index;
> - return *p;
> + return gs_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,
> - .data = &gs_tty_driver,
> -};
> -
> -static void gserial_console_init(void)
> +static int gs_console_connect(struct gs_port *port)
> {
> - register_console(&gserial_cons);
> + struct gs_console *cons = port->console;
> + struct usb_request *req;
> + struct usb_ep *ep;
> +
> + if (!cons)
> + return 0;
> +
> + ep = port->port_usb->in;
> + req = gs_alloc_req(ep, ep->maxpacket, GFP_ATOMIC);
> + if (!req)
> + return -ENOMEM;
> + req->complete = gs_console_complete_out;
> + req->context = cons;
> + req->length = 0;
> +
> + spin_lock(&cons->lock);
> + cons->req = req;
> + cons->console.data = ep;
> + spin_unlock(&cons->lock);
> +
> + pr_debug("ttyGS%d: console connected!\n", port->port_num);
> +
> + schedule_work(&cons->work);
> +
> + return 0;
> +}
> +
> +static void gs_console_disconnect(struct gs_port *port)
> +{
> + struct gs_console *cons = port->console;
> + struct usb_request *req;
> + struct usb_ep *ep;
> +
> + if (!cons)
> + return;
> +
> + spin_lock(&cons->lock);
> +
> + req = cons->req;
> + ep = cons->console.data;
> + cons->req = NULL;
> +
> + spin_unlock(&cons->lock);
> +
> + if (!req)
> + return;
> +
> + usb_ep_dequeue(ep, req);
> + gs_free_req(ep, req);
> }
>
> -static void gserial_console_exit(void)
> +static int gs_console_init(struct gs_port *port)
> {
> - struct gscons_info *info = &gscons_info;
> + struct gs_console *cons;
> + int err;
> +
> + if (port->console)
> + return 0;
> +
> + cons = kzalloc(sizeof(*port->console), GFP_KERNEL);
> + if (!cons)
> + return -ENOMEM;
> +
> + strcpy(cons->console.name, "ttyGS");
> + cons->console.write = gs_console_write;
> + cons->console.device = gs_console_device;
> + cons->console.flags = CON_PRINTBUFFER;
> + cons->console.index = port->port_num;
> +
> + INIT_WORK(&cons->work, gs_console_work);
> + spin_lock_init(&cons->lock);
> +
> + err = kfifo_alloc(&cons->buf, GS_CONSOLE_BUF_SIZE, GFP_KERNEL);
> + if (err) {
> + pr_err("ttyGS%d: allocate console buffer failed\n", port->port_num);
> + kfree(cons);
> + return err;
> + }
> +
> + port->console = cons;
> + register_console(&cons->console);
> +
> + spin_lock_irq(&port->port_lock);
> + if (port->port_usb)
> + gs_console_connect(port);
> + spin_unlock_irq(&port->port_lock);
> +
> + return 0;
> +}
> +
> +static void gs_console_exit(struct gs_port *port)
> +{
> + struct gs_console *cons = port->console;
> +
> + if (!cons)
> + return;
> +
> + unregister_console(&cons->console);
> +
> + spin_lock_irq(&port->port_lock);
> + if (cons->req)
> + gs_console_disconnect(port);
> + spin_unlock_irq(&port->port_lock);
>
> - unregister_console(&gserial_cons);
> - if (!IS_ERR_OR_NULL(info->console_thread))
> - kthread_stop(info->console_thread);
> - kfifo_free(&info->con_buf);
> + cancel_work_sync(&cons->work);
> + kfifo_free(&cons->buf);
> + kfree(cons);
> + port->console = NULL;
> }
>
> #else
>
> -static int gs_console_connect(int port_num)
> +static int gs_console_connect(struct gs_port *port)
> {
> return 0;
> }
>
> -static void gs_console_disconnect(struct usb_ep *ep)
> +static void gs_console_disconnect(struct gs_port *port)
> {
> }
>
> -static void gserial_console_init(void)
> +static int gs_console_init(struct gs_port *port)
> {
> + return -ENOSYS;
> }
>
> -static void gserial_console_exit(void)
> +static void gs_console_exit(struct gs_port *port)
> {
> }
>
> @@ -1197,18 +1171,19 @@ void gserial_free_line(unsigned char port_num)
> return;
> }
> port = ports[port_num].port;
> + gs_console_exit(port);
> ports[port_num].port = NULL;
> mutex_unlock(&ports[port_num].lock);
>
> gserial_free_port(port);
> tty_unregister_device(gs_tty_driver, port_num);
> - gserial_console_exit();
> }
> EXPORT_SYMBOL_GPL(gserial_free_line);
>
> int gserial_alloc_line(unsigned char *line_num)
> {
> struct usb_cdc_line_coding coding;
> + struct gs_port *port;
> struct device *tty_dev;
> int ret;
> int port_num;
> @@ -1231,23 +1206,24 @@ int gserial_alloc_line(unsigned char *line_num)
>
> /* ... and sysfs class devices, so mdev/udev make /dev/ttyGS* */
>
> - tty_dev = tty_port_register_device(&ports[port_num].port->port,
> + port = ports[port_num].port;
> + tty_dev = tty_port_register_device(&port->port,
> gs_tty_driver, port_num, NULL);
> if (IS_ERR(tty_dev)) {
> - struct gs_port *port;
> pr_err("%s: failed to register tty for port %d, err %ld\n",
> __func__, port_num, PTR_ERR(tty_dev));
>
> ret = PTR_ERR(tty_dev);
> mutex_lock(&ports[port_num].lock);
> - port = ports[port_num].port;
> ports[port_num].port = NULL;
> mutex_unlock(&ports[port_num].lock);
> gserial_free_port(port);
> goto err;
> }
> *line_num = port_num;
> - gserial_console_init();
> +
> + if (!port_num)
> + gs_console_init(port);
> err:
> return ret;
> }
> @@ -1329,7 +1305,7 @@ int gserial_connect(struct gserial *gser, u8 port_num)
> gser->disconnect(gser);
> }
>
> - status = gs_console_connect(port_num);
> + status = gs_console_connect(port);
> spin_unlock_irqrestore(&port->port_lock, flags);
>
> return status;
> @@ -1361,6 +1337,8 @@ void gserial_disconnect(struct gserial *gser)
> /* tell the TTY glue not to do I/O here any more */
> spin_lock_irqsave(&port->port_lock, flags);
>
> + gs_console_disconnect(port);
> +
> /* REVISIT as above: how best to track this? */
> port->port_line_coding = gser->port_line_coding;
>
> @@ -1388,7 +1366,6 @@ void gserial_disconnect(struct gserial *gser)
> port->read_allocated = port->read_started =
> port->write_allocated = port->write_started = 0;
>
> - gs_console_disconnect(gser->in);
> spin_unlock_irqrestore(&port->port_lock, flags);
> }
> EXPORT_SYMBOL_GPL(gserial_disconnect);
> --
> 2.20.1
>
--
Baolin Wang
Best Regards
