Re: [PATCH v5 2/6] usb: gadget: u_serial: reimplement console support

[Ladislav Michl <ladis@linux-mips.org>]

On Tue, Jul 23, 2019 at 10:18:15AM +0800, Baolin Wang wrote:
> Hi Michal,
> 
> On Mon, 22 Jul 2019 at 23:26, Michał Mirosław <mirq-linux@rere.qmqm.pl> 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.

...which is not that relevant there. Current kthead implementation
is buggy, see this series for what needs to be done to fix it:
https://marc.info/?l=linux-usb&m=156305214227371&w=2
and as Michał's fix is superior to fixing kthread I'm voting for his
solution. Only one of my patches is still needed and I'll resend
once this fix hits -next.

> >
> > Only port ttyGS0 gets to be a console for now.
> >
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Tested-by: Ladislav Michl <ladis@linux-mips.org>
> >
> > ---
> >   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