Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles
On Tue, Feb 26, 2019 at 05:07:10PM +, Mans Rullgard wrote:
> The SIMCom SIM5218 and compatible devices have 5 USB interfaces, only 4
> of which are serial ports. The fifth is a network interface supported
> by the qmi-wwan driver. Furthermore, the serial ports do not support
> modem control signals. Add driver_info flags to reflect this.
>
> Signed-off-by: Mans Rullgard
> ---
> drivers/usb/serial/option.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index fb544340888b..af4cbfecc3ff 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -1066,7 +1066,8 @@ static const struct usb_device_id option_ids[] = {
> .driver_info = RSVD(3) },
> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x6613)}, /* Onda H600/ZTE MF330 */
> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x0023)}, /* ONYX 3G device */
> - { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000)}, /* SIMCom SIM5218 */
> + { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000), /* SIMCom SIM5218 */
> + .driver_info = NCTRL(0) | NCTRL(1) | NCTRL(2) | NCTRL(3) | RSVD(4) },
> /* Quectel products using Qualcomm vendor ID */
> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC15)},
> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC20),
Could you please provide the output of usb-devices (or lsusb -v) for
this device?
Thanks,
Johan
Re: [PATCH 1/5] usb: gadget: u_serial: add missing port entry locking
On Wed, Feb 27, 2019 at 08:48:56AM +0100, Michał Mirosław wrote: > gserial_alloc_line() misses locking (for a release barrier) while > resetting port entry on TTY allocation failure. Fix this. > > Signed-off-by: Michał Mirosław > --- > drivers/usb/gadget/function/u_serial.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/gadget/function/u_serial.c > b/drivers/usb/gadget/function/u_serial.c > index 65f634ec7fc2..bb1e2e1d0076 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -1239,8 +1239,10 @@ int gserial_alloc_line(unsigned char *line_num) > __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; > } Should this be backported to stable kernels to resolve this issue? thanks, greg k-h
Re: [PATCH 3/5] usb: gadget: u_serial: make OBEX port not a console
On Wed, Feb 27, 2019 at 08:48:57AM +0100, Michał Mirosław wrote:
> Prevent OBEX serial port from ever becoming a console.
Why?
> /* management of individual TTY ports */
> -int gserial_alloc_line(unsigned char *port_line);
> +int gserial_alloc_line(unsigned char *port_line, bool maybe_console);
Boolean flags in function calls are a major pain over time.
There is no way to know, when you see this function being called, what
"true" or "false" means, so you need to go look up the function header
here (as I had to do in this patch), to get a clue as to what is going
on.
It is MUCH better to just have a new function:
gserial_alloc_line_no_console()
and leave gserial_alloc_line() alone, or, just have two functions:
gserial_alloc_line_no_console()
gserial_alloc_line_console()
which resolve internally to the same function:
static int __gserial_alloc_line(unsigned char *port_line, bool maybe_console)
{
...
}
int gserial_alloc_line_no_console(unsigned char *port_line)
{
return __gserial_alloc_line(port_line, false);
}
int gserial_alloc_line_console(unsigned char *port_line)
{
return __gserial_alloc_line(port_line, true);
}
Trust me, when you have to go fix this code up in 5 years, you will
thank me for having to force you to write the code this way :)
thanks,
greg k-h
Re: [PATCH 5/5] usb: gadget: u_serial: diagnose missed console messages
On Wed, Feb 27, 2019 at 08:48:58AM +0100, Michał Mirosław wrote: > Insert markers in console stream instead of making console output > glued together when drops happen. I'm sorry, but I can not understand this sentance at all. Can you please write more here to help out? thanks, greg k-h
Re: [PATCH 2/5] usb: gadget: u_serial: reimplement console support
On Wed, Feb 27, 2019 at 08:48:57AM +0100, Michał Mirosław 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. > > Only port ttyGS0 gets to be a console for now. > > Signed-off-by: Michał Mirosław > --- > drivers/usb/gadget/function/u_serial.c | 352 - > 1 file changed, 164 insertions(+), 188 deletions(-) Very nice, getting rid of the kthread alone makes this patch worth while :)
Re: [PATCH] usb: typec: altmodes/displayport: Fall back to multi-func pins
On Mon, Feb 25, 2019 at 07:50:32PM +0100, Hans de Goede wrote: > Hi Heikki, > > On 25-02-19 16:49, Heikki Krogerus wrote: > > On Mon, Feb 25, 2019 at 01:56:37PM +0100, Hans de Goede wrote: > > > If our port-partner supports both DP-only operation (pin-assignment C) > > > and multi-func operation (pin-assignment D) and we only support > > > pin-assignment D and the port-partner prefers DP-only mode, then > > > before this commit we would and up masking out pin-assignment D from > > > the available pin-assignments and fail to pick a pin-assignment. > > > > > > Instead only mask out the multi-func pin-assignments if we support > > > dp-only pin-assignments, so that we correctly fall-back to a multi-func > > > pin-assignment in this case (by picking pin-assignment D). > > > > > > Signed-off-by: Hans de Goede > > > > Should this be handled as a fix? > > AFAIK they are no users if this yet, until we've agreement > on the DT bindings and code merged for adding alt-modes > to an usb-connector node, nothing will be using this code, > so I see little use in adding a Cc: stable or some such. True. thanks, -- heikki
Re: [PATCH] usb: typec: pi3usb30532: Keep orientation when setting mux to safe mode
On Mon, Feb 25, 2019 at 07:52:10PM +0100, Hans de Goede wrote: > Hi, > > On 25-02-19 16:50, Heikki Krogerus wrote: > > On Fri, Feb 22, 2019 at 08:22:39PM +0100, Hans de Goede wrote: > > > Keep the orientation value when setting the mux to safe mode, this > > > fixes the orientation getting reset when switching alt-modes. > > > > > > Signed-off-by: Hans de Goede > > > > Should this be also a fix? > > This only comes into play when switching alt-modes, so more > or less the same as with the displayport altmode fix: > > "There are no users if this yet, until we've agreement > on the DT bindings and code merged for adding alt-modes > to an usb-connector node, nothing will be using this code, > so I see little use in adding a Cc: stable or some such." Agree, thanks, -- heikki
Re: [PATCH 1/5] usb: gadget: u_serial: add missing port entry locking
On Wed, Feb 27, 2019 at 09:41:07AM +0100, Greg Kroah-Hartman wrote: > On Wed, Feb 27, 2019 at 08:48:56AM +0100, Michał Mirosław wrote: > > gserial_alloc_line() misses locking (for a release barrier) while > > resetting port entry on TTY allocation failure. Fix this. > > > > Signed-off-by: Michał Mirosław > > --- > > drivers/usb/gadget/function/u_serial.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/u_serial.c > > b/drivers/usb/gadget/function/u_serial.c > > index 65f634ec7fc2..bb1e2e1d0076 100644 > > --- a/drivers/usb/gadget/function/u_serial.c > > +++ b/drivers/usb/gadget/function/u_serial.c > > @@ -1239,8 +1239,10 @@ int gserial_alloc_line(unsigned char *line_num) > > __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; > > } > > Should this be backported to stable kernels to resolve this issue? This code has been there for ages and the failure case is rather obscure, but I guess it won't hurt to fix it. Best Regards, Michał Mirosław
[PATCH v2 2/5] usb: gadget: u_serial: reimplement console support
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.
Only port ttyGS0 gets to be a console for now.
Signed-off-by: Michał Mirosław
---
v2: no changes
---
drivers/usb/gadget/function/u_serial.c | 352 -
1 file changed, 164 insertions(+), 188 deletions(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index bb1e2e1d0076..8d2d861e1543 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_SIZE8192
/* console info */
-struct gscons_info {
- struct gs_port *port;
- struct task_struct *console_thread;
- struct kfifocon_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 kfifobuf;
};
/*
@@ -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
boolopenclose; /* 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,189 @@ 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_ep *ep;
+ struct usb_request *req = cons->req;
+ struct usb_ep *ep = cons->console.data;
+ size_t size = 0;
- 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;
+ size = kfifo_out(&cons->buf, req->buf, ep->maxpacket);
+ if (!size)
+ return;
- gs_request_free(req, ep);
- info->co
[PATCH v2 5/5] usb: gadget: u_serial: diagnose missed console messages
Insert markers in console stream marking places where data
is missing. This makes the hole in the data stand out clearly
instead of glueing together unrelated messages.
Signed-off-by: Michał Mirosław
---
v2: commit message massage
---
drivers/usb/gadget/function/u_serial.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 604e187fadb7..66518aba58c2 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -88,6 +88,7 @@ struct gs_console {
spinlock_t lock;
struct usb_request *req;
struct kfifobuf;
+ size_t missed;
};
/*
@@ -930,6 +931,15 @@ static void __gs_console_push(struct gs_console *cons)
if (!size)
return;
+ if (cons->missed && ep->maxpacket >= 64) {
+ char buf[64];
+ size_t len;
+
+ len = sprintf(buf, "\n[MISSED %zu bytes]\n", cons->missed);
+ kfifo_in(&cons->buf, buf, len);
+ cons->missed = 0;
+ }
+
req->length = size;
if (usb_ep_queue(ep, req, GFP_ATOMIC))
req->length = 0;
@@ -951,10 +961,13 @@ static void gs_console_write(struct console *co,
{
struct gs_console *cons = container_of(co, struct gs_console, console);
unsigned long flags;
+ size_t n;
spin_lock_irqsave(&cons->lock, flags);
- kfifo_in(&cons->buf, buf, count);
+ n = kfifo_in(&cons->buf, buf, count);
+ if (n < count)
+ cons->missed += count - n;
if (cons->req && !cons->req->length)
schedule_work(&cons->work);
--
2.20.1
[PATCH v2 4/5] usb: gadget: u_serial: allow more console gadget ports
Allow configuring more than one console using USB serial or ACM gadget.
By default, only first (ttyGS0) is a console, but this may be changed
using function's new "console" attribute.
Signed-off-by: Michał Mirosław
---
v2: no changes
---
drivers/usb/gadget/function/f_acm.c| 21
drivers/usb/gadget/function/f_serial.c | 21
drivers/usb/gadget/function/u_serial.c | 46 ++
drivers/usb/gadget/function/u_serial.h | 7
4 files changed, 95 insertions(+)
diff --git a/drivers/usb/gadget/function/f_acm.c
b/drivers/usb/gadget/function/f_acm.c
index 9fc98de83624..7c152c28b26c 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -771,6 +771,24 @@ static struct configfs_item_operations acm_item_ops = {
.release= acm_attr_release,
};
+#ifdef CONFIG_U_SERIAL_CONSOLE
+
+static ssize_t f_acm_console_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ return gserial_set_console(to_f_serial_opts(item)->port_num,
+ page, count);
+}
+
+static ssize_t f_acm_console_show(struct config_item *item, char *page)
+{
+ return gserial_get_console(to_f_serial_opts(item)->port_num, page);
+}
+
+CONFIGFS_ATTR(f_acm_, console);
+
+#endif /* CONFIG_U_SERIAL_CONSOLE */
+
static ssize_t f_acm_port_num_show(struct config_item *item, char *page)
{
return sprintf(page, "%u\n", to_f_serial_opts(item)->port_num);
@@ -779,6 +797,9 @@ static ssize_t f_acm_port_num_show(struct config_item
*item, char *page)
CONFIGFS_ATTR_RO(f_acm_, port_num);
static struct configfs_attribute *acm_attrs[] = {
+#ifdef CONFIG_U_SERIAL_CONSOLE
+ &f_acm_attr_console,
+#endif
&f_acm_attr_port_num,
NULL,
};
diff --git a/drivers/usb/gadget/function/f_serial.c
b/drivers/usb/gadget/function/f_serial.c
index c860f30a0ea2..1406255d0865 100644
--- a/drivers/usb/gadget/function/f_serial.c
+++ b/drivers/usb/gadget/function/f_serial.c
@@ -266,6 +266,24 @@ static struct configfs_item_operations serial_item_ops = {
.release= serial_attr_release,
};
+#ifdef CONFIG_U_SERIAL_CONSOLE
+
+static ssize_t f_serial_console_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ return gserial_set_console(to_f_serial_opts(item)->port_num,
+ page, count);
+}
+
+static ssize_t f_serial_console_show(struct config_item *item, char *page)
+{
+ return gserial_get_console(to_f_serial_opts(item)->port_num, page);
+}
+
+CONFIGFS_ATTR(f_serial_, console);
+
+#endif /* CONFIG_U_SERIAL_CONSOLE */
+
static ssize_t f_serial_port_num_show(struct config_item *item, char *page)
{
return sprintf(page, "%u\n", to_f_serial_opts(item)->port_num);
@@ -274,6 +292,9 @@ static ssize_t f_serial_port_num_show(struct config_item
*item, char *page)
CONFIGFS_ATTR_RO(f_serial_, port_num);
static struct configfs_attribute *acm_attrs[] = {
+#ifdef CONFIG_U_SERIAL_CONSOLE
+ &f_serial_attr_console,
+#endif
&f_serial_attr_port_num,
NULL,
};
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 3466d94f1441..604e187fadb7 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1080,6 +1080,52 @@ static void gs_console_exit(struct gs_port *port)
port->console = NULL;
}
+ssize_t gserial_set_console(unsigned char port_num, const char *page, size_t
count)
+{
+ struct gs_port *port;
+ bool enable;
+ int ret = -ENXIO;
+
+ mutex_lock(&ports[port_num].lock);
+ port = ports[port_num].port;
+
+ if (WARN_ON(port == NULL))
+ goto out;
+
+ ret = strtobool(page, &enable);
+ if (ret)
+ goto out;
+
+ if (enable)
+ ret = gs_console_init(port);
+ else
+ gs_console_exit(port);
+
+ mutex_unlock(&ports[port_num].lock);
+out:
+ return ret < 0 ? ret : count;
+}
+EXPORT_SYMBOL_GPL(gserial_set_console);
+
+ssize_t gserial_get_console(unsigned char port_num, char *page)
+{
+ struct gs_port *port;
+ ssize_t ret = -ENXIO;
+
+ mutex_lock(&ports[port_num].lock);
+ port = ports[port_num].port;
+
+ if (WARN_ON(port == NULL))
+ goto out;
+
+ ret = sprintf(page, "%u\n", !!port->console);
+
+ mutex_unlock(&ports[port_num].lock);
+out:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gserial_get_console);
+
#else
static int gs_console_connect(struct gs_port *port)
diff --git a/drivers/usb/gadget/function/u_serial.h
b/drivers/usb/gadget/function/u_serial.h
index a86bd3ce4781..15b6061e385b 100644
--- a/drivers/usb/gadget/function/u_serial.h
+++ b/drivers/usb/gadget/function/u_serial.h
@@ -58,6 +58,13 @@ int gserial_alloc_line_raw(unsigned char *port_line);
int gserial_alloc_line(unsigned char *port_line);
void gs
[PATCH v2 1/5] usb: gadget: u_serial: add missing port entry locking
gserial_alloc_line() misses locking (for a release barrier) while resetting port entry on TTY allocation failure. Fix this. Signed-off-by: Michał Mirosław --- v2: no changes --- drivers/usb/gadget/function/u_serial.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 65f634ec7fc2..bb1e2e1d0076 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -1239,8 +1239,10 @@ int gserial_alloc_line(unsigned char *line_num) __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; } -- 2.20.1
[PATCH v2 3/5] usb: gadget: u_serial: make OBEX port not a console
Prevent OBEX serial port from ever becoming a console. Console messages
will definitely break the protocol, and since you have to instantiate
the port making it explicitly for OBEX, there is no point in allowing
console to break it by mistake.
Signed-off-by: Michał Mirosław
---
v2: change of API
+ commit message massage
---
drivers/usb/gadget/function/f_obex.c | 2 +-
drivers/usb/gadget/function/u_serial.c | 16
drivers/usb/gadget/function/u_serial.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/function/f_obex.c
b/drivers/usb/gadget/function/f_obex.c
index 55b7f57d2dc7..3f11f41619f9 100644
--- a/drivers/usb/gadget/function/f_obex.c
+++ b/drivers/usb/gadget/function/f_obex.c
@@ -432,7 +432,7 @@ static struct usb_function_instance *obex_alloc_inst(void)
return ERR_PTR(-ENOMEM);
opts->func_inst.free_func_inst = obex_free_inst;
- ret = gserial_alloc_line(&opts->port_num);
+ ret = gserial_alloc_line_raw(&opts->port_num);
if (ret) {
kfree(opts);
return ERR_PTR(ret);
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 8d2d861e1543..3466d94f1441 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1179,7 +1179,7 @@ void gserial_free_line(unsigned char port_num)
}
EXPORT_SYMBOL_GPL(gserial_free_line);
-int gserial_alloc_line(unsigned char *line_num)
+int gserial_alloc_line_raw(unsigned char *line_num)
{
struct usb_cdc_line_coding coding;
struct gs_port *port;
@@ -1220,12 +1220,20 @@ int gserial_alloc_line(unsigned char *line_num)
goto err;
}
*line_num = port_num;
-
- if (!port_num)
- gs_console_init(port);
err:
return ret;
}
+EXPORT_SYMBOL_GPL(gserial_alloc_line_raw);
+
+int gserial_alloc_line(unsigned char *line_num)
+{
+ int ret = gserial_alloc_line_raw(line_num);
+
+ if (!ret && !*line_num)
+ gs_console_init(ports[*line_num].port);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(gserial_alloc_line);
/**
diff --git a/drivers/usb/gadget/function/u_serial.h
b/drivers/usb/gadget/function/u_serial.h
index 9acaac1cbb75..a86bd3ce4781 100644
--- a/drivers/usb/gadget/function/u_serial.h
+++ b/drivers/usb/gadget/function/u_serial.h
@@ -54,6 +54,7 @@ struct usb_request *gs_alloc_req(struct usb_ep *ep, unsigned
len, gfp_t flags);
void gs_free_req(struct usb_ep *, struct usb_request *req);
/* management of individual TTY ports */
+int gserial_alloc_line_raw(unsigned char *port_line);
int gserial_alloc_line(unsigned char *port_line);
void gserial_free_line(unsigned char port_line);
--
2.20.1
[PATCH v2 0/5] usb: gadget: u_serial: console for multiple ports
This series makes it possible to have more control over console using USB serial gadget ports. This can be useful when you need more than one USB console or are configuring multiple serial port function via configfs. The patches are against usb-next branch. Michał Mirosław (5): usb: gadget: u_serial: add missing port entry locking usb: gadget: u_serial: reimplement console support usb: gadget: u_serial: make OBEX port not a console usb: gadget: u_serial: allow more console gadget ports usb: gadget: u_serial: diagnose missed console messages drivers/usb/gadget/function/f_acm.c| 21 ++ drivers/usb/gadget/function/f_obex.c | 2 +- drivers/usb/gadget/function/f_serial.c | 21 ++ drivers/usb/gadget/function/u_serial.c | 419 ++--- drivers/usb/gadget/function/u_serial.h | 8 + 5 files changed, 283 insertions(+), 188 deletions(-) -- 2.20.1
Re: [PATCH v2 1/5] usb: gadget: u_serial: add missing port entry locking
On Wed, Feb 27, 2019 at 10:29:37AM +0100, Michał Mirosław wrote: > gserial_alloc_line() misses locking (for a release barrier) while > resetting port entry on TTY allocation failure. Fix this. > > Signed-off-by: Michał Mirosław > --- > v2: no changes Shouldn't you have added a: Cc: stable to this? :)
Re: [PATCH v2 3/5] usb: gadget: u_serial: make OBEX port not a console
On Wed, Feb 27, 2019 at 10:29:38AM +0100, Michał Mirosław wrote: > /* management of individual TTY ports */ > +int gserial_alloc_line_raw(unsigned char *port_line); > int gserial_alloc_line(unsigned char *port_line); What is the difference between "raw" and "not raw"? I can't even answer that question, and I maintain the tty layer, so what hope is there for someone else? :) nameing is hard, I know... thanks, greg k-h
Re: [PATCH v2 5/5] usb: gadget: u_serial: diagnose missed console messages
On Wed, Feb 27, 2019 at 10:29:39AM +0100, Michał Mirosław wrote: > Insert markers in console stream marking places where data > is missing. This makes the hole in the data stand out clearly > instead of glueing together unrelated messages. > > Signed-off-by: Michał Mirosław > --- > v2: commit message massage Provide an example of that what "marker" looks like in the commit log text? That would be nice to see... thanks, greg k-h
Re: [PATCH 2/2] usb: typec: add typec switch via GPIO control
On Mon, Feb 25, 2019 at 07:27:08AM +, Jun Li wrote: > This patch adds a simple typec switch driver which only needs > a GPIO to switch the super speed active channel according to > typec orientation. > > Signed-off-by: Li Jun > --- > drivers/usb/typec/mux/Kconfig | 6 +++ > drivers/usb/typec/mux/Makefile | 1 + > drivers/usb/typec/mux/gpio-switch.c | 105 > > 3 files changed, 112 insertions(+) > create mode 100644 drivers/usb/typec/mux/gpio-switch.c > > diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig > index 01ed0d5..bc7d3c7 100644 > --- a/drivers/usb/typec/mux/Kconfig > +++ b/drivers/usb/typec/mux/Kconfig > @@ -9,4 +9,10 @@ config TYPEC_MUX_PI3USB30532 > Say Y or M if your system has a Pericom PI3USB30532 Type-C cross > switch / mux chip found on some devices with a Type-C port. > > +config TYPEC_SWITCH_GPIO > + tristate "Simple Super Speed Active Switch via GPIO" depends on GPIOLIB? > + help > + Say Y or M if your system has a typec super speed channel > + switch via a simple GPIO control. > + thanks, -- heikki
Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote: > Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load > drm/kms drivers know about DisplayPort over Type-C hotplug events. > > Signed-off-by: Hans de Goede I'm OK with this. I'll wait for the v2 and see if I can test these. thanks, -- heikki
Re: [PATCH v2 3/5] usb: gadget: u_serial: make OBEX port not a console
On Wed, Feb 27, 2019 at 10:36:25AM +0100, Greg Kroah-Hartman wrote: > On Wed, Feb 27, 2019 at 10:29:38AM +0100, Michał Mirosław wrote: > > /* management of individual TTY ports */ > > +int gserial_alloc_line_raw(unsigned char *port_line); > > int gserial_alloc_line(unsigned char *port_line); > What is the difference between "raw" and "not raw"? I can't even answer > that question, and I maintain the tty layer, so what hope is there for > someone else? :) > > nameing is hard, I know... Yes it is. gserial_alloc_line_no_console() then? Best Regards, Michał Mirosław
Re: [PATCH v2 3/5] usb: gadget: u_serial: make OBEX port not a console
On Wed, Feb 27, 2019 at 11:14:27AM +0100, Michał Mirosław wrote: > On Wed, Feb 27, 2019 at 10:36:25AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Feb 27, 2019 at 10:29:38AM +0100, Michał Mirosław wrote: > > > /* management of individual TTY ports */ > > > +int gserial_alloc_line_raw(unsigned char *port_line); > > > int gserial_alloc_line(unsigned char *port_line); > > What is the difference between "raw" and "not raw"? I can't even answer > > that question, and I maintain the tty layer, so what hope is there for > > someone else? :) > > > > nameing is hard, I know... > > Yes it is. gserial_alloc_line_no_console() then? That makes a lot more sense to me, thank you. greg k-h
[PATCH v3 3/5] usb: gadget: u_serial: make OBEX port not a console
Prevent OBEX serial port from ever becoming a console. Console messages
will definitely break the protocol, and since you have to instantiate
the port making it explicitly for OBEX, there is no point in allowing
console to break it by mistake.
Signed-off-by: Michał Mirosław
---
v3: rename gserial_alloc_line_raw() -> gserial_alloc_line_no_console()
v2: change of API
+ commit message massage
---
drivers/usb/gadget/function/f_obex.c | 2 +-
drivers/usb/gadget/function/u_serial.c | 16
drivers/usb/gadget/function/u_serial.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/function/f_obex.c
b/drivers/usb/gadget/function/f_obex.c
index 55b7f57d2dc7..ab26d84ed95e 100644
--- a/drivers/usb/gadget/function/f_obex.c
+++ b/drivers/usb/gadget/function/f_obex.c
@@ -432,7 +432,7 @@ static struct usb_function_instance *obex_alloc_inst(void)
return ERR_PTR(-ENOMEM);
opts->func_inst.free_func_inst = obex_free_inst;
- ret = gserial_alloc_line(&opts->port_num);
+ ret = gserial_alloc_line_no_console(&opts->port_num);
if (ret) {
kfree(opts);
return ERR_PTR(ret);
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 8d2d861e1543..18f5a2e9abd3 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1179,7 +1179,7 @@ void gserial_free_line(unsigned char port_num)
}
EXPORT_SYMBOL_GPL(gserial_free_line);
-int gserial_alloc_line(unsigned char *line_num)
+int gserial_alloc_line_no_console(unsigned char *line_num)
{
struct usb_cdc_line_coding coding;
struct gs_port *port;
@@ -1220,12 +1220,20 @@ int gserial_alloc_line(unsigned char *line_num)
goto err;
}
*line_num = port_num;
-
- if (!port_num)
- gs_console_init(port);
err:
return ret;
}
+EXPORT_SYMBOL_GPL(gserial_alloc_line_no_console);
+
+int gserial_alloc_line(unsigned char *line_num)
+{
+ int ret = gserial_alloc_line_no_console(line_num);
+
+ if (!ret && !*line_num)
+ gs_console_init(ports[*line_num].port);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(gserial_alloc_line);
/**
diff --git a/drivers/usb/gadget/function/u_serial.h
b/drivers/usb/gadget/function/u_serial.h
index 9acaac1cbb75..8b472b0c8cb4 100644
--- a/drivers/usb/gadget/function/u_serial.h
+++ b/drivers/usb/gadget/function/u_serial.h
@@ -54,6 +54,7 @@ struct usb_request *gs_alloc_req(struct usb_ep *ep, unsigned
len, gfp_t flags);
void gs_free_req(struct usb_ep *, struct usb_request *req);
/* management of individual TTY ports */
+int gserial_alloc_line_no_console(unsigned char *port_line);
int gserial_alloc_line(unsigned char *port_line);
void gserial_free_line(unsigned char port_line);
--
2.20.1
[PATCH v3 1/5] usb: gadget: u_serial: add missing port entry locking
gserial_alloc_line() misses locking (for a release barrier) while resetting port entry on TTY allocation failure. Fix this. Cc: sta...@vger.kernel.org Signed-off-by: Michał Mirosław --- v3: added cc-stable v2: no changes --- drivers/usb/gadget/function/u_serial.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index 65f634ec7fc2..bb1e2e1d0076 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -1239,8 +1239,10 @@ int gserial_alloc_line(unsigned char *line_num) __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; } -- 2.20.1
[PATCH v3 5/5] usb: gadget: u_serial: diagnose missed console messages
Insert markers in console stream marking places where data
is missing. This makes the hole in the data stand out clearly
instead of glueing together unrelated messages.
Example output as seen from USB host side:
[0.064078] pinctrl core: registered pin 16 (UART3_RTS_N PC0) on
7868.pinmux
[0.064130] pinctrl
[missed 114987 bytes]
[4.302299] udevd[134]: starting version 3.2.5
[4.306845] random: udevd: uninitialized urandom read (16 bytes read)
Signed-off-by: Michał Mirosław
---
v3: added example output
+ lowecase 'missed'
v2: commit message massage
---
drivers/usb/gadget/function/u_serial.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 994882861db7..9e143bd081b6 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -88,6 +88,7 @@ struct gs_console {
spinlock_t lock;
struct usb_request *req;
struct kfifobuf;
+ size_t missed;
};
/*
@@ -930,6 +931,15 @@ static void __gs_console_push(struct gs_console *cons)
if (!size)
return;
+ if (cons->missed && ep->maxpacket >= 64) {
+ char buf[64];
+ size_t len;
+
+ len = sprintf(buf, "\n[missed %zu bytes]\n", cons->missed);
+ kfifo_in(&cons->buf, buf, len);
+ cons->missed = 0;
+ }
+
req->length = size;
if (usb_ep_queue(ep, req, GFP_ATOMIC))
req->length = 0;
@@ -951,10 +961,13 @@ static void gs_console_write(struct console *co,
{
struct gs_console *cons = container_of(co, struct gs_console, console);
unsigned long flags;
+ size_t n;
spin_lock_irqsave(&cons->lock, flags);
- kfifo_in(&cons->buf, buf, count);
+ n = kfifo_in(&cons->buf, buf, count);
+ if (n < count)
+ cons->missed += count - n;
if (cons->req && !cons->req->length)
schedule_work(&cons->work);
--
2.20.1
[PATCH v3 4/5] usb: gadget: u_serial: allow more console gadget ports
Allow configuring more than one console using USB serial or ACM gadget.
By default, only first (ttyGS0) is a console, but this may be changed
using function's new "console" attribute.
Signed-off-by: Michał Mirosław
---
v3: no changes
v2: no changes
---
drivers/usb/gadget/function/f_acm.c| 21
drivers/usb/gadget/function/f_serial.c | 21
drivers/usb/gadget/function/u_serial.c | 46 ++
drivers/usb/gadget/function/u_serial.h | 7
4 files changed, 95 insertions(+)
diff --git a/drivers/usb/gadget/function/f_acm.c
b/drivers/usb/gadget/function/f_acm.c
index 9fc98de83624..7c152c28b26c 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -771,6 +771,24 @@ static struct configfs_item_operations acm_item_ops = {
.release= acm_attr_release,
};
+#ifdef CONFIG_U_SERIAL_CONSOLE
+
+static ssize_t f_acm_console_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ return gserial_set_console(to_f_serial_opts(item)->port_num,
+ page, count);
+}
+
+static ssize_t f_acm_console_show(struct config_item *item, char *page)
+{
+ return gserial_get_console(to_f_serial_opts(item)->port_num, page);
+}
+
+CONFIGFS_ATTR(f_acm_, console);
+
+#endif /* CONFIG_U_SERIAL_CONSOLE */
+
static ssize_t f_acm_port_num_show(struct config_item *item, char *page)
{
return sprintf(page, "%u\n", to_f_serial_opts(item)->port_num);
@@ -779,6 +797,9 @@ static ssize_t f_acm_port_num_show(struct config_item
*item, char *page)
CONFIGFS_ATTR_RO(f_acm_, port_num);
static struct configfs_attribute *acm_attrs[] = {
+#ifdef CONFIG_U_SERIAL_CONSOLE
+ &f_acm_attr_console,
+#endif
&f_acm_attr_port_num,
NULL,
};
diff --git a/drivers/usb/gadget/function/f_serial.c
b/drivers/usb/gadget/function/f_serial.c
index c860f30a0ea2..1406255d0865 100644
--- a/drivers/usb/gadget/function/f_serial.c
+++ b/drivers/usb/gadget/function/f_serial.c
@@ -266,6 +266,24 @@ static struct configfs_item_operations serial_item_ops = {
.release= serial_attr_release,
};
+#ifdef CONFIG_U_SERIAL_CONSOLE
+
+static ssize_t f_serial_console_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ return gserial_set_console(to_f_serial_opts(item)->port_num,
+ page, count);
+}
+
+static ssize_t f_serial_console_show(struct config_item *item, char *page)
+{
+ return gserial_get_console(to_f_serial_opts(item)->port_num, page);
+}
+
+CONFIGFS_ATTR(f_serial_, console);
+
+#endif /* CONFIG_U_SERIAL_CONSOLE */
+
static ssize_t f_serial_port_num_show(struct config_item *item, char *page)
{
return sprintf(page, "%u\n", to_f_serial_opts(item)->port_num);
@@ -274,6 +292,9 @@ static ssize_t f_serial_port_num_show(struct config_item
*item, char *page)
CONFIGFS_ATTR_RO(f_serial_, port_num);
static struct configfs_attribute *acm_attrs[] = {
+#ifdef CONFIG_U_SERIAL_CONSOLE
+ &f_serial_attr_console,
+#endif
&f_serial_attr_port_num,
NULL,
};
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index 18f5a2e9abd3..994882861db7 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -1080,6 +1080,52 @@ static void gs_console_exit(struct gs_port *port)
port->console = NULL;
}
+ssize_t gserial_set_console(unsigned char port_num, const char *page, size_t
count)
+{
+ struct gs_port *port;
+ bool enable;
+ int ret = -ENXIO;
+
+ mutex_lock(&ports[port_num].lock);
+ port = ports[port_num].port;
+
+ if (WARN_ON(port == NULL))
+ goto out;
+
+ ret = strtobool(page, &enable);
+ if (ret)
+ goto out;
+
+ if (enable)
+ ret = gs_console_init(port);
+ else
+ gs_console_exit(port);
+
+ mutex_unlock(&ports[port_num].lock);
+out:
+ return ret < 0 ? ret : count;
+}
+EXPORT_SYMBOL_GPL(gserial_set_console);
+
+ssize_t gserial_get_console(unsigned char port_num, char *page)
+{
+ struct gs_port *port;
+ ssize_t ret = -ENXIO;
+
+ mutex_lock(&ports[port_num].lock);
+ port = ports[port_num].port;
+
+ if (WARN_ON(port == NULL))
+ goto out;
+
+ ret = sprintf(page, "%u\n", !!port->console);
+
+ mutex_unlock(&ports[port_num].lock);
+out:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gserial_get_console);
+
#else
static int gs_console_connect(struct gs_port *port)
diff --git a/drivers/usb/gadget/function/u_serial.h
b/drivers/usb/gadget/function/u_serial.h
index 8b472b0c8cb4..e5b08ab8cf7a 100644
--- a/drivers/usb/gadget/function/u_serial.h
+++ b/drivers/usb/gadget/function/u_serial.h
@@ -58,6 +58,13 @@ int gserial_alloc_line_no_console(unsigned char *port_line);
int gserial_alloc_line(unsigned c
[PATCH v3 0/5] usb: gadget: u_serial: console for multiple ports
This series makes it possible to have more control over console using USB serial gadget ports. This can be useful when you need more than one USB console or are configuring multiple serial port function via configfs. The patches are against usb-next branch. Michał Mirosław (5): usb: gadget: u_serial: add missing port entry locking usb: gadget: u_serial: reimplement console support usb: gadget: u_serial: make OBEX port not a console usb: gadget: u_serial: allow more console gadget ports usb: gadget: u_serial: diagnose missed console messages drivers/usb/gadget/function/f_acm.c| 21 ++ drivers/usb/gadget/function/f_obex.c | 2 +- drivers/usb/gadget/function/f_serial.c | 21 ++ drivers/usb/gadget/function/u_serial.c | 419 ++--- drivers/usb/gadget/function/u_serial.h | 8 + 5 files changed, 283 insertions(+), 188 deletions(-) -- 2.20.1
[PATCH v3 2/5] usb: gadget: u_serial: reimplement console support
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.
Only port ttyGS0 gets to be a console for now.
Signed-off-by: Michał Mirosław
---
v3: no changes
v2: no changes
---
drivers/usb/gadget/function/u_serial.c | 352 -
1 file changed, 164 insertions(+), 188 deletions(-)
diff --git a/drivers/usb/gadget/function/u_serial.c
b/drivers/usb/gadget/function/u_serial.c
index bb1e2e1d0076..8d2d861e1543 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_SIZE8192
/* console info */
-struct gscons_info {
- struct gs_port *port;
- struct task_struct *console_thread;
- struct kfifocon_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 kfifobuf;
};
/*
@@ -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
boolopenclose; /* 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,189 @@ 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_ep *ep;
+ struct usb_request *req = cons->req;
+ struct usb_ep *ep = cons->console.data;
+ size_t size = 0;
- 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;
+ size = kfifo_out(&cons->buf, req->buf, ep->maxpacket);
+ if (!size)
+ return;
- gs_request_free(req, ep)
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi Hans, On Mon, Feb 25, 2019 at 02:20:34PM +0100, Hans de Goede wrote: > Hi All, > > On some Cherry Trail devices, DisplayPort over Type-C is supported through > a USB-PD microcontroller (e.g. a fusb302) + a mux to switch the superspeed > datalines between USB-3 and DP (e.g. a pi3usb30532). The kernel in this > case does the PD/alt-mode negotiation itself, rather then everything being > handled in firmware. > > So the kernel itself picks an alt-mode, tells the Type-C "dongle" to switch > to DP mode and sets the mux accordingly. In this setup the HPD pin is not > connected, so the i915 driver needs to respond to a software event and scan > the DP port for changes manually. > > Thanks to Heikki's great work on the DisplayPort altmode support in the > typec subsys, we now correctly tell the dongle to switch to DP altmode > and we correctly set the mux and orientation switches to connect the > DP lines to the Type-C connector. > > This just leaves sending an out-of-band hotplug event from the Type-C > subsystem to the i915 driver and then we've fully working DP over Type-C > on these devices. > > This series implements this. The first patch adds a generic mechanism > for oob hotplug events to be send to the drm subsys, the second patch > adds support for this mechanism to the i915 driver and the third patch > makes the typec displayport_altmode driver send these events. > > The commit message of the first patch explains why I've chosen to things > the way these patches do them. One thing that this series does not consider is the DP lane count problem. The GPU drivers (i915 in this case) does not know is four, two or one DP lanes in use. I guess that is not a critical issue since there is a workaround (I think) where the driver basically does trial and error, but ideally we should be able to tell i915 also the pin assignment that was negotiated with the partner device so it knows the DP lane count. My original idea was that we pass that struct typec_displayport_data as payload in the event. From the Configuration VDO the GPU drivers can then see the negotiated DP pin assignment and determine the lane count. thanks, -- heikki
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, 27 Feb 2019, Heikki Krogerus wrote: > One thing that this series does not consider is the DP lane count > problem. The GPU drivers (i915 in this case) does not know is four, > two or one DP lanes in use. Also, orientation. > I guess that is not a critical issue since there is a workaround (I > think) where the driver basically does trial and error, but ideally we > should be able to tell i915 also the pin assignment that was > negotiated with the partner device so it knows the DP lane count. Yeah, if the information is there, we'd like to know. With the orientation, there's a worst case of sixth attempt of finding out there's just one lane in a certain orientation. Couple that with link rate selection (did it not work because too high link rate or because the lanes are just not there?) we get pretty confused about what we should try. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: Do usb_submit_urb() memory allocation errors really exist in the wild?
On Di, 2019-02-26 at 19:42 +0100, Petr Tesarik wrote: > On Tue, 26 Feb 2019 13:46:19 +0100 > Nicolas Saenz Julienne wrote: > > > [...] > > > USB and "critical" should never be in the same sentance, without the > > > word "not" being in there as well. :) > > > > Fair enough. It might still annoy someone, which computers were never meant > > to. > > :) > > I prefer to say "unrecoverable". While a block device may simply retry > a write, and nobody notices (unless it's to frequent), it's easily > audible (and may be annoying) if a USB sound card skips a frame. No, I am sorry but this is something I need to correct. You cannot easily recover from ENOMEM in a block driver. The reason is virtual memory. The common reason why you would get ENOMEM is memory pressure. Under memory pressure the system is writing out dirty pages, either to swap or a file. The SCSI layer can deal with blocked devices, but at some point, you need to make progress. In other words, the critical case is with GFP_NOIO. I think GFP_KERNEL will not realistically fail. GFP_NOIO, now that is a different question. Regards Oliver
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
On Wed, Feb 27, 2019 at 01:16:27PM +0200, Jani Nikula wrote: > On Wed, 27 Feb 2019, Heikki Krogerus wrote: > > One thing that this series does not consider is the DP lane count > > problem. The GPU drivers (i915 in this case) does not know is four, > > two or one DP lanes in use. > > Also, orientation. > > > I guess that is not a critical issue since there is a workaround (I > > think) where the driver basically does trial and error, but ideally we > > should be able to tell i915 also the pin assignment that was > > negotiated with the partner device so it knows the DP lane count. > > Yeah, if the information is there, we'd like to know. With the > orientation, there's a worst case of sixth attempt of finding out > there's just one lane in a certain orientation. Couple that with link > rate selection (did it not work because too high link rate or because > the lanes are just not there?) we get pretty confused about what we > should try. The orientation is also considered in the alt mode API. We have a function for that typec_altmode_get_orientation(), but it of course requires that the caller has a handle to the alt mode object. So basically we would again need tight coupling between the DP connector and USB Type-C connector. Hans, I'm not so sure we should, or can, rule out the "tight coupling" option after all. thanks, -- heikki
Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles
Johan Hovold writes:
> On Tue, Feb 26, 2019 at 05:07:10PM +, Mans Rullgard wrote:
>> The SIMCom SIM5218 and compatible devices have 5 USB interfaces, only 4
>> of which are serial ports. The fifth is a network interface supported
>> by the qmi-wwan driver. Furthermore, the serial ports do not support
>> modem control signals. Add driver_info flags to reflect this.
>>
>> Signed-off-by: Mans Rullgard
>> ---
>> drivers/usb/serial/option.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
>> index fb544340888b..af4cbfecc3ff 100644
>> --- a/drivers/usb/serial/option.c
>> +++ b/drivers/usb/serial/option.c
>> @@ -1066,7 +1066,8 @@ static const struct usb_device_id option_ids[] = {
>>.driver_info = RSVD(3) },
>> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x6613)}, /* Onda H600/ZTE MF330 */
>> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x0023)}, /* ONYX 3G device */
>> -{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000)}, /* SIMCom SIM5218 */
>> +{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000), /* SIMCom SIM5218 */
>> + .driver_info = NCTRL(0) | NCTRL(1) | NCTRL(2) | NCTRL(3) | RSVD(4) },
>> /* Quectel products using Qualcomm vendor ID */
>> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC15)},
>> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC20),
>
> Could you please provide the output of usb-devices (or lsusb -v) for
> this device?
lsusb -v:
Bus 001 Device 003: ID 05c6:9000 Qualcomm, Inc. SIMCom SIM5218 modem
Device Descriptor:
bLength18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass0
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize064
idVendor 0x05c6 Qualcomm, Inc.
idProduct 0x9000 SIMCom SIM5218 modem
bcdDevice0.00
iManufacturer 3 SimTech, Incorporated
iProduct2 SimTech SIM5360
iSerial 0
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 138
bNumInterfaces 5
bConfigurationValue 1
iConfiguration 1 SimTech Configuration
bmAttributes 0xe0
Self Powered
Remote Wakeup
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber0
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass255 Vendor Specific Subclass
bInterfaceProtocol255 Vendor Specific Protocol
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 32
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 32
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber1
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass255 Vendor Specific Subclass
bInterfaceProtocol255 Vendor Specific Protocol
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 32
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02 EP 2 OUT
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 32
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber2
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass255 Vendor Specific Subclass
bInterfaceProtocol255 Vendor Specific Protocol
iInt
Re: [PATCH v3 1/5] usb: gadget: u_serial: add missing port entry locking
On Wed, Feb 27, 2019 at 11:46:57AM +0100, Michał Mirosław wrote: > gserial_alloc_line() misses locking (for a release barrier) while > resetting port entry on TTY allocation failure. Fix this. > > Cc: sta...@vger.kernel.org > Signed-off-by: Michał Mirosław > Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v3 3/5] usb: gadget: u_serial: make OBEX port not a console
On Wed, Feb 27, 2019 at 11:46:58AM +0100, Michał Mirosław wrote: > Prevent OBEX serial port from ever becoming a console. Console messages > will definitely break the protocol, and since you have to instantiate > the port making it explicitly for OBEX, there is no point in allowing > console to break it by mistake. > > Signed-off-by: Michał Mirosław Much saner api, thanks for making these changes. Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v3 2/5] usb: gadget: u_serial: reimplement console support
On Wed, Feb 27, 2019 at 11:46:57AM +0100, Michał Mirosław 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. > > Only port ttyGS0 gets to be a console for now. > > Signed-off-by: Michał Mirosław It's a tough chunk of code to review, but I _think_ it looks correct. As long as you have actually tested it :) Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v3 5/5] usb: gadget: u_serial: diagnose missed console messages
On Wed, Feb 27, 2019 at 11:46:59AM +0100, Michał Mirosław wrote: > Insert markers in console stream marking places where data > is missing. This makes the hole in the data stand out clearly > instead of glueing together unrelated messages. > > Example output as seen from USB host side: > > [0.064078] pinctrl core: registered pin 16 (UART3_RTS_N PC0) on > 7868.pinmux > [0.064130] pinctrl > [missed 114987 bytes] > [4.302299] udevd[134]: starting version 3.2.5 > [4.306845] random: udevd: uninitialized urandom read (16 bytes read) > > Signed-off-by: Michał Mirosław Thanks for the changes here. Reviewed-by: Greg Kroah-Hartman
Re: [PATCH v3 4/5] usb: gadget: u_serial: allow more console gadget ports
On Wed, Feb 27, 2019 at 11:46:58AM +0100, Michał Mirosław wrote: > Allow configuring more than one console using USB serial or ACM gadget. > > By default, only first (ttyGS0) is a console, but this may be changed > using function's new "console" attribute. > > Signed-off-by: Michał Mirosław Reviewed-by: Greg Kroah-Hartman
Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles
Adding Bjørn.
On Wed, Feb 27, 2019 at 11:57:16AM +, Måns Rullgård wrote:
> Johan Hovold writes:
>
> > On Tue, Feb 26, 2019 at 05:07:10PM +, Mans Rullgard wrote:
> >> The SIMCom SIM5218 and compatible devices have 5 USB interfaces, only 4
> >> of which are serial ports. The fifth is a network interface supported
> >> by the qmi-wwan driver. Furthermore, the serial ports do not support
> >> modem control signals. Add driver_info flags to reflect this.
> >>
> >> Signed-off-by: Mans Rullgard
> >> ---
> >> drivers/usb/serial/option.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> >> index fb544340888b..af4cbfecc3ff 100644
> >> --- a/drivers/usb/serial/option.c
> >> +++ b/drivers/usb/serial/option.c
> >> @@ -1066,7 +1066,8 @@ static const struct usb_device_id option_ids[] = {
> >> .driver_info = RSVD(3) },
> >>{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x6613)}, /* Onda H600/ZTE MF330 */
> >>{ USB_DEVICE(QUALCOMM_VENDOR_ID, 0x0023)}, /* ONYX 3G device */
> >> - { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000)}, /* SIMCom SIM5218 */
> >> + { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000), /* SIMCom SIM5218 */
> >> +.driver_info = NCTRL(0) | NCTRL(1) | NCTRL(2) | NCTRL(3) | RSVD(4) },
> >>/* Quectel products using Qualcomm vendor ID */
> >>{ USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC15)},
> >>{ USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC20),
> >
> > Could you please provide the output of usb-devices (or lsusb -v) for
> > this device?
>
> lsusb -v:
> Bus 001 Device 003: ID 05c6:9000 Qualcomm, Inc. SIMCom SIM5218 modem
> Device Descriptor:
> bLength18
> bDescriptorType 1
> bcdUSB 2.00
> bDeviceClass0
> bDeviceSubClass 0
> bDeviceProtocol 0
> bMaxPacketSize064
> idVendor 0x05c6 Qualcomm, Inc.
> idProduct 0x9000 SIMCom SIM5218 modem
> bcdDevice0.00
> iManufacturer 3 SimTech, Incorporated
> iProduct2 SimTech SIM5360
> iSerial 0
> bNumConfigurations 1
> Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength 138
> bNumInterfaces 5
> bConfigurationValue 1
> iConfiguration 1 SimTech Configuration
> bmAttributes 0xe0
> Self Powered
> Remote Wakeup
> MaxPower 500mA
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber0
> bAlternateSetting 0
> bNumEndpoints 2
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass255 Vendor Specific Subclass
> bInterfaceProtocol255 Vendor Specific Protocol
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81 EP 1 IN
> bmAttributes2
> Transfer TypeBulk
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0200 1x 512 bytes
> bInterval 32
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x01 EP 1 OUT
> bmAttributes2
> Transfer TypeBulk
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0200 1x 512 bytes
> bInterval 32
> Interface Descriptor:
> bLength 9
> bDescriptorType 4
> bInterfaceNumber1
> bAlternateSetting 0
> bNumEndpoints 2
> bInterfaceClass 255 Vendor Specific Class
> bInterfaceSubClass255 Vendor Specific Subclass
> bInterfaceProtocol255 Vendor Specific Protocol
> iInterface 0
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x82 EP 2 IN
> bmAttributes2
> Transfer TypeBulk
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0200 1x 512 bytes
> bInterval 32
> Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x02 EP 2 OUT
> bmAttributes2
> Transfer TypeBulk
> Synch Type None
> Usage Type Data
> wMaxPacketSize 0x0200 1x 512 bytes
> bInterval 32
> Interface Descriptor:
> bLength 9
>
Re: usb: gadget: net2280: Fix function net2280_dequeue()
Zitat von Alan Stern :
On Tue, 19 Feb 2019 gu...@kiener-muenchen.de wrote:
Hi Alan,
My last proposal "udc: net2280: Fix overrun of OUT messages" is still
under investigation.
During the random stress tests I found a new rare problem:
When a request must be dequeued with net2280_dequeue() e.g. due
to a device clear action and the same request is finished by the
function scan_dma_completions():
https://elixir.bootlin.com/linux/v5.0-rc7/source/drivers/usb/gadget/udc/net2280.c#L1269
then the following search loop does not find the request and the function
returns the error -EINVAL without restoring the state ep->stopped = stopped!
https://elixir.bootlin.com/linux/v5.0-rc7/source/drivers/usb/gadget/udc/net2280.c#L1269
Thus the endpoint keeps blocked and does not receive any data.
When we insert ep->stopped = stopped here:
if (&req->req != _req) {
ep->stopped = stopped; // <
spin_unlock_irqrestore(&ep->dev->lock, flags);
dev_err(&ep->dev->pdev->dev, "%s: Request mismatch\n",
__func__);
return -EINVAL;
}
then the driver continues as expected, although the driver issues the
false error message "Request mismatch".
What do you think? It's labor-intensive to fix the error message here,
or shall we leave it as it is, and only insert the "ep->stopped = stopped;"
Yes, this is a bug. I think the dev_err() should be changed to
ep_dbg(). It's not a real error, after all -- it's just a race.
I agree. ep_dbg() is better here. I see the same bug in the sibling driver
net2272_dequeue():
https://elixir.bootlin.com/linux/v5.0-rc8/source/drivers/usb/gadget/udc/net2272.c#L948
The bug is quite obvious to me, however I'm not aware that we use this driver
in any of our instruments. I cannot test it. What do you recommend?
- Shall I fix it within the same commit.
- Shall I fix it with another commit.
- Don't care about it
Do that and insert the "ep->stopped = stopped;" line. Also, consider
changing the -EINVAL return code here to -EIDRM. This is analogous to
what usb_unlink_urb() does on the host side.
EIDRM sounds better. However I see that all udc drivers calling the
_dequeue() function returns -EINVAL when a request cannot be dequeued.
I do not dare to break this tradition.
BTW we tested my last proposal:
https://patchwork.kernel.org/patch/10796161/
... I see that we can simplify this patch when we just insert the
stop_out_nacking(ep) between line 909 and 910:
https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/net2280.c#L909
and remove the stop_out_naking() call in start_queue()...
I will add this improvement to the new patch sequence.
Guido
Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles
Johan Hovold writes:
> Adding Bjørn.
>
> On Wed, Feb 27, 2019 at 11:57:16AM +, Måns Rullgård wrote:
>> Johan Hovold writes:
>>
>> > On Tue, Feb 26, 2019 at 05:07:10PM +, Mans Rullgard wrote:
>> >> The SIMCom SIM5218 and compatible devices have 5 USB interfaces, only 4
>> >> of which are serial ports. The fifth is a network interface supported
>> >> by the qmi-wwan driver. Furthermore, the serial ports do not support
>> >> modem control signals. Add driver_info flags to reflect this.
>> >>
>> >> Signed-off-by: Mans Rullgard
>> >> ---
>> >> drivers/usb/serial/option.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
>> >> index fb544340888b..af4cbfecc3ff 100644
>> >> --- a/drivers/usb/serial/option.c
>> >> +++ b/drivers/usb/serial/option.c
>> >> @@ -1066,7 +1066,8 @@ static const struct usb_device_id option_ids[] = {
>> >> .driver_info = RSVD(3) },
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x6613)}, /* Onda H600/ZTE MF330 */
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x0023)}, /* ONYX 3G device */
>> >> - { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000)}, /* SIMCom SIM5218 */
>> >> + { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000), /* SIMCom SIM5218 */
>> >> + .driver_info = NCTRL(0) | NCTRL(1) | NCTRL(2) | NCTRL(3) | RSVD(4) },
>> >> /* Quectel products using Qualcomm vendor ID */
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC15)},
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC20),
>> >
>> > Could you please provide the output of usb-devices (or lsusb -v) for
>> > this device?
>>
>> lsusb -v:
>> [...]
> So the patch looks fine to me. The fifth interface is QMI, but hasn't
> been available for use until now then, and this seems to have been the
> vendors idea from the start:
>
>
> http://www.microchip.ua/simcom/WCDMA/APPNOTES/SIMCom_3G_Linux_driver_Application%20Note_V1.00.pdf
That document predates the qmi-wwan driver in the kernel. Note that
this driver has an ID table entry for interface 4 of this device. Right
now, whichever driver is probed first claims that interface. I haven't
actually tried using the QMI interface, though.
> And you're seeing errors when opening ports 0-3 due to the DTR calls
> which I guess no one noticed or cared about before?
Right, some userspace tools complain about this.
> Before you sent me the lsusb I searched for it and came across the below
> thread where Bjørn's having a go at SIMCom. In it there's output from a
> second device using the same id but with entirely different descriptors.
>
>
> https://forum.openwrt.org/t/lte-wireless-module-support-by-openwrt-led-on-tplink/13586?page=3
>
> If this is a common theme with this vendor we may need to be extra
> careful when making changes.
Isn't this a common theme with most USB vendors, especially wireless things?
Regardless, setting the NCTRL flag should be harmless.
--
Måns Rullgård
Re: [PATCH] usb: core: Fix typo in description of "authorized_default"
On Wed, Feb 27, 2019 at 12:33:32PM +0100, Jakub Wilk wrote: > Add missing right parenthesis. > > Signed-off-by: Jakub Wilk > --- > drivers/usb/core/hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 015b126ce455..c29ca6e8577c 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -379,7 +379,7 @@ module_param(authorized_default, int, S_IRUGO|S_IWUSR); > MODULE_PARM_DESC(authorized_default, > "Default USB device authorization: 0 is not authorized, 1 is " > "authorized, -1 is authorized except for wireless USB (default, > " > - "old behaviour"); > + "old behaviour)"); Nice find, thanks! greg k-h
Re: [PATCH] usb: core: Fix typo in description of "authorized_default"
On Wed, Feb 27, 2019 at 12:33:32PM +0100, Jakub Wilk wrote: > Add missing right parenthesis. > > Signed-off-by: Jakub Wilk > --- > drivers/usb/core/hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 015b126ce455..c29ca6e8577c 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -379,7 +379,7 @@ module_param(authorized_default, int, S_IRUGO|S_IWUSR); > MODULE_PARM_DESC(authorized_default, > "Default USB device authorization: 0 is not authorized, 1 is " > "authorized, -1 is authorized except for wireless USB (default, > " > - "old behaviour"); > + "old behaviour)"); > /*-*/ This line of text just changed in my tree, can you please check out linux-next and redo the patch based on that one? The error is still there. thanks, greg k-h
Re: [PATCH 0/3] Propagate DP-over-Type-C hotplug events from Type-C subsys to drm-drivers
Hi, On 27-02-19 12:16, Jani Nikula wrote: On Wed, 27 Feb 2019, Heikki Krogerus wrote: One thing that this series does not consider is the DP lane count problem. The GPU drivers (i915 in this case) does not know is four, two or one DP lanes in use. Also, orientation. The orientation should simply be correct with Type-C over DP. The mux / orientation-switch used will take care of (physically) swapping the lanes if the connector is inserted upside-down. I guess that is not a critical issue since there is a workaround (I think) where the driver basically does trial and error, but ideally we should be able to tell i915 also the pin assignment that was negotiated with the partner device so it knows the DP lane count. Yeah, if the information is there, we'd like to know. So far machines actually using a setup where the kernel does the USB-PD / Type-C negotiation rather then this being handled in firmware in say the Alpine Ridge controller, are very rare. AFAIK in the Alpine Ridge controller case, there is no communication with the i915 driver, the only thing the Alpine Ridge controller does which the everything done in the kernel approach does not, is that it actually has a pin connected to the HDP pin of the displayport in question. But that just lets the i915 driver know about hotplug-events, not how many lanes are used. Currently I'm aware of only 2 x86 models which actually need the hotplug event propagation from the Type-C subsystem to the i915 driver. Do we really want to come up with a much more complex solution to optimize for this corner case, while the much more common case (Alpine Ridge controller) does not provide this info to the i915 driver? To give some idea of the complexity, first of all we need some mechanism to let the kernel know which drm_connector is connected to which Type-C port. For the 2 models I know if which need this, this info is not available and we would need to hardcode it in the kernel based on e.g. DMI matching. Then once we have this link, we need to start thinking about probe ordering. Likely we need the typec framework to find the drm_connector, since the typec-partner device is only created when there actually is a DP capable "dongle" connected, making doing it the other way around tricky. Then the typec-partner device needs to get a reference or some such to make sure the drm_connector does not fgo away during the lifetime of the typec-partner device. I would really like to avoid this, so if we want to send more info to the i915 driver, I suggest we create some event struct for this which gets passed to the notifier, this could include a string to describe the DP connector which the Type-C connector is connected to when the mux is set to DP mode, e.g. "i915/DP-1" should be unique or probably better, use the PCI device name, so: ":00:02.0/DP-1" Then we still have a loose coupling avoiding lifetime issues, while the receiving drm driver can check which connector the event is for and we can then also include other info such as lane-count and orientation in the event struct. With the orientation As said, the orientation *should* be corrected by the mux switch, it is corrected by the mux switch on the hardware I know about and actually getting it wrong breaks the display output (we had a bug there), so I guess that getting it wrong leads to the lines being swizzled in a way which the i915 driver does not check for ... Regards, Hans
Re: [PATCH 3/3] usb: typec: altmodes/displayport: Notify drm subsys of hotplug events
Hi, On 27-02-19 10:44, Heikki Krogerus wrote: On Mon, Feb 25, 2019 at 02:20:37PM +0100, Hans de Goede wrote: Use the new drm_kms_call_oob_hotplug_notifier_chain() function to load drm/kms drivers know about DisplayPort over Type-C hotplug events. Signed-off-by: Hans de Goede I'm OK with this. I'll wait for the v2 and see if I can test these. The only change I've queued for v2 is adding a "depends on DRM" to the TYPEC_DP_ALTMODE Kconfig. And given the discussion about passing lane-info, it might be a while before we get a v2, so if you want to give this a test run, it is probably best to just test v1 for now (if you've time). Note to test this on the GPD win (which you have AFAIK) you will also need the fusb302 + pi3usb30532 patches I've send out recently, as well as: https://github.com/jwrdegoede/linux-sunxi/commit/945c6fe0a18957357b42e04ed34bf33667003030 I've one Type-C to VGA dongle (without any other functions) where the Type-C mode negotiation fails. This one does work on a XPS 15 so I need to borrow some hardware from a friend so that I can capture the USB-PD signals and see what the Alpine Ridge controller does different compared to the in kernel stack and fix this. My other 4 dongles work fine, including this "standard" model: http://media.redgamingtech.com/rgt-website/2015/03/Apple-HDMI-Usb-Type-C-dongle.jpg Regards, Hans
[PATCH] usb: core: Fix typo in description of "authorized_default"
Add missing right parenthesis. Signed-off-by: Jakub Wilk --- drivers/usb/core/hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 015b126ce455..c29ca6e8577c 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -379,7 +379,7 @@ module_param(authorized_default, int, S_IRUGO|S_IWUSR); MODULE_PARM_DESC(authorized_default, "Default USB device authorization: 0 is not authorized, 1 is " "authorized, -1 is authorized except for wireless USB (default, " - "old behaviour"); + "old behaviour)"); /*-*/ /** -- 2.20.1
Re: [PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property
Hi Rob, Thanks for the information. Please find my comments below. Regards, Srinath. On Tue, Feb 26, 2019 at 11:33 PM Rob Herring wrote: > > On Mon, Feb 25, 2019 at 10:57 PM Srinath Mannam > wrote: > > > > Hi Rob, > > Thanks for the review, Please see my comments below in line. > > > > Regards, > > Srinath. > > On Tue, Feb 26, 2019 at 3:08 AM Rob Herring wrote: > > > > > > On Tue, Feb 05, 2019 at 11:48:53AM +0530, Srinath Mannam wrote: > > > > Add usb-phy-port-reset optional property to set quirk in xhci platform > > > > driver which forces USB port PHY reset on port disconnect event. > > > > > > > > Signed-off-by: Srinath Mannam > > > > Reviewed-by: Ray Jui > > > > --- > > > > Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt > > > > b/Documentation/devicetree/bindings/usb/usb-xhci.txt > > > > index fea8b15..ecbdb15 100644 > > > > --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt > > > > +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt > > > > @@ -40,6 +40,7 @@ Optional properties: > > > >- usb3-lpm-capable: determines if platform is USB3 LPM capable > > > >- quirk-broken-port-ped: set if the controller has broken port > > > > disable mechanism > > > >- imod-interval-ns: default interrupt moderation interval is 5000ns > > > > + - usb-phy-port-reset: set this to do USB PORT PHY reset while > > > > disconnect > > > >- phys : see usb-hcd.txt in the current directory > > > > > > This should be implied by the HCI or phy compatible string (depending > > > on who exactly needs the quirky behavior). > > Stingray USB HS PHY connected to xHCI port has an issue, if full speed > > devices connected to this port then > > after all High Speed devices connected to this port are detected at > > full speed instead of high speed. > > So that we need to do PHY (which is connected to port) reset on xHCI > > port disconnect event. > > That is the reason we required to add quirk in xHCI. > > So, by looking at the xhci host and phy compatible strings (or maybe > just the phy) you can determine whether you need to reset the port or > not. All the information you need is in DT already. xHCI controller in our SOC has three ports each port has one PHY(SS/HS) connected to it. HS PHY has to reset on its corresponding port disconnect event. port disconnect event is captured in xHCI host framework so, quirk has to be registered in xHCI framework only. But we are using "generic-xhci" generic compatible string for our xHCI controller. As per your advice, we will add new compatible string in xhci-plat.c driver for our xHCI controller and will add quirk part of that. Thank you. > > Rob
[PATCH v2] usb: core: Fix typo in description of "authorized_default"
Add missing right parenthesis. Signed-off-by: Jakub Wilk --- drivers/usb/core/hcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 3b6e3e25f59e..3189181bb628 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -385,7 +385,7 @@ module_param(authorized_default, int, S_IRUGO|S_IWUSR); MODULE_PARM_DESC(authorized_default, "Default USB device authorization: 0 is not authorized, 1 is " "authorized, 2 is authorized for internal devices, -1 is " - "authorized except for wireless USB (default, old behaviour"); + "authorized except for wireless USB (default, old behaviour)"); /*-*/ /** -- 2.11.0
Re: Bus noise periodically causes ci_hdrc IRQ lockup
On 2/27/19 2:22 AM, Peter Chen wrote: Chandler From the above, we know the controller is at RX active status. But I am sorry I got the imprecise instruction from IC guys. The reason we do that is to know the RX status change during one packet, for your example, there are ISOC packets, so, measure time is about 20us. (1024 bytes). First, you may need to know the time between two reads, it needs to be within 1-10us, then we need to know all reads within 20us, you could save the read value at memory, and print it out after all reads are done. Besides, You need to write debug register from 1 to 8 (not 7). for (write USBPHY.USBPHYx_DEBUG1n($USBPHY + 0x70) from 1 to 8) read all USBPHY.USBPHYx_DEBUG0_STATUS value within 20us I am not sure if you could do above at userspace. Peter Hi Peter, I managed to preform those requests in 20uS. The results are not much different. Time: 18uS 0x020CA060 @ 0x1: 0x7B2C 0x020CA060 @ 0x2: 0x0064 0x020CA060 @ 0x3: 0x108401C0 (still changes on every read) 0x020CA060 @ 0x4: 0x00010001 0x020CA060 @ 0x5: 0x01011101 0x020CA060 @ 0x6: 0x0101 0x020CA060 @ 0x7: 0x05300010 0x020CA060 @ 0x8: 0x8101 I should mention that PORTSC and other registers (besides the DEBUG0_STATUS @ 3) don't change at all between reads. I did manage to find a way to reset the root hub through GPIO, and I see the following changes occur: PORTSC reg: 18001a05 --> 18001205 0x020CA060 @ 0x2: 0x0064 --> 0x0024 After this change the registers remain static and will go back the previous state after releasing reset. Let me summary your observation: - bind/unbind ci_hdrc device can recover connection - Reset HUB can't recover, and will go the previous error state after reset From the register, we do see something abnormal, and the RX is waiting the SYNC Field. We need to see the dp/dm status to know if HUB is wrong, eg, sending data exceed 20us (larger than 1024 bytes) I will continue looking into probing Dm/Dp. You would like me to do this *while* the failure occurs, or after? After the error occurs. Peter Hi Peter, That summary is accurate. I soldered some leads onto the host/hub connection and hooked up to oscilloscope. The findings were interesting: Directly after failure: 0x020CA060 @ 0x2: 0x0064 PORTSC reg: 18001a05 Dm line: 150 mV Dp line: 0 V After failure, hub reset asserted: 0x020CA060 @ 0x2: 0x0024 PORTSC reg: 18001205 Dm line: 0 V Dp line: 0 V After failure, hub reset released: 0x020CA060 @ 0x2: 0x0064 PORTSC reg: 18001a05 Dm line: 150 mV Dp line: 0 V It seems strange that it would switch between those voltages -- could the hub and host be trying to write different values at the same time? I have noticed something new happening (maybe as a result of hooking up the probe?). A couple times now after initial failure, the device has changed states later. In this state the Linux USB devices appear to 'wake up' and start throwing errors. (failure occurs) [ 227.323636] smsc95xx 1-1.4.1:1.0 eth1: Failed to read reg index 0x0114: -110 [ 227.323659] smsc95xx 1-1.4.1:1.0 eth1: Error reading MII_ACCESS [ 227.323677] smsc95xx 1-1.4.1:1.0 eth1: MII is busy in smsc95xx_mdio_read [ 227.323694] smsc95xx 1-1.4.1:1.0 eth1: Failed to read MII_BMSR (no errors for 25 minutes, then something changes) [ 1752.092896] uvcvideo: Non-zero status (-71) in video completion handler. [ 1752.124744] uvcvideo: Non-zero status (-71) in video completion handler. [ 1752.124866] usb 1-1.4: clear tt 3 (91c1) error -71 ...lots of errors... Registers: 0x020CA060 @ 0x1: 0x 0x020CA060 @ 0x2: 0x00140060 0x020CA060 @ 0x3: 0x10801110 0x020CA060 @ 0x4: 0x00010001 0x020CA060 @ 0x5: 0x01011101 0x020CA060 @ 0x6: 0x0101 0x020CA060 @ 0x7: 0x06200010 (changing) 0x020CA060 @ 0x8: 0x1101 PORTSC reg: steady at 10001801 Dm line: steady at 3 V Dp line: steady at 0 V And after that, when I reset the hub it returns to normal operation. ...lots of errors... [ 1977.285844] usb 1-1.4: clear tt 3 (91c1) error -71 (hub reset asserted) [ 1977.309718] usb 1-1: USB disconnect, device number 2 (hub reset released) [ 2088.226453] usb 1-1: new high-speed USB device number 29 using ci_hdrc Let me know what you think of this, Chandler
[PATCH] xhci: use iopoll for xhci_handshake
In cases such as xhci_abort_cmd_ring(), xhci_handshake() is called with
a spin lock held (and local interrupts disabled) with a huge 5 second
timeout. This can translates to 5 million calls to udelay(1). By its
very nature, udelay() is not meant to be precise, it only guarantees to
delay a minimum of 1 microsecond. Therefore the actual delay of
xhci_handshake() can be significantly longer. If the average udelay(1)
is greater than 2.2 us, the total time in xhci_handshake() - with
interrupts disabled can be > 11 seconds triggering the kernel's soft lockup
detector.
To avoid this, let's replace the open coded io polling loop with one from
iopoll.h that uses a loop timed with the more presumably reliable ktime
infrastructure.
Signed-off-by: Daniel Kurtz
---
drivers/usb/host/xhci.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608e..fde5ff84ba69b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include "xhci.h"
#include "xhci-trace.h"
@@ -69,18 +70,18 @@ static bool td_on_ring(struct xhci_td *td, struct xhci_ring
*ring)
int xhci_handshake(void __iomem *ptr, u32 mask, u32 done, int usec)
{
u32 result;
+ int ret;
- do {
- result = readl(ptr);
- if (result == ~(u32)0) /* card removed */
- return -ENODEV;
- result &= mask;
- if (result == done)
- return 0;
- udelay(1);
- usec--;
- } while (usec > 0);
- return -ETIMEDOUT;
+ ret = readl_poll_timeout_atomic(ptr, result,
+ ((result & mask) == done || (result == ~(u32)0)),
+ 1, usec);
+ if (ret)
+ return ret;
+
+ if (result == ~(u32)0) /* card removed */
+ return -ENODEV;
+
+ return 0;
}
/*
--
2.21.0.rc2.261.ga7da99ff1b-goog
RE: Bus noise periodically causes ci_hdrc IRQ lockup
> > Let me summary your observation: > > - bind/unbind ci_hdrc device can recover connection > > - Reset HUB can't recover, and will go the previous error state after > > reset > > > > From the register, we do see something abnormal, and the RX is > > waiting the SYNC Field. We need to see the dp/dm status to know if HUB > > is wrong, eg, sending data exceed 20us (larger than 1024 bytes) > > > >> I will continue looking into probing Dm/Dp. You would like me to do > >> this > >> *while* the failure occurs, or after? > >> > > After the error occurs. > > > > Peter > > > Hi Peter, > > That summary is accurate. > > I soldered some leads onto the host/hub connection and hooked up to > oscilloscope. > The findings were interesting: > > Directly after failure: >0x020CA060 @ 0x2: 0x0064 >PORTSC reg: 18001a05 >Dm line: 150 mV >Dp line: 0 V > After failure, hub reset asserted: >0x020CA060 @ 0x2: 0x0024 >PORTSC reg: 18001205 >Dm line: 0 V >Dp line: 0 V > After failure, hub reset released: >0x020CA060 @ 0x2: 0x0064 >PORTSC reg: 18001a05 >Dm line: 150 mV >Dp line: 0 V > > It seems strange that it would switch between those voltages -- could the hub > and > host be trying to write different values at the same time? > HUB and host are impossible to send the data together. > I have noticed something new happening (maybe as a result of hooking up the > probe?). > A couple times now after initial failure, the device has changed states later. > In this state the Linux USB devices appear to 'wake up' and start throwing > errors. > > (failure occurs) > [ 227.323636] smsc95xx 1-1.4.1:1.0 eth1: Failed to read reg index > 0x0114: - > 110 [ 227.323659] smsc95xx 1-1.4.1:1.0 eth1: Error reading MII_ACCESS > [ 227.323677] smsc95xx 1-1.4.1:1.0 eth1: MII is busy in smsc95xx_mdio_read > [ 227.323694] smsc95xx 1-1.4.1:1.0 eth1: Failed to read MII_BMSR > (no errors for 25 minutes, then something changes) [ 1752.092896] > uvcvideo: > Non-zero status (-71) in video completion handler. > [ 1752.124744] uvcvideo: Non-zero status (-71) in video completion handler. > [ 1752.124866] usb 1-1.4: clear tt 3 (91c1) error -71 > ...lots of errors... > > Registers: > 0x020CA060 @ 0x1: 0x > 0x020CA060 @ 0x2: 0x00140060 > 0x020CA060 @ 0x3: 0x10801110 > 0x020CA060 @ 0x4: 0x00010001 > 0x020CA060 @ 0x5: 0x01011101 > 0x020CA060 @ 0x6: 0x0101 > 0x020CA060 @ 0x7: 0x06200010 (changing) > 0x020CA060 @ 0x8: 0x1101 >PORTSC reg: steady at 10001801 >Dm line: steady at 3 V >Dp line: steady at 0 V > > And after that, when I reset the hub it returns to normal operation. > >...lots of errors... > [ 1977.285844] usb 1-1.4: clear tt 3 (91c1) error -71 > (hub reset asserted) > [ 1977.309718] usb 1-1: USB disconnect, device number 2 > (hub reset released) > [ 2088.226453] usb 1-1: new high-speed USB device number 29 using ci_hdrc > > Let me know what you think of this, > It seems you record DM/DP opposite, please confirm it. Besides, - Do you observe this USB issue at specific board or some boards? - After connecting probe, sometimes the reset HUB can recover, and somethings can't? - When HUB's reset is asserted, does the register dump and measure are like below (can't recover situation): 0x020CA060 @ 0x1: 0x7B2C 0x020CA060 @ 0x2: 0x0024 0x020CA060 @ 0x3: 0x108401C0 (still changes on every read) 0x020CA060 @ 0x4: 0x00010001 0x020CA060 @ 0x5: 0x01011101 0x020CA060 @ 0x6: 0x0101 0x020CA060 @ 0x7: 0x05300010 0x020CA060 @ 0x8: 0x8101 PORTSC reg: 18001205 Dm line: 0 V Dp line: 0 V Besides, I need your whole kernel log with and without using probe, your original kernel log at github is ok (but need to let me access). I need to know if bus reset and bus suspend occur during the whole process. My guesses are: First log: the HUB enters FS with unknown reason, it adds its 1.5 Kohm (minimum can be 900ohm) @3.3V, and the host is 45ohm, so the host sees it is ~150mV. The host controller is stuck at HS rxactive state at this situation forever. Second log: the host controller is ok; the hub is wrong. The HUB can't be recovered by bus reset, only hardware reset HUB can be recovered. Next step: - Try USBPHYx_RXn.ENVADJ (0x20ca020) as 0x3 and 0x1, see if something changes. - When the error occurs, what code will run and what log will show, we could try the recovery method. Peter
Re: [PATCH] xhci: use iopoll for xhci_handshake
On Wed, Feb 27, 2019 at 03:19:17PM -0700, Daniel Kurtz wrote: > In cases such as xhci_abort_cmd_ring(), xhci_handshake() is called with > a spin lock held (and local interrupts disabled) with a huge 5 second > timeout. This can translates to 5 million calls to udelay(1). By its > very nature, udelay() is not meant to be precise, it only guarantees to > delay a minimum of 1 microsecond. Therefore the actual delay of > xhci_handshake() can be significantly longer. If the average udelay(1) > is greater than 2.2 us, the total time in xhci_handshake() - with > interrupts disabled can be > 11 seconds triggering the kernel's soft lockup > detector. > > To avoid this, let's replace the open coded io polling loop with one from > iopoll.h that uses a loop timed with the more presumably reliable ktime > infrastructure. > > Signed-off-by: Daniel Kurtz Looks sane to me, nice fixup. Reviewed-by: Greg Kroah-Hartman Is this causing problems on older kernels/devices today such that we should backport this? thanks, greg k-h
