[PATCH 0/5] usb: xhci: dbc: make modular and add RAW interface

[PATCH 0/5] usb: xhci: dbc: make modular and add RAW interface

[Prabhat Chand Pandey]

This patch-set adds the following features to dbc driver:

- show the active dbc function and dbc descriptors, allowing
  user space to dynamically modify the descriptors.

- modularize dbc core to enable it to expose different function
  interfaces, till now only TTY interface was exposed.

- use the new framework to expose RAW interface that can be
  used by any user-space application to directly read from and 
  write into dbc bulk end-points.


Abhilash K V (1):
  usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

K V, Abhilash (1):
  usb: xhci: dbc: Provide sysfs option to configure dbc descriptors

Prabhat Chand Pandey (3):
  usb: xhci: dbc: make DbC modular, introducing dbc_function structure
  usb: xhci: dbc: DbC TTY driver to use new interface
  usb: xhci: dbc: Document describe about dbc raw interface

 .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 112 ++++
 Documentation/usb/dbc_raw.rst                 | 136 +++++
 Documentation/usb/index.rst                   |  16 +
 drivers/usb/host/Kconfig                      |  24 +-
 drivers/usb/host/Makefile                     |   5 +-
 drivers/usb/host/xhci-dbgcap.c                | 498 ++++++++++++++++--
 drivers/usb/host/xhci-dbgcap.h                |  36 +-
 drivers/usb/host/xhci-dbgraw.c                | 365 +++++++++++++
 drivers/usb/host/xhci-dbgtty.c                |  81 ++-
 9 files changed, 1206 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/usb/dbc_raw.rst
 create mode 100644 Documentation/usb/index.rst
 create mode 100644 drivers/usb/host/xhci-dbgraw.c

-- 
2.21.0

[PATCH 1/5] usb: xhci: dbc: make DbC modular, introducing dbc_function structure

[Prabhat Chand Pandey]

This patch introduces the dbc_function structure as a first step in
making DbC modular and capable in exposing different user interfaces using
different "functions", which may implement the callbacks exposed here
according to the driver's need.

Only one "function" can be registered at a time.
The generic way to enable a DbC function would be, using sys-fs:

Locate the directory for PCI enumerated XHCI host controller in the
sysfs path.
e.g.: cd /sys/bus/pci/devices/0000:00:14.0
$ echo "enable" > dbc

This is done AFTER the function is selected at build time. Only 1 function
can be selected at a time.

Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-dbgcap.c | 159 ++++++++++++++++++++++-----------
 drivers/usb/host/xhci-dbgcap.h |  32 ++++++-
 2 files changed, 138 insertions(+), 53 deletions(-)

diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 52e32644a4b2..96adc53b0fdb 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -9,11 +9,14 @@
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
 #include <linux/nls.h>
+#include <linux/module.h>
 
 #include "xhci.h"
 #include "xhci-trace.h"
 #include "xhci-dbgcap.h"
 
+static struct dbc_function *dbc_registered_func;
+
 static inline void *
 dbc_dma_alloc_coherent(struct xhci_hcd *xhci, size_t size,
 		       dma_addr_t *dma_handle, gfp_t flags)
@@ -35,41 +38,42 @@ dbc_dma_free_coherent(struct xhci_hcd *xhci, size_t size,
 				  size, cpu_addr, dma_handle);
 }
 
-static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings)
+static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings,
+					struct dbc_function *func)
 {
 	struct usb_string_descriptor	*s_desc;
 	u32				string_length;
 
 	/* Serial string: */
 	s_desc = (struct usb_string_descriptor *)strings->serial;
-	utf8s_to_utf16s(DBC_STRING_SERIAL, strlen(DBC_STRING_SERIAL),
+	utf8s_to_utf16s(func->string.serial, strlen(func->string.serial),
 			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
 			DBC_MAX_STRING_LENGTH);
 
-	s_desc->bLength		= (strlen(DBC_STRING_SERIAL) + 1) * 2;
+	s_desc->bLength		= (strlen(func->string.serial) + 1) * 2;
 	s_desc->bDescriptorType	= USB_DT_STRING;
 	string_length		= s_desc->bLength;
 	string_length		<<= 8;
 
 	/* Product string: */
 	s_desc = (struct usb_string_descriptor *)strings->product;
-	utf8s_to_utf16s(DBC_STRING_PRODUCT, strlen(DBC_STRING_PRODUCT),
+	utf8s_to_utf16s(func->string.product, strlen(func->string.product),
 			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
 			DBC_MAX_STRING_LENGTH);
 
-	s_desc->bLength		= (strlen(DBC_STRING_PRODUCT) + 1) * 2;
+	s_desc->bLength		= (strlen(func->string.product) + 1) * 2;
 	s_desc->bDescriptorType	= USB_DT_STRING;
 	string_length		+= s_desc->bLength;
 	string_length		<<= 8;
 
 	/* Manufacture string: */
 	s_desc = (struct usb_string_descriptor *)strings->manufacturer;
-	utf8s_to_utf16s(DBC_STRING_MANUFACTURER,
-			strlen(DBC_STRING_MANUFACTURER),
+	utf8s_to_utf16s(func->string.manufacturer,
+			strlen(func->string.manufacturer),
 			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
 			DBC_MAX_STRING_LENGTH);
 
-	s_desc->bLength		= (strlen(DBC_STRING_MANUFACTURER) + 1) * 2;
+	s_desc->bLength		= (strlen(func->string.manufacturer) + 1) * 2;
 	s_desc->bDescriptorType	= USB_DT_STRING;
 	string_length		+= s_desc->bLength;
 	string_length		<<= 8;
@@ -84,7 +88,9 @@ static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings)
 	return string_length;
 }
 
-static void xhci_dbc_init_contexts(struct xhci_hcd *xhci, u32 string_length)
+static void xhci_dbc_init_contexts(struct xhci_hcd *xhci,
+					struct dbc_function *func,
+					u32 string_length)
 {
 	struct xhci_dbc		*dbc;
 	struct dbc_info_context	*info;
@@ -124,10 +130,10 @@ static void xhci_dbc_init_contexts(struct xhci_hcd *xhci, u32 string_length)
 	/* Set DbC context and info registers: */
 	xhci_write_64(xhci, dbc->ctx->dma, &dbc->regs->dccp);
 
-	dev_info = cpu_to_le32((DBC_VENDOR_ID << 16) | DBC_PROTOCOL);
+	dev_info = cpu_to_le32((func->vid << 16) | func->protocol);
 	writel(dev_info, &dbc->regs->devinfo1);
 
-	dev_info = cpu_to_le32((DBC_DEVICE_REV << 16) | DBC_PRODUCT_ID);
+	dev_info = cpu_to_le32((func->device_rev << 16) | func->pid);
 	writel(dev_info, &dbc->regs->devinfo2);
 }
 
@@ -181,11 +187,13 @@ static void xhci_dbc_flush_endpoint_requests(struct dbc_ep *dep)
 		xhci_dbc_flush_single_request(req);
 }
 
-static void xhci_dbc_flush_requests(struct xhci_dbc *dbc)
+
+void xhci_dbc_flush_requests(struct xhci_dbc *dbc)
 {
 	xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_OUT]);
 	xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_IN]);
 }
+EXPORT_SYMBOL_GPL(xhci_dbc_flush_requests);
 
 struct dbc_request *
 dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags)
@@ -205,6 +213,7 @@ dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags)
 
 	return req;
 }
+EXPORT_SYMBOL_GPL(dbc_alloc_request);
 
 void
 dbc_free_request(struct dbc_ep *dep, struct dbc_request *req)
@@ -213,6 +222,7 @@ dbc_free_request(struct dbc_ep *dep, struct dbc_request *req)
 
 	kfree(req);
 }
+EXPORT_SYMBOL_GPL(dbc_free_request);
 
 static void
 xhci_dbc_queue_trb(struct xhci_ring *ring, u32 field1,
@@ -344,6 +354,7 @@ int dbc_ep_queue(struct dbc_ep *dep, struct dbc_request *req,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dbc_ep_queue);
 
 static inline void xhci_dbc_do_eps_init(struct xhci_hcd *xhci, bool direction)
 {
@@ -371,7 +382,9 @@ static void xhci_dbc_eps_exit(struct xhci_hcd *xhci)
 	memset(dbc->eps, 0, sizeof(struct dbc_ep) * ARRAY_SIZE(dbc->eps));
 }
 
-static int xhci_dbc_mem_init(struct xhci_hcd *xhci, gfp_t flags)
+static int xhci_dbc_mem_init(struct xhci_hcd *xhci,
+				struct dbc_function *func,
+				gfp_t flags)
 {
 	int			ret;
 	dma_addr_t		deq;
@@ -418,8 +431,8 @@ static int xhci_dbc_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 	xhci_write_64(xhci, deq, &dbc->regs->erdp);
 
 	/* Setup strings and contexts: */
-	string_length = xhci_dbc_populate_strings(dbc->string);
-	xhci_dbc_init_contexts(xhci, string_length);
+	string_length = xhci_dbc_populate_strings(dbc->string, func);
+	xhci_dbc_init_contexts(xhci, func, string_length);
 
 	xhci_dbc_eps_init(xhci);
 	dbc->state = DS_INITIALIZED;
@@ -478,20 +491,9 @@ static int xhci_do_dbc_start(struct xhci_hcd *xhci)
 	u32			ctrl;
 	struct xhci_dbc		*dbc = xhci->dbc;
 
-	if (dbc->state != DS_DISABLED)
+	if (dbc->state != DS_INITIALIZED)
 		return -EINVAL;
 
-	writel(0, &dbc->regs->control);
-	ret = xhci_handshake(&dbc->regs->control,
-			     DBC_CTRL_DBC_ENABLE,
-			     0, 1000);
-	if (ret)
-		return ret;
-
-	ret = xhci_dbc_mem_init(xhci, GFP_ATOMIC);
-	if (ret)
-		return ret;
-
 	ctrl = readl(&dbc->regs->control);
 	writel(ctrl | DBC_CTRL_DBC_ENABLE | DBC_CTRL_PORT_ENABLE,
 	       &dbc->regs->control);
@@ -552,9 +554,13 @@ static void xhci_dbc_stop(struct xhci_hcd *xhci)
 
 	cancel_delayed_work_sync(&dbc->event_work);
 
-	if (port->registered)
-		xhci_dbc_tty_unregister_device(xhci);
-
+	if (port->registered &&
+	    dbc_registered_func &&
+	    dbc_registered_func->post_disconnect) {
+		ret = dbc_registered_func->post_disconnect(dbc);
+		if (ret)
+			xhci_err(xhci, "dbc post_disconnect error %d\n", ret);
+	}
 	spin_lock_irqsave(&dbc->lock, flags);
 	ret = xhci_do_dbc_stop(xhci);
 	spin_unlock_irqrestore(&dbc->lock, flags);
@@ -798,16 +804,12 @@ static void xhci_dbc_handle_events(struct work_struct *work)
 
 	switch (evtr) {
 	case EVT_GSER:
-		ret = xhci_dbc_tty_register_device(xhci);
-		if (ret) {
-			xhci_err(xhci, "failed to alloc tty device\n");
-			break;
-		}
-
-		xhci_info(xhci, "DbC now attached to /dev/ttyDBC0\n");
+		if (dbc_registered_func->post_config)
+			dbc_registered_func->post_config(dbc);
 		break;
 	case EVT_DISC:
-		xhci_dbc_tty_unregister_device(xhci);
+		if (dbc_registered_func->post_disconnect)
+			ret = dbc_registered_func->post_disconnect(dbc);
 		break;
 	case EVT_DONE:
 		break;
@@ -912,46 +914,76 @@ static ssize_t dbc_store(struct device *dev,
 			 struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
+	struct xhci_dbc		*dbc;
 	struct xhci_hcd		*xhci;
+	int			ret = 0;
 
 	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
 
-	if (!strncmp(buf, "enable", 6))
+	if (!strncmp(buf, "enable", 6) && dbc->state == DS_DISABLED) {
+		if (!dbc_registered_func)
+			return -EINVAL;
+		if (!try_module_get(dbc_registered_func->owner))
+			return -ENODEV;
+		ret = xhci_dbc_mem_init(dbc->xhci, dbc_registered_func,
+						GFP_ATOMIC);
+		if (ret)
+			goto err;
+		if (dbc_registered_func->run)
+			ret = dbc_registered_func->run(dbc);
+		if (ret) {
+			xhci_dbc_mem_cleanup(xhci);
+			dbc->state = DS_DISABLED;
+			goto err;
+		}
 		xhci_dbc_start(xhci);
-	else if (!strncmp(buf, "disable", 7))
+	} else if (!strncmp(buf, "disable", 7) && dbc->state != DS_DISABLED) {
+		if (!dbc_registered_func)
+			return -EINVAL;
 		xhci_dbc_stop(xhci);
-	else
+		if (dbc_registered_func->stop)
+			dbc_registered_func->stop(dbc);
+		module_put(dbc_registered_func->owner);
+	} else
 		return -EINVAL;
 
 	return count;
+err:
+	module_put(dbc_registered_func->owner);
+	return ret;
 }
 
 static DEVICE_ATTR_RW(dbc);
 
+static struct attribute *dbc_dev_attributes[] = {
+	&dev_attr_dbc.attr,
+	NULL
+};
+
+static const struct attribute_group dbc_dev_attrib_grp = {
+	.attrs = dbc_dev_attributes,
+};
+
+
 int xhci_dbc_init(struct xhci_hcd *xhci)
 {
 	int			ret;
 	struct device		*dev = xhci_to_hcd(xhci)->self.controller;
 
 	ret = xhci_do_dbc_init(xhci);
-	if (ret)
-		goto init_err3;
-
-	ret = xhci_dbc_tty_register_driver(xhci);
 	if (ret)
 		goto init_err2;
 
-	ret = device_create_file(dev, &dev_attr_dbc);
+	ret = sysfs_create_group(&dev->kobj, &dbc_dev_attrib_grp);
 	if (ret)
 		goto init_err1;
 
 	return 0;
 
 init_err1:
-	xhci_dbc_tty_unregister_driver();
-init_err2:
 	xhci_do_dbc_exit(xhci);
-init_err3:
+init_err2:
 	return ret;
 }
 
@@ -963,11 +995,38 @@ void xhci_dbc_exit(struct xhci_hcd *xhci)
 		return;
 
 	device_remove_file(dev, &dev_attr_dbc);
-	xhci_dbc_tty_unregister_driver();
 	xhci_dbc_stop(xhci);
 	xhci_do_dbc_exit(xhci);
 }
 
+static inline int is_invalid(int val)
+{
+	return (val <  0 || val > 0xffff);
+}
+
+int xhci_dbc_register_function(struct dbc_function *func)
+{
+	if (dbc_registered_func)
+		return -EBUSY;
+
+	if (is_invalid(func->protocol) ||
+		is_invalid(func->vid) ||
+		is_invalid(func->pid) ||
+		is_invalid(func->device_rev))
+		return -EINVAL;
+	if (!func->run || !func->stop)
+		return -EINVAL;
+	dbc_registered_func = func;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_dbc_register_function);
+
+void xhci_dbc_unregister_function(void)
+{
+	dbc_registered_func = NULL;
+}
+EXPORT_SYMBOL_GPL(xhci_dbc_unregister_function);
+
 #ifdef CONFIG_PM
 int xhci_dbc_suspend(struct xhci_hcd *xhci)
 {
diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
index ce0c6072bd48..b4d5622a9030 100644
--- a/drivers/usb/host/xhci-dbgcap.h
+++ b/drivers/usb/host/xhci-dbgcap.h
@@ -11,6 +11,7 @@
 
 #include <linux/tty.h>
 #include <linux/kfifo.h>
+#include <linux/kernel.h>
 
 struct dbc_regs {
 	__le32	capability;
@@ -48,9 +49,9 @@ struct dbc_info_context {
 
 #define DBC_MAX_PACKET			1024
 #define DBC_MAX_STRING_LENGTH		64
-#define DBC_STRING_MANUFACTURER		"Linux Foundation"
-#define DBC_STRING_PRODUCT		"Linux USB Debug Target"
-#define DBC_STRING_SERIAL		"0001"
+#define DBC_STR_MANUFACTURER		"Linux Foundation"
+#define DBC_STR_PRODUCT		"Linux USB Debug Target"
+#define DBC_STR_SERIAL			"0001"
 #define	DBC_CONTEXT_SIZE		64
 
 /*
@@ -75,6 +76,7 @@ struct dbc_str_descs {
 #define DBC_PRODUCT_ID			0x0010	/* device 0010 */
 #define DBC_DEVICE_REV			0x0010	/* 0.10 */
 
+
 enum dbc_state {
 	DS_DISABLED = 0,
 	DS_INITIALIZED,
@@ -108,6 +110,25 @@ struct dbc_ep {
 	unsigned			direction:1;
 };
 
+struct dbc_function {
+	char				func_name[32];
+	/* string descriptors */
+	struct dbc_str_descs            string;
+	/* other device or interface descriptors */
+	u16				protocol;
+	u16				vid;
+	u16				pid;
+	u16				device_rev;
+
+	/* callbacks */
+	int (*run)(struct xhci_dbc *dbc);
+	int (*post_config)(struct xhci_dbc *dbc);
+	int (*post_disconnect)(struct xhci_dbc *dbc);
+	int (*stop)(struct xhci_dbc *dbc);
+
+	/* module owner */
+	struct module			*owner;
+};
 #define DBC_QUEUE_SIZE			16
 #define DBC_WRITE_BUF_SIZE		8192
 
@@ -151,6 +172,8 @@ struct xhci_dbc {
 	struct dbc_ep			eps[2];
 
 	struct dbc_port			port;
+	/* priv pointer for a function */
+	void                            *func_priv;
 };
 
 #define dbc_bulkout_ctx(d)		\
@@ -200,8 +223,11 @@ void xhci_dbc_tty_unregister_driver(void);
 int xhci_dbc_tty_register_device(struct xhci_hcd *xhci);
 void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci);
 struct dbc_request *dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags);
+void xhci_dbc_flush_reqests(struct xhci_dbc *dbc);
 void dbc_free_request(struct dbc_ep *dep, struct dbc_request *req);
 int dbc_ep_queue(struct dbc_ep *dep, struct dbc_request *req, gfp_t gfp_flags);
+int xhci_dbc_register_function(struct dbc_function *func);
+void xhci_dbc_unregister_function(void);
 #ifdef CONFIG_PM
 int xhci_dbc_suspend(struct xhci_hcd *xhci);
 int xhci_dbc_resume(struct xhci_hcd *xhci);
-- 
2.21.0

Re: [PATCH 1/5] usb: xhci: dbc: make DbC modular, introducing dbc_function structure

[Greg KH]

On Fri, Jun 07, 2019 at 12:03:02PM +0530, Prabhat Chand Pandey wrote:
> This patch introduces the dbc_function structure as a first step in
> making DbC modular and capable in exposing different user interfaces using
> different "functions", which may implement the callbacks exposed here
> according to the driver's need.
> 
> Only one "function" can be registered at a time.
> The generic way to enable a DbC function would be, using sys-fs:

"sysfs"

> 
> Locate the directory for PCI enumerated XHCI host controller in the
> sysfs path.
> e.g.: cd /sys/bus/pci/devices/0000:00:14.0
> $ echo "enable" > dbc

You are adding a new sysfs file to the pci device?  why?

> This is done AFTER the function is selected at build time. Only 1 function
> can be selected at a time.

Why?

And "dbc" is debug port, right?  Or is it something else?

> Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-dbgcap.c | 159 ++++++++++++++++++++++-----------
>  drivers/usb/host/xhci-dbgcap.h |  32 ++++++-
>  2 files changed, 138 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 52e32644a4b2..96adc53b0fdb 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -9,11 +9,14 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
>  #include <linux/nls.h>
> +#include <linux/module.h>
>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
>  #include "xhci-dbgcap.h"
>  
> +static struct dbc_function *dbc_registered_func;
> +
>  static inline void *
>  dbc_dma_alloc_coherent(struct xhci_hcd *xhci, size_t size,
>  		       dma_addr_t *dma_handle, gfp_t flags)
> @@ -35,41 +38,42 @@ dbc_dma_free_coherent(struct xhci_hcd *xhci, size_t size,
>  				  size, cpu_addr, dma_handle);
>  }
>  
> -static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings)
> +static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings,
> +					struct dbc_function *func)
>  {
>  	struct usb_string_descriptor	*s_desc;
>  	u32				string_length;
>  
>  	/* Serial string: */
>  	s_desc = (struct usb_string_descriptor *)strings->serial;
> -	utf8s_to_utf16s(DBC_STRING_SERIAL, strlen(DBC_STRING_SERIAL),
> +	utf8s_to_utf16s(func->string.serial, strlen(func->string.serial),
>  			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
>  			DBC_MAX_STRING_LENGTH);
>  
> -	s_desc->bLength		= (strlen(DBC_STRING_SERIAL) + 1) * 2;
> +	s_desc->bLength		= (strlen(func->string.serial) + 1) * 2;
>  	s_desc->bDescriptorType	= USB_DT_STRING;
>  	string_length		= s_desc->bLength;
>  	string_length		<<= 8;
>  
>  	/* Product string: */
>  	s_desc = (struct usb_string_descriptor *)strings->product;
> -	utf8s_to_utf16s(DBC_STRING_PRODUCT, strlen(DBC_STRING_PRODUCT),
> +	utf8s_to_utf16s(func->string.product, strlen(func->string.product),
>  			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
>  			DBC_MAX_STRING_LENGTH);
>  
> -	s_desc->bLength		= (strlen(DBC_STRING_PRODUCT) + 1) * 2;
> +	s_desc->bLength		= (strlen(func->string.product) + 1) * 2;
>  	s_desc->bDescriptorType	= USB_DT_STRING;
>  	string_length		+= s_desc->bLength;
>  	string_length		<<= 8;
>  
>  	/* Manufacture string: */
>  	s_desc = (struct usb_string_descriptor *)strings->manufacturer;
> -	utf8s_to_utf16s(DBC_STRING_MANUFACTURER,
> -			strlen(DBC_STRING_MANUFACTURER),
> +	utf8s_to_utf16s(func->string.manufacturer,
> +			strlen(func->string.manufacturer),
>  			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
>  			DBC_MAX_STRING_LENGTH);
>  
> -	s_desc->bLength		= (strlen(DBC_STRING_MANUFACTURER) + 1) * 2;
> +	s_desc->bLength		= (strlen(func->string.manufacturer) + 1) * 2;
>  	s_desc->bDescriptorType	= USB_DT_STRING;
>  	string_length		+= s_desc->bLength;
>  	string_length		<<= 8;
> @@ -84,7 +88,9 @@ static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings)
>  	return string_length;
>  }
>  
> -static void xhci_dbc_init_contexts(struct xhci_hcd *xhci, u32 string_length)
> +static void xhci_dbc_init_contexts(struct xhci_hcd *xhci,
> +					struct dbc_function *func,
> +					u32 string_length)
>  {
>  	struct xhci_dbc		*dbc;
>  	struct dbc_info_context	*info;
> @@ -124,10 +130,10 @@ static void xhci_dbc_init_contexts(struct xhci_hcd *xhci, u32 string_length)
>  	/* Set DbC context and info registers: */
>  	xhci_write_64(xhci, dbc->ctx->dma, &dbc->regs->dccp);
>  
> -	dev_info = cpu_to_le32((DBC_VENDOR_ID << 16) | DBC_PROTOCOL);
> +	dev_info = cpu_to_le32((func->vid << 16) | func->protocol);
>  	writel(dev_info, &dbc->regs->devinfo1);
>  
> -	dev_info = cpu_to_le32((DBC_DEVICE_REV << 16) | DBC_PRODUCT_ID);
> +	dev_info = cpu_to_le32((func->device_rev << 16) | func->pid);
>  	writel(dev_info, &dbc->regs->devinfo2);
>  }
>  
> @@ -181,11 +187,13 @@ static void xhci_dbc_flush_endpoint_requests(struct dbc_ep *dep)
>  		xhci_dbc_flush_single_request(req);
>  }
>  
> -static void xhci_dbc_flush_requests(struct xhci_dbc *dbc)
> +
> +void xhci_dbc_flush_requests(struct xhci_dbc *dbc)
>  {
>  	xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_OUT]);
>  	xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_IN]);
>  }
> +EXPORT_SYMBOL_GPL(xhci_dbc_flush_requests);
>  
>  struct dbc_request *
>  dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags)
> @@ -205,6 +213,7 @@ dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags)
>  
>  	return req;
>  }
> +EXPORT_SYMBOL_GPL(dbc_alloc_request);
>  
>  void
>  dbc_free_request(struct dbc_ep *dep, struct dbc_request *req)
> @@ -213,6 +222,7 @@ dbc_free_request(struct dbc_ep *dep, struct dbc_request *req)
>  
>  	kfree(req);
>  }
> +EXPORT_SYMBOL_GPL(dbc_free_request);
>  
>  static void
>  xhci_dbc_queue_trb(struct xhci_ring *ring, u32 field1,
> @@ -344,6 +354,7 @@ int dbc_ep_queue(struct dbc_ep *dep, struct dbc_request *req,
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(dbc_ep_queue);
>  
>  static inline void xhci_dbc_do_eps_init(struct xhci_hcd *xhci, bool direction)
>  {
> @@ -371,7 +382,9 @@ static void xhci_dbc_eps_exit(struct xhci_hcd *xhci)
>  	memset(dbc->eps, 0, sizeof(struct dbc_ep) * ARRAY_SIZE(dbc->eps));
>  }
>  
> -static int xhci_dbc_mem_init(struct xhci_hcd *xhci, gfp_t flags)
> +static int xhci_dbc_mem_init(struct xhci_hcd *xhci,
> +				struct dbc_function *func,
> +				gfp_t flags)
>  {
>  	int			ret;
>  	dma_addr_t		deq;
> @@ -418,8 +431,8 @@ static int xhci_dbc_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>  	xhci_write_64(xhci, deq, &dbc->regs->erdp);
>  
>  	/* Setup strings and contexts: */
> -	string_length = xhci_dbc_populate_strings(dbc->string);
> -	xhci_dbc_init_contexts(xhci, string_length);
> +	string_length = xhci_dbc_populate_strings(dbc->string, func);
> +	xhci_dbc_init_contexts(xhci, func, string_length);
>  
>  	xhci_dbc_eps_init(xhci);
>  	dbc->state = DS_INITIALIZED;
> @@ -478,20 +491,9 @@ static int xhci_do_dbc_start(struct xhci_hcd *xhci)
>  	u32			ctrl;
>  	struct xhci_dbc		*dbc = xhci->dbc;
>  
> -	if (dbc->state != DS_DISABLED)
> +	if (dbc->state != DS_INITIALIZED)
>  		return -EINVAL;
>  
> -	writel(0, &dbc->regs->control);
> -	ret = xhci_handshake(&dbc->regs->control,
> -			     DBC_CTRL_DBC_ENABLE,
> -			     0, 1000);
> -	if (ret)
> -		return ret;
> -
> -	ret = xhci_dbc_mem_init(xhci, GFP_ATOMIC);
> -	if (ret)
> -		return ret;
> -
>  	ctrl = readl(&dbc->regs->control);
>  	writel(ctrl | DBC_CTRL_DBC_ENABLE | DBC_CTRL_PORT_ENABLE,
>  	       &dbc->regs->control);
> @@ -552,9 +554,13 @@ static void xhci_dbc_stop(struct xhci_hcd *xhci)
>  
>  	cancel_delayed_work_sync(&dbc->event_work);
>  
> -	if (port->registered)
> -		xhci_dbc_tty_unregister_device(xhci);
> -
> +	if (port->registered &&
> +	    dbc_registered_func &&
> +	    dbc_registered_func->post_disconnect) {
> +		ret = dbc_registered_func->post_disconnect(dbc);
> +		if (ret)
> +			xhci_err(xhci, "dbc post_disconnect error %d\n", ret);
> +	}
>  	spin_lock_irqsave(&dbc->lock, flags);
>  	ret = xhci_do_dbc_stop(xhci);
>  	spin_unlock_irqrestore(&dbc->lock, flags);
> @@ -798,16 +804,12 @@ static void xhci_dbc_handle_events(struct work_struct *work)
>  
>  	switch (evtr) {
>  	case EVT_GSER:
> -		ret = xhci_dbc_tty_register_device(xhci);
> -		if (ret) {
> -			xhci_err(xhci, "failed to alloc tty device\n");
> -			break;
> -		}
> -
> -		xhci_info(xhci, "DbC now attached to /dev/ttyDBC0\n");
> +		if (dbc_registered_func->post_config)
> +			dbc_registered_func->post_config(dbc);
>  		break;
>  	case EVT_DISC:
> -		xhci_dbc_tty_unregister_device(xhci);
> +		if (dbc_registered_func->post_disconnect)
> +			ret = dbc_registered_func->post_disconnect(dbc);
>  		break;
>  	case EVT_DONE:
>  		break;
> @@ -912,46 +914,76 @@ static ssize_t dbc_store(struct device *dev,
>  			 struct device_attribute *attr,
>  			 const char *buf, size_t count)
>  {
> +	struct xhci_dbc		*dbc;
>  	struct xhci_hcd		*xhci;
> +	int			ret = 0;
>  
>  	xhci = hcd_to_xhci(dev_get_drvdata(dev));
> +	dbc = xhci->dbc;
>  
> -	if (!strncmp(buf, "enable", 6))
> +	if (!strncmp(buf, "enable", 6) && dbc->state == DS_DISABLED) {
> +		if (!dbc_registered_func)
> +			return -EINVAL;
> +		if (!try_module_get(dbc_registered_func->owner))
> +			return -ENODEV;
> +		ret = xhci_dbc_mem_init(dbc->xhci, dbc_registered_func,
> +						GFP_ATOMIC);
> +		if (ret)
> +			goto err;
> +		if (dbc_registered_func->run)
> +			ret = dbc_registered_func->run(dbc);
> +		if (ret) {
> +			xhci_dbc_mem_cleanup(xhci);
> +			dbc->state = DS_DISABLED;
> +			goto err;
> +		}
>  		xhci_dbc_start(xhci);
> -	else if (!strncmp(buf, "disable", 7))
> +	} else if (!strncmp(buf, "disable", 7) && dbc->state != DS_DISABLED) {
> +		if (!dbc_registered_func)
> +			return -EINVAL;
>  		xhci_dbc_stop(xhci);
> -	else
> +		if (dbc_registered_func->stop)
> +			dbc_registered_func->stop(dbc);
> +		module_put(dbc_registered_func->owner);
> +	} else
>  		return -EINVAL;
>  
>  	return count;
> +err:
> +	module_put(dbc_registered_func->owner);
> +	return ret;
>  }
>  
>  static DEVICE_ATTR_RW(dbc);
>  
> +static struct attribute *dbc_dev_attributes[] = {
> +	&dev_attr_dbc.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dbc_dev_attrib_grp = {
> +	.attrs = dbc_dev_attributes,
> +};
> +
> +
>  int xhci_dbc_init(struct xhci_hcd *xhci)
>  {
>  	int			ret;
>  	struct device		*dev = xhci_to_hcd(xhci)->self.controller;
>  
>  	ret = xhci_do_dbc_init(xhci);
> -	if (ret)
> -		goto init_err3;
> -
> -	ret = xhci_dbc_tty_register_driver(xhci);
>  	if (ret)
>  		goto init_err2;
>  
> -	ret = device_create_file(dev, &dev_attr_dbc);
> +	ret = sysfs_create_group(&dev->kobj, &dbc_dev_attrib_grp);
>  	if (ret)
>  		goto init_err1;
>  
>  	return 0;
>  
>  init_err1:
> -	xhci_dbc_tty_unregister_driver();
> -init_err2:
>  	xhci_do_dbc_exit(xhci);
> -init_err3:
> +init_err2:
>  	return ret;
>  }
>  
> @@ -963,11 +995,38 @@ void xhci_dbc_exit(struct xhci_hcd *xhci)
>  		return;
>  
>  	device_remove_file(dev, &dev_attr_dbc);
> -	xhci_dbc_tty_unregister_driver();
>  	xhci_dbc_stop(xhci);
>  	xhci_do_dbc_exit(xhci);
>  }
>  
> +static inline int is_invalid(int val)
> +{
> +	return (val <  0 || val > 0xffff);
> +}
> +
> +int xhci_dbc_register_function(struct dbc_function *func)
> +{
> +	if (dbc_registered_func)
> +		return -EBUSY;
> +
> +	if (is_invalid(func->protocol) ||
> +		is_invalid(func->vid) ||
> +		is_invalid(func->pid) ||
> +		is_invalid(func->device_rev))
> +		return -EINVAL;
> +	if (!func->run || !func->stop)
> +		return -EINVAL;
> +	dbc_registered_func = func;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xhci_dbc_register_function);
> +
> +void xhci_dbc_unregister_function(void)
> +{
> +	dbc_registered_func = NULL;
> +}
> +EXPORT_SYMBOL_GPL(xhci_dbc_unregister_function);
> +
>  #ifdef CONFIG_PM
>  int xhci_dbc_suspend(struct xhci_hcd *xhci)
>  {
> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
> index ce0c6072bd48..b4d5622a9030 100644
> --- a/drivers/usb/host/xhci-dbgcap.h
> +++ b/drivers/usb/host/xhci-dbgcap.h
> @@ -11,6 +11,7 @@
>  
>  #include <linux/tty.h>
>  #include <linux/kfifo.h>
> +#include <linux/kernel.h>
>  
>  struct dbc_regs {
>  	__le32	capability;
> @@ -48,9 +49,9 @@ struct dbc_info_context {
>  
>  #define DBC_MAX_PACKET			1024
>  #define DBC_MAX_STRING_LENGTH		64
> -#define DBC_STRING_MANUFACTURER		"Linux Foundation"
> -#define DBC_STRING_PRODUCT		"Linux USB Debug Target"
> -#define DBC_STRING_SERIAL		"0001"
> +#define DBC_STR_MANUFACTURER		"Linux Foundation"
> +#define DBC_STR_PRODUCT		"Linux USB Debug Target"
> +#define DBC_STR_SERIAL			"0001"
>  #define	DBC_CONTEXT_SIZE		64
>  
>  /*
> @@ -75,6 +76,7 @@ struct dbc_str_descs {
>  #define DBC_PRODUCT_ID			0x0010	/* device 0010 */
>  #define DBC_DEVICE_REV			0x0010	/* 0.10 */
>  
> +
>  enum dbc_state {
>  	DS_DISABLED = 0,
>  	DS_INITIALIZED,
> @@ -108,6 +110,25 @@ struct dbc_ep {
>  	unsigned			direction:1;
>  };
>  
> +struct dbc_function {
> +	char				func_name[32];
> +	/* string descriptors */
> +	struct dbc_str_descs            string;
> +	/* other device or interface descriptors */
> +	u16				protocol;
> +	u16				vid;
> +	u16				pid;
> +	u16				device_rev;

le16 for all of these?

> +	/* callbacks */
> +	int (*run)(struct xhci_dbc *dbc);
> +	int (*post_config)(struct xhci_dbc *dbc);
> +	int (*post_disconnect)(struct xhci_dbc *dbc);
> +	int (*stop)(struct xhci_dbc *dbc);
> +
> +	/* module owner */
> +	struct module			*owner;
> +};

Why would a function have a module owner?

What exactly is a "function"?  is it a driver?  If so, why not make it a
real driver, have a real bus, and bind drivers to the devices on the
bus?

thanks,

greg k-h

[PATCH 2/5] usb: xhci: dbc: DbC TTY driver to use new interface

[Prabhat Chand Pandey]

Change DbC TTY driver to use the new modular interface exposed by the DbC
core. This will allow other function drivers with a different interface
also to use DbC.

[no need to add running number to tty driver name, remove it. -Mathias]
Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/Kconfig       | 15 ++++++-
 drivers/usb/host/Makefile      |  4 +-
 drivers/usb/host/xhci-dbgcap.h |  4 --
 drivers/usb/host/xhci-dbgtty.c | 81 ++++++++++++++++++++++++++++++----
 4 files changed, 90 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index d809671c5fea..c29ed8a61dcb 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -30,13 +30,26 @@ config USB_XHCI_HCD
 if USB_XHCI_HCD
 config USB_XHCI_DBGCAP
 	bool "xHCI support for debug capability"
-	depends on TTY
 	---help---
 	  Say 'Y' to enable the support for the xHCI debug capability. Make
 	  sure that your xHCI host supports the extended debug capability and
 	  you want a TTY serial device based on the xHCI debug capability
 	  before enabling this option. If unsure, say 'N'.
 
+choice
+	prompt "Select function for debug capability"
+	depends on USB_XHCI_DBGCAP
+
+config USB_XHCI_DBGCAP_TTY
+	tristate "xHCI DbC tty driver support"
+	depends on USB_XHCI_HCD && USB_XHCI_DBGCAP && TTY
+	help
+	  Say 'Y' to enable the support for the tty driver interface to xHCI
+	  debug capability. This will expose a /dev/ttyDBC* device node on device
+	  which may be used by the usb-debug driver on the debug host.
+	  If unsure, say 'N'.
+endchoice
+
 config USB_XHCI_PCI
        tristate
        depends on USB_PCI
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 84514f71ae44..b21b0ea9e966 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -16,9 +16,11 @@ xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
 xhci-hcd-y += xhci-trace.o
 
 ifneq ($(CONFIG_USB_XHCI_DBGCAP), )
-	xhci-hcd-y += xhci-dbgcap.o xhci-dbgtty.o
+	xhci-hcd-y += xhci-dbgcap.o
 endif
 
+obj-$(CONFIG_USB_XHCI_DBGCAP_TTY) += xhci-dbgtty.o
+
 ifneq ($(CONFIG_USB_XHCI_MTK), )
 	xhci-hcd-y += xhci-mtk-sch.o
 endif
diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
index b4d5622a9030..302e6ca72370 100644
--- a/drivers/usb/host/xhci-dbgcap.h
+++ b/drivers/usb/host/xhci-dbgcap.h
@@ -218,10 +218,6 @@ static inline struct dbc_ep *get_out_ep(struct xhci_hcd *xhci)
 #ifdef CONFIG_USB_XHCI_DBGCAP
 int xhci_dbc_init(struct xhci_hcd *xhci);
 void xhci_dbc_exit(struct xhci_hcd *xhci);
-int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci);
-void xhci_dbc_tty_unregister_driver(void);
-int xhci_dbc_tty_register_device(struct xhci_hcd *xhci);
-void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci);
 struct dbc_request *dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags);
 void xhci_dbc_flush_reqests(struct xhci_dbc *dbc);
 void dbc_free_request(struct dbc_ep *dep, struct dbc_request *req);
diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index aff79ff5aba4..f75a95006c51 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -7,13 +7,15 @@
  * Author: Lu Baolu <baolu.lu@linux.intel.com>
  */
 
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
-
 #include "xhci.h"
 #include "xhci-dbgcap.h"
 
+#define DBC_STR_FUNC_TTY    "TTY"
+
 static unsigned int
 dbc_send_packet(struct dbc_port *port, char *packet, unsigned int size)
 {
@@ -279,12 +281,11 @@ static const struct tty_operations dbc_tty_ops = {
 	.unthrottle		= dbc_tty_unthrottle,
 };
 
-static struct tty_driver *dbc_tty_driver;
-
-int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
+static int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
 {
 	int			status;
 	struct xhci_dbc		*dbc = xhci->dbc;
+	struct tty_driver	*dbc_tty_driver;
 
 	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
 					  TTY_DRIVER_DYNAMIC_DEV);
@@ -296,7 +297,6 @@ int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
 
 	dbc_tty_driver->driver_name = "dbc_serial";
 	dbc_tty_driver->name = "ttyDBC";
-
 	dbc_tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
 	dbc_tty_driver->subtype = SERIAL_TYPE_NORMAL;
 	dbc_tty_driver->init_termios = tty_std_termios;
@@ -315,16 +315,19 @@ int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
 		put_tty_driver(dbc_tty_driver);
 		dbc_tty_driver = NULL;
 	}
+	dbc->func_priv = dbc_tty_driver;
 
 	return status;
 }
 
-void xhci_dbc_tty_unregister_driver(void)
+static void xhci_dbc_tty_unregister_driver(struct xhci_dbc *dbc)
 {
+	struct tty_driver	*dbc_tty_driver =
+					(struct tty_driver *) dbc->func_priv;
 	if (dbc_tty_driver) {
 		tty_unregister_driver(dbc_tty_driver);
 		put_tty_driver(dbc_tty_driver);
-		dbc_tty_driver = NULL;
+		dbc->func_priv = NULL;
 	}
 }
 
@@ -440,12 +443,14 @@ xhci_dbc_tty_exit_port(struct dbc_port *port)
 	tty_port_destroy(&port->port);
 }
 
-int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
+static int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
 {
 	int			ret;
 	struct device		*tty_dev;
 	struct xhci_dbc		*dbc = xhci->dbc;
 	struct dbc_port		*port = &dbc->port;
+	struct tty_driver	*dbc_tty_driver =
+					(struct tty_driver *) dbc->func_priv;
 
 	xhci_dbc_tty_init_port(xhci, port);
 	tty_dev = tty_port_register_device(&port->port,
@@ -493,6 +498,8 @@ void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci)
 {
 	struct xhci_dbc		*dbc = xhci->dbc;
 	struct dbc_port		*port = &dbc->port;
+	struct tty_driver	*dbc_tty_driver =
+					(struct tty_driver *) dbc->func_priv;
 
 	tty_unregister_device(dbc_tty_driver, 0);
 	xhci_dbc_tty_exit_port(port);
@@ -503,3 +510,61 @@ void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci)
 	xhci_dbc_free_requests(get_out_ep(xhci), &port->read_queue);
 	xhci_dbc_free_requests(get_in_ep(xhci), &port->write_pool);
 }
+
+static int dbc_tty_post_config(struct xhci_dbc *dbc)
+{
+	return xhci_dbc_tty_register_device(dbc->xhci);
+}
+
+static int dbc_tty_post_disconnect(struct xhci_dbc *dbc)
+{
+	xhci_dbc_tty_unregister_device(dbc->xhci);
+	return 0;
+}
+
+static int dbc_tty_run(struct xhci_dbc *dbc)
+{
+	return  xhci_dbc_tty_register_driver(dbc->xhci);
+}
+
+static int dbc_tty_stop(struct xhci_dbc *dbc)
+{
+	xhci_dbc_tty_unregister_driver(dbc);
+	return 0;
+}
+
+static struct dbc_function tty_func = {
+	.owner = THIS_MODULE,
+	.string = {
+		.manufacturer = DBC_STR_MANUFACTURER,
+		.product = DBC_STR_PRODUCT,
+		.serial = DBC_STR_SERIAL,
+	},
+	.protocol = DBC_PROTOCOL,
+	.vid =     DBC_VENDOR_ID,
+	.pid =     DBC_PRODUCT_ID,
+	.device_rev = DBC_DEVICE_REV,
+	.func_name = DBC_STR_FUNC_TTY,
+
+	.post_config = dbc_tty_post_config,
+	.post_disconnect = dbc_tty_post_disconnect,
+	.run = dbc_tty_run,
+	.stop = dbc_tty_stop,
+};
+
+static int __init xhci_dbc_tty_init(void)
+{
+	return xhci_dbc_register_function(&tty_func);
+}
+
+static void __exit xhci_dbc_tty_fini(void)
+{
+	xhci_dbc_unregister_function();
+}
+
+late_initcall(xhci_dbc_tty_init);
+module_exit(xhci_dbc_tty_fini);
+
+MODULE_DESCRIPTION("xHCI DbC tty driver");
+MODULE_AUTHOR("Baoulu Lu");
+MODULE_LICENSE("GPL");
-- 
2.21.0

Re: [PATCH 2/5] usb: xhci: dbc: DbC TTY driver to use new interface

[Greg KH]

On Fri, Jun 07, 2019 at 12:03:03PM +0530, Prabhat Chand Pandey wrote:
> Change DbC TTY driver to use the new modular interface exposed by the DbC
> core. This will allow other function drivers with a different interface
> also to use DbC.

What are those other function drivers?  What is wrong with tty?

I'm missing a _lot_ of background information here...

> [no need to add running number to tty driver name, remove it. -Mathias]
> Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/Kconfig       | 15 ++++++-
>  drivers/usb/host/Makefile      |  4 +-
>  drivers/usb/host/xhci-dbgcap.h |  4 --
>  drivers/usb/host/xhci-dbgtty.c | 81 ++++++++++++++++++++++++++++++----
>  4 files changed, 90 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index d809671c5fea..c29ed8a61dcb 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -30,13 +30,26 @@ config USB_XHCI_HCD
>  if USB_XHCI_HCD
>  config USB_XHCI_DBGCAP
>  	bool "xHCI support for debug capability"
> -	depends on TTY
>  	---help---
>  	  Say 'Y' to enable the support for the xHCI debug capability. Make
>  	  sure that your xHCI host supports the extended debug capability and
>  	  you want a TTY serial device based on the xHCI debug capability
>  	  before enabling this option. If unsure, say 'N'.
>  
> +choice
> +	prompt "Select function for debug capability"
> +	depends on USB_XHCI_DBGCAP
> +
> +config USB_XHCI_DBGCAP_TTY
> +	tristate "xHCI DbC tty driver support"
> +	depends on USB_XHCI_HCD && USB_XHCI_DBGCAP && TTY
> +	help
> +	  Say 'Y' to enable the support for the tty driver interface to xHCI
> +	  debug capability. This will expose a /dev/ttyDBC* device node on device
> +	  which may be used by the usb-debug driver on the debug host.
> +	  If unsure, say 'N'.

Module name?

> +endchoice

So you have to choose at build time the "function"?  Why?  I thougth
this was to be dynamic?

> +
>  config USB_XHCI_PCI
>         tristate
>         depends on USB_PCI
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index 84514f71ae44..b21b0ea9e966 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -16,9 +16,11 @@ xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
>  xhci-hcd-y += xhci-trace.o
>  
>  ifneq ($(CONFIG_USB_XHCI_DBGCAP), )
> -	xhci-hcd-y += xhci-dbgcap.o xhci-dbgtty.o
> +	xhci-hcd-y += xhci-dbgcap.o
>  endif
>  
> +obj-$(CONFIG_USB_XHCI_DBGCAP_TTY) += xhci-dbgtty.o
> +
>  ifneq ($(CONFIG_USB_XHCI_MTK), )
>  	xhci-hcd-y += xhci-mtk-sch.o
>  endif
> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
> index b4d5622a9030..302e6ca72370 100644
> --- a/drivers/usb/host/xhci-dbgcap.h
> +++ b/drivers/usb/host/xhci-dbgcap.h
> @@ -218,10 +218,6 @@ static inline struct dbc_ep *get_out_ep(struct xhci_hcd *xhci)
>  #ifdef CONFIG_USB_XHCI_DBGCAP
>  int xhci_dbc_init(struct xhci_hcd *xhci);
>  void xhci_dbc_exit(struct xhci_hcd *xhci);
> -int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci);
> -void xhci_dbc_tty_unregister_driver(void);
> -int xhci_dbc_tty_register_device(struct xhci_hcd *xhci);
> -void xhci_dbc_tty_unregister_device(struct xhci_hcd *xhci);
>  struct dbc_request *dbc_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags);
>  void xhci_dbc_flush_reqests(struct xhci_dbc *dbc);
>  void dbc_free_request(struct dbc_ep *dep, struct dbc_request *req);
> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> index aff79ff5aba4..f75a95006c51 100644
> --- a/drivers/usb/host/xhci-dbgtty.c
> +++ b/drivers/usb/host/xhci-dbgtty.c
> @@ -7,13 +7,15 @@
>   * Author: Lu Baolu <baolu.lu@linux.intel.com>
>   */
>  
> +#include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> -
>  #include "xhci.h"
>  #include "xhci-dbgcap.h"
>  
> +#define DBC_STR_FUNC_TTY    "TTY"

What is this for?  You only use it once, what does it mean?

> +
>  static unsigned int
>  dbc_send_packet(struct dbc_port *port, char *packet, unsigned int size)
>  {
> @@ -279,12 +281,11 @@ static const struct tty_operations dbc_tty_ops = {
>  	.unthrottle		= dbc_tty_unthrottle,
>  };
>  
> -static struct tty_driver *dbc_tty_driver;
> -
> -int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
> +static int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
>  {
>  	int			status;
>  	struct xhci_dbc		*dbc = xhci->dbc;
> +	struct tty_driver	*dbc_tty_driver;
>  
>  	dbc_tty_driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
>  					  TTY_DRIVER_DYNAMIC_DEV);
> @@ -296,7 +297,6 @@ int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
>  
>  	dbc_tty_driver->driver_name = "dbc_serial";
>  	dbc_tty_driver->name = "ttyDBC";
> -
>  	dbc_tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
>  	dbc_tty_driver->subtype = SERIAL_TYPE_NORMAL;
>  	dbc_tty_driver->init_termios = tty_std_termios;

Unneeded change.

> @@ -315,16 +315,19 @@ int xhci_dbc_tty_register_driver(struct xhci_hcd *xhci)
>  		put_tty_driver(dbc_tty_driver);
>  		dbc_tty_driver = NULL;
>  	}
> +	dbc->func_priv = dbc_tty_driver;
>  
>  	return status;
>  }
>  
> -void xhci_dbc_tty_unregister_driver(void)
> +static void xhci_dbc_tty_unregister_driver(struct xhci_dbc *dbc)
>  {
> +	struct tty_driver	*dbc_tty_driver =
> +					(struct tty_driver *) dbc->func_priv;

Horrid formatting.  Checkpatch would have warned you about this mess, so
it's obvious you didn't run it :(

>  	if (dbc_tty_driver) {
>  		tty_unregister_driver(dbc_tty_driver);
>  		put_tty_driver(dbc_tty_driver);
> -		dbc_tty_driver = NULL;
> +		dbc->func_priv = NULL;
>  	}
>  }
>  
> @@ -440,12 +443,14 @@ xhci_dbc_tty_exit_port(struct dbc_port *port)
>  	tty_port_destroy(&port->port);
>  }
>  
> -int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
> +static int xhci_dbc_tty_register_device(struct xhci_hcd *xhci)
>  {
>  	int			ret;
>  	struct device		*tty_dev;
>  	struct xhci_dbc		*dbc = xhci->dbc;
>  	struct dbc_port		*port = &dbc->port;
> +	struct tty_driver	*dbc_tty_driver =
> +					(struct tty_driver *) dbc->func_priv;

Again, ick.  Why are you casting?

And again, checkpatch.pl please.

I give up here, sorry, this series is a mess :(

greg k-h

[PATCH 3/5] usb: xhci: dbc: Provide sysfs option to configure dbc descriptors

[Prabhat Chand Pandey]

From: "K V, Abhilash" <abhilash.k.v@intel.com>

Show the active dbc function and dbc descriptors, allowing
user space to dynamically modify the descriptors

The DBC specific sysfs attributes are separated into two groups,
in the first group there are dbc & dbc_function sysfs attributes and in
second group all other DBC descriptor specific sysfs attributes.

First group of attributes will be populated at the time of dbc_init and
second group of attributes will only be populated when "dbc" attribute
value is set to "enable".

Whenever "dbc" attribute value will be "disable" then second group
of attributes will be removed.

Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
Signed-off-by: Gururaj K <gururaj.k@intel.com>
Signed-off-by: K V, Abhilash <abhilash.k.v@intel.com>
Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 112 ++++++
 drivers/usb/host/xhci-dbgcap.c                | 339 ++++++++++++++++++
 2 files changed, 451 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
index 0088aba4caa8..b46b6afc6c4a 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
+++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
@@ -23,3 +23,115 @@ Description:
 		Reading this attribute gives the state of the DbC. It
 		can be one of the following states: disabled, enabled,
 		initialized, connected, configured and stalled.
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_function
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+
+		This framework can be utilized to provide various interfaces.
+		By Default, it is configured to provide a serial Interface.
+
+		This attribute lets us configure the interface provided
+		by Dbc functionality. By Default, this attribute value
+		is "TTY" corresponding to the the serial interface. Other
+		values can be supported in future to provide a varied
+		interface to use DbC.
+
+		Reading this attribute gives the interface which is
+		currently configured with DbC. If it is "TTY" then serial
+		interface is configured.
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_manufacturer
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the manufacturer name as
+		presented by the debug device in the USB Manufacturer String
+		descriptor. The default value is "Linux Foundation".
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_product
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the product name as
+		presented by the debug device in the USB Product String
+		descriptor. The default value is "Linux USB Debug Target".
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_serial
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the serial number as
+		presented by the debug device in the USB Serial Number String
+		descriptor. The default value is "0001".
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_protocol
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the bInterfaceProtocol field as
+		presented by the debug device in the USB Interface descriptor.
+
+		The default value is  1  (GNU Remote Debug command).
+		Other permissible value is 0 which is for vendor defined debug
+		target.
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_vid
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the idVendor field as
+		presented by the debug device in the USB Device descriptor.
+		The default value is 0x1d6b (Linux Foundation).
+		It can be any 16-bit integer.
+
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_pid
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the idProduct field as
+		presented by the debug device in the USB Device descriptor.
+		The default value is 0x0010. It can be any 16-bit integer.
+
+What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_device_rev
+Date:		June 2018
+Contact:	Rajaram Regupathy <rajaram.regupathy@intel.com>
+Description:
+		xHCI USB host controllers provide Debug Capability (Dbc)
+		as an extended feature. When configured Dbc presents a
+		debug device which is fully compliant with the USB
+		framework.
+		This attribute lets us change the bcdDevice field as
+		presented by the debug device in the USB Device descriptor.
+		The default value is 0x0010 (device rev 0.10).
+		It can be any 16-bit integer.
diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 96adc53b0fdb..d6bd1eacc208 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -873,6 +873,315 @@ static int xhci_do_dbc_init(struct xhci_hcd *xhci)
 	return 0;
 }
 
+static ssize_t dbc_function_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	int    count = 0;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (!dbc)
+		return -ENODEV;
+	if (!dbc_registered_func)
+		count += sprintf(buf, "%s\n", "none");
+	else
+		count += sprintf(buf, "%s\n", dbc_registered_func->func_name);
+
+	return count;
+}
+
+static ssize_t dbc_manufacturer_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	struct usb_string_descriptor    *s_desc;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (!dbc || !dbc->string)
+		return -ENODEV;
+	s_desc = (struct usb_string_descriptor *) dbc->string->manufacturer;
+	return utf16s_to_utf8s((wchar_t *) s_desc->wData, s_desc->bLength / 2,
+			UTF16_LITTLE_ENDIAN, buf, DBC_MAX_STRING_LENGTH);
+}
+
+static ssize_t dbc_manufacturer_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	struct usb_string_descriptor    *s_desc;
+	int ret;
+	struct dbc_info_context	*info;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	s_desc = (struct usb_string_descriptor *) dbc->string->manufacturer;
+	ret =  utf8s_to_utf16s(buf, strlen(buf),
+				UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
+				DBC_MAX_STRING_LENGTH);
+	if (ret < 0)
+		return ret;
+	s_desc->bLength         = (strlen(buf) + 1) * 2;
+	info			= (struct dbc_info_context *)dbc->ctx->bytes;
+	info->length		= (info->length & ~(0xffu << 8))
+					|  (s_desc->bLength) << 8;
+	return size;
+}
+
+static ssize_t dbc_product_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	struct usb_string_descriptor    *s_desc;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (!dbc || !dbc->string)
+		return -ENODEV;
+	s_desc = (struct usb_string_descriptor *) dbc->string->product;
+	return utf16s_to_utf8s((wchar_t *) s_desc->wData, s_desc->bLength / 2,
+			UTF16_LITTLE_ENDIAN, buf, DBC_MAX_STRING_LENGTH);
+}
+
+static ssize_t dbc_product_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	struct usb_string_descriptor    *s_desc;
+	int ret;
+	struct dbc_info_context	*info;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	s_desc = (struct usb_string_descriptor *) dbc->string->serial;
+	ret = utf8s_to_utf16s(buf, strlen(buf),
+				UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
+				DBC_MAX_STRING_LENGTH);
+	if (ret < 0)
+		return ret;
+	s_desc->bLength         = (strlen(buf) + 1) * 2;
+	info			= (struct dbc_info_context *)dbc->ctx->bytes;
+	info->length		= (info->length & ~(0xffu << 16))
+					 |  (s_desc->bLength) << 16;
+	return size;
+}
+
+static ssize_t dbc_serial_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	struct usb_string_descriptor    *s_desc;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (!dbc || !dbc->string)
+		return -ENODEV;
+	s_desc = (struct usb_string_descriptor *) dbc->string->serial;
+	return utf16s_to_utf8s((wchar_t *) s_desc->wData, s_desc->bLength / 2,
+			UTF16_LITTLE_ENDIAN, buf, DBC_MAX_STRING_LENGTH);
+}
+
+static ssize_t dbc_serial_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	struct usb_string_descriptor    *s_desc;
+	int ret;
+	struct dbc_info_context	*info;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	s_desc = (struct usb_string_descriptor *) dbc->string->serial;
+	ret = utf8s_to_utf16s(buf, strlen(buf),
+				UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
+				DBC_MAX_STRING_LENGTH);
+	if (ret < 0)
+		return ret;
+	s_desc->bLength         = (strlen(buf) + 1) * 2;
+	info			= (struct dbc_info_context *)dbc->ctx->bytes;
+	info->length		= (info->length & ~(0xffu << 24))
+					 |  (s_desc->bLength) << 24;
+	return size;
+}
+
+static ssize_t dbc_protocol_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	void __iomem *ptr;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	ptr = &dbc->regs->devinfo1;
+	return sprintf(buf, "%02x\n", le32_to_cpu(readl(ptr)) & 0xff);
+}
+
+static ssize_t dbc_protocol_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	int value;
+	int ret;
+	void __iomem *ptr;
+	u32 dev_info;
+
+	ret = kstrtoint(buf, 0, &value);
+	if (ret || value < 0 || value > 0xff)
+		return -EINVAL;
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	ptr = &dbc->regs->devinfo1;
+	dev_info = le32_to_cpu(readl(ptr));
+	dev_info = cpu_to_le32((dev_info & ~(0xffu)) | value);
+	writel(dev_info, ptr);
+	return size;
+}
+
+static ssize_t dbc_vid_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	void __iomem *ptr;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	ptr = &dbc->regs->devinfo1;
+	return sprintf(buf, "%04x\n", ((u32)le32_to_cpu(readl(ptr))) >> 16);
+}
+
+static ssize_t dbc_vid_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	int value;
+	int ret;
+	void __iomem *ptr;
+	u32 dev_info;
+
+	ret = kstrtoint(buf, 0, &value);
+	if (ret || value < 0 || value > 0xffff)
+		return -EINVAL;
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	ptr = &dbc->regs->devinfo1;
+	dev_info = le32_to_cpu(readl(ptr));
+	dev_info = cpu_to_le32((dev_info & ~(0xffffu << 16)) | (value << 16));
+	writel(dev_info, ptr);
+	return size;
+}
+
+
+static ssize_t dbc_pid_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	void __iomem *ptr;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	ptr = &dbc->regs->devinfo2;
+	return sprintf(buf, "%04x\n", le32_to_cpu(readl(ptr)) & 0xffff);
+}
+
+static ssize_t dbc_pid_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	int value;
+	int ret;
+	void __iomem *ptr;
+	u32 dev_info;
+
+	ret = kstrtoint(buf, 0, &value);
+	if (ret || value < 0 || value > 0xffff)
+		return -EINVAL;
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	ptr = &dbc->regs->devinfo2;
+	dev_info = le32_to_cpu(readl(ptr));
+	dev_info = cpu_to_le32((dev_info & ~(0xffffu)) | value);
+	writel(dev_info, ptr);
+	return size;
+}
+
+static ssize_t dbc_device_rev_show(struct device *dev,
+			struct device_attribute *attr,
+			char *buf)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	void __iomem *ptr;
+
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	ptr = &dbc->regs->devinfo2;
+	return sprintf(buf, "%04x\n",  ((u32)le32_to_cpu(readl(ptr))) >> 16);
+}
+
+static ssize_t dbc_device_rev_store(struct device *dev,
+			 struct device_attribute *attr,
+			 const char *buf, size_t size)
+{
+	struct xhci_dbc         *dbc;
+	struct xhci_hcd         *xhci;
+	int value;
+	int ret;
+	void __iomem *ptr;
+	u32 dev_info;
+
+	ret = kstrtoint(buf, 0, &value);
+	if (ret || value < 0 || value > 0xffff)
+		return -EINVAL;
+	xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	dbc = xhci->dbc;
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+	ptr = &dbc->regs->devinfo2;
+	dev_info = le32_to_cpu(readl(ptr));
+	dev_info = cpu_to_le32((dev_info & ~(0xffffu << 16)) | (value << 16));
+	writel(dev_info, ptr);
+	return size;
+}
+
 static ssize_t dbc_show(struct device *dev,
 			struct device_attribute *attr,
 			char *buf)
@@ -910,6 +1219,29 @@ static ssize_t dbc_show(struct device *dev,
 	return sprintf(buf, "%s\n", p);
 }
 
+static DEVICE_ATTR_RW(dbc_manufacturer);
+static DEVICE_ATTR_RW(dbc_product);
+static DEVICE_ATTR_RW(dbc_serial);
+static DEVICE_ATTR_RW(dbc_protocol);
+static DEVICE_ATTR_RW(dbc_vid);
+static DEVICE_ATTR_RW(dbc_pid);
+static DEVICE_ATTR_RW(dbc_device_rev);
+
+static struct attribute *dbc_descriptor_attributes[] = {
+	&dev_attr_dbc_manufacturer.attr,
+	&dev_attr_dbc_product.attr,
+	&dev_attr_dbc_serial.attr,
+	&dev_attr_dbc_protocol.attr,
+	&dev_attr_dbc_vid.attr,
+	&dev_attr_dbc_pid.attr,
+	&dev_attr_dbc_device_rev.attr,
+	NULL
+};
+
+static const struct attribute_group dbc_descriptor_attrib_grp = {
+	.attrs = dbc_descriptor_attributes,
+};
+
 static ssize_t dbc_store(struct device *dev,
 			 struct device_attribute *attr,
 			 const char *buf, size_t count)
@@ -938,6 +1270,10 @@ static ssize_t dbc_store(struct device *dev,
 			goto err;
 		}
 		xhci_dbc_start(xhci);
+		ret = sysfs_create_group(&dev->kobj,
+					 &dbc_descriptor_attrib_grp);
+		if (ret)
+			goto err;
 	} else if (!strncmp(buf, "disable", 7) && dbc->state != DS_DISABLED) {
 		if (!dbc_registered_func)
 			return -EINVAL;
@@ -945,6 +1281,7 @@ static ssize_t dbc_store(struct device *dev,
 		if (dbc_registered_func->stop)
 			dbc_registered_func->stop(dbc);
 		module_put(dbc_registered_func->owner);
+		sysfs_remove_group(&dev->kobj, &dbc_descriptor_attrib_grp);
 	} else
 		return -EINVAL;
 
@@ -955,9 +1292,11 @@ static ssize_t dbc_store(struct device *dev,
 }
 
 static DEVICE_ATTR_RW(dbc);
+static DEVICE_ATTR_RO(dbc_function);
 
 static struct attribute *dbc_dev_attributes[] = {
 	&dev_attr_dbc.attr,
+	&dev_attr_dbc_function.attr,
 	NULL
 };
 
-- 
2.21.0

Re: [PATCH 3/5] usb: xhci: dbc: Provide sysfs option to configure dbc descriptors

[Greg KH]

On Fri, Jun 07, 2019 at 12:03:04PM +0530, Prabhat Chand Pandey wrote:
> From: "K V, Abhilash" <abhilash.k.v@intel.com>
> 
> Show the active dbc function and dbc descriptors, allowing
> user space to dynamically modify the descriptors
> 
> The DBC specific sysfs attributes are separated into two groups,
> in the first group there are dbc & dbc_function sysfs attributes and in
> second group all other DBC descriptor specific sysfs attributes.
> 
> First group of attributes will be populated at the time of dbc_init and
> second group of attributes will only be populated when "dbc" attribute
> value is set to "enable".
> 
> Whenever "dbc" attribute value will be "disable" then second group
> of attributes will be removed.
> 
> Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> Signed-off-by: Gururaj K <gururaj.k@intel.com>
> Signed-off-by: K V, Abhilash <abhilash.k.v@intel.com>
> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 112 ++++++
>  drivers/usb/host/xhci-dbgcap.c                | 339 ++++++++++++++++++
>  2 files changed, 451 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> index 0088aba4caa8..b46b6afc6c4a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> @@ -23,3 +23,115 @@ Description:
>  		Reading this attribute gives the state of the DbC. It
>  		can be one of the following states: disabled, enabled,
>  		initialized, connected, configured and stalled.
> +
> +What:		/sys/bus/pci/drivers/xhci_hcd/.../dbc_function

You are putting xhci-specific files in a pci device directory.  why not
put it in your host controller device instead?  That's the best place
for this, and what about xhci drivers that are NOT pci?

Don't abuse the pci device for things that are not pci.

> +static struct attribute *dbc_descriptor_attributes[] = {
> +	&dev_attr_dbc_manufacturer.attr,
> +	&dev_attr_dbc_product.attr,
> +	&dev_attr_dbc_serial.attr,
> +	&dev_attr_dbc_protocol.attr,
> +	&dev_attr_dbc_vid.attr,
> +	&dev_attr_dbc_pid.attr,
> +	&dev_attr_dbc_device_rev.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group dbc_descriptor_attrib_grp = {
> +	.attrs = dbc_descriptor_attributes,
> +};

ATTRIBUTE_GROUPS()?

> +
>  static ssize_t dbc_store(struct device *dev,
>  			 struct device_attribute *attr,
>  			 const char *buf, size_t count)
> @@ -938,6 +1270,10 @@ static ssize_t dbc_store(struct device *dev,
>  			goto err;
>  		}
>  		xhci_dbc_start(xhci);
> +		ret = sysfs_create_group(&dev->kobj,
> +					 &dbc_descriptor_attrib_grp);

If you EVER have to make a sysfs call within a driver, that is a HUGE
red flag that you are doing something wrong.

You are doing something wrong here, you just raced with userspace and
created a bunch of new files that userspace will not see.

Please do this correctly by setting the default device group for the
driver.

> +		if (ret)
> +			goto err;
>  	} else if (!strncmp(buf, "disable", 7) && dbc->state != DS_DISABLED) {
>  		if (!dbc_registered_func)
>  			return -EINVAL;
> @@ -945,6 +1281,7 @@ static ssize_t dbc_store(struct device *dev,
>  		if (dbc_registered_func->stop)
>  			dbc_registered_func->stop(dbc);
>  		module_put(dbc_registered_func->owner);
> +		sysfs_remove_group(&dev->kobj, &dbc_descriptor_attrib_grp);

Same here.

And does this really remove the files for when the PCI device is removed
from the system?  Or the driver from the device?  It doesn't seem like
it, but I might be missing a codepath here...

thanks,

greg k-h

[PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Prabhat Chand Pandey]

From: Abhilash K V <abhilash.k.v@intel.com>

This patch provides a raw device interface on xhci Debug capability.
This abstracts dbc functionality to user space inorder to facilitate
various frameworks to utilize xhci debug capability.

It helps to render the target as an usb debug class device on host and
establish an usb connection by providing two bulk endpoints.

[don't dynamically allocate tiny space for name only -Mathias]
Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/Kconfig       |   9 +
 drivers/usb/host/Makefile      |   1 +
 drivers/usb/host/xhci-dbgraw.c | 365 +++++++++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)
 create mode 100644 drivers/usb/host/xhci-dbgraw.c

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index c29ed8a61dcb..0f801977cd1e 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -48,6 +48,15 @@ config USB_XHCI_DBGCAP_TTY
 	  debug capability. This will expose a /dev/ttyDBC* device node on device
 	  which may be used by the usb-debug driver on the debug host.
 	  If unsure, say 'N'.
+
+config USB_XHCI_DBGCAP_RAW
+       tristate "xHCI DbC raw driver support"
+       depends on USB_XHCI_HCD && USB_XHCI_DBGCAP
+       help
+         Say 'Y' to enable the support for the raw driver interface to xHCI
+         debug capability. This will expose a device node corresponding to
+         1 bulk IN and 1 bulk OUT endpoints to be presented to debug host.
+         If unsure, say 'N'.
 endchoice
 
 config USB_XHCI_PCI
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index b21b0ea9e966..a4aee6a5daf0 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -20,6 +20,7 @@ ifneq ($(CONFIG_USB_XHCI_DBGCAP), )
 endif
 
 obj-$(CONFIG_USB_XHCI_DBGCAP_TTY) += xhci-dbgtty.o
+obj-$(CONFIG_USB_XHCI_DBGCAP_RAW) += xhci-dbgraw.o
 
 ifneq ($(CONFIG_USB_XHCI_MTK), )
 	xhci-hcd-y += xhci-mtk-sch.o
diff --git a/drivers/usb/host/xhci-dbgraw.c b/drivers/usb/host/xhci-dbgraw.c
new file mode 100644
index 000000000000..f7ca4b089dbd
--- /dev/null
+++ b/drivers/usb/host/xhci-dbgraw.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0+
+/**
+ * Raw DbC for xHCI debug capability
+ *
+ * Copyright (C) 2019 Intel Corporation
+ *
+ * Author: Rajaram Regupathy <rajaram.regupathy@intel.com>
+ * Author: Abhilash K V <abhilash.k.v@intel.com>
+ * Author: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
+ */
+
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+
+#include "xhci.h"
+#include "xhci-dbgcap.h"
+
+#define DBC_XHCI_MINORS     8
+#define DBC_STR_FUNC_RAW    "RAW"
+#define DBC_RAW_BULK_BUFFER_SIZE  (SZ_64K)
+
+static DEFINE_IDR(dbc_minors);
+
+struct dbc_dev {
+	struct mutex dev_excl;
+	struct mutex read_excl;
+	struct mutex write_excl;
+
+	wait_queue_head_t read_wq;
+	wait_queue_head_t write_wq;
+
+	int error;
+	bool in_use;
+	char name[16];
+	struct xhci_dbc *dbc;
+	struct miscdevice misc_dev;
+};
+
+static void xhci_dbc_free_req(struct dbc_ep *dep, struct dbc_request *req)
+{
+	kfree(req->buf);
+	dbc_free_request(dep, req);
+}
+
+struct dbc_request *xhci_dbc_alloc_requests(struct dbc_ep *dep,
+		void (*fn)(struct xhci_hcd *, struct dbc_request *))
+{
+	struct dbc_request *req;
+
+	req = dbc_alloc_request(dep, GFP_KERNEL);
+	if (!req)
+		return req;
+
+	req->length = DBC_RAW_BULK_BUFFER_SIZE;
+	req->buf = kmalloc(req->length, GFP_KERNEL);
+	if (!req->buf)
+		xhci_dbc_free_req(dep, req);
+
+	req->complete = fn;
+
+	return req;
+}
+
+static void dbc_complete_in(struct xhci_hcd *xhci,
+				struct dbc_request *req)
+{
+	struct xhci_dbc *dbc = (struct xhci_dbc *) xhci->dbc;
+	struct dbc_dev *dev = (struct dbc_dev *) dbc->func_priv;
+
+	if (req->status)
+		dev->error = req->status;
+
+	wake_up(&dev->write_wq);
+}
+
+static void dbc_complete_out(struct xhci_hcd *xhci,
+				struct dbc_request *req)
+{
+	struct xhci_dbc *dbc = (struct xhci_dbc *) xhci->dbc;
+	struct dbc_dev *dev = (struct dbc_dev *) dbc->func_priv;
+
+	if (req->status)
+		dev->error = req->status;
+
+	wake_up(&dev->read_wq);
+}
+
+static ssize_t dbc_read(struct file *fp, char __user *buf,
+				size_t count, loff_t *pos)
+{
+	int status = 0;
+	struct dbc_dev *dev = (struct dbc_dev *) fp->private_data;
+	struct xhci_dbc   *dbc = dev->dbc;
+	struct dbc_request *req;
+	struct dbc_port   *port = &dbc->port;
+	int r = count, xfer;
+	int ret;
+
+	if (dbc->state != DS_CONFIGURED)
+		return -EAGAIN;
+
+	port->in = get_in_ep(dbc->xhci);
+
+	mutex_lock(&dev->read_excl);
+
+	req = xhci_dbc_alloc_requests(port->in, dbc_complete_out);
+	if (!req) {
+		r = -ENOMEM;
+		goto alloc_fail;
+	}
+
+	req->actual = 0;
+
+	xfer = min_t(size_t, count, DBC_RAW_BULK_BUFFER_SIZE);
+	req->length = xfer;
+
+	status = dbc_ep_queue(port->in, req, GFP_ATOMIC);
+	if (status) {
+		dev->error = status;
+		r = status;
+		goto request_fail;
+	}
+
+	ret = wait_event_interruptible(dev->read_wq,
+			(req->status != -EINPROGRESS));
+
+	if (ret < 0) {
+		r = ret;
+		goto request_fail;
+	}
+
+	if (dev->error) {
+		r = dev->error;
+		goto request_fail;
+	}
+
+	xfer = (req->actual < count) ? req->actual : count;
+	if (!req->actual) {
+		r = 0;
+	} else {
+		r = copy_to_user(buf, req->buf, xfer);
+		if (!r)
+			r = xfer;
+	}
+
+request_fail:
+	xhci_dbc_free_req(port->in, req);
+alloc_fail:
+	mutex_unlock(&dev->read_excl);
+
+	return r;
+}
+
+static ssize_t dbc_write(struct file *fp, const char __user *buf,
+				size_t count, loff_t *pos)
+{
+	int status = 0;
+	struct dbc_dev *dev = (struct dbc_dev *) fp->private_data;
+	struct xhci_dbc *dbc = dev->dbc;
+	struct dbc_request *req = 0;
+	struct dbc_port   *port = &dbc->port;
+	int r = count, xfer;
+	int ret;
+
+	if (dbc->state != DS_CONFIGURED)
+		return -EAGAIN;
+
+	port->out = get_out_ep(dbc->xhci);
+
+	mutex_lock(&dev->write_excl);
+
+	/* get an idle tx request to use */
+	req = xhci_dbc_alloc_requests(port->out, dbc_complete_in);
+	if (!req) {
+		r = -ENOMEM;
+		goto alloc_fail;
+	}
+
+	req->actual = 0;
+	xfer = min_t(size_t, count, DBC_RAW_BULK_BUFFER_SIZE);
+
+	ret = copy_from_user(req->buf, buf, xfer);
+	if (ret) {
+		r = ret;
+		goto request_fail;
+	}
+	r = xfer;
+	req->length = xfer;
+	status = dbc_ep_queue(port->out, req, GFP_ATOMIC);
+	if (status) {
+		dev->error = status;
+		r = status;
+		goto request_fail;
+	}
+
+	ret = wait_event_interruptible(dev->write_wq,
+			(req->status != -EINPROGRESS));
+	if (ret < 0)
+		r = ret;
+
+request_fail:
+	xhci_dbc_free_req(port->out, req);
+alloc_fail:
+	mutex_unlock(&dev->write_excl);
+
+	return r;
+}
+
+static int dbc_open(struct inode *ip, struct file *fp)
+{
+	struct dbc_dev *dbc_dev;
+	struct xhci_dbc *dbc;
+	int r = 0;
+
+	dbc_dev = container_of(fp->private_data, struct dbc_dev, misc_dev);
+
+	mutex_lock(&dbc_dev->dev_excl);
+	if (dbc_dev->in_use) {
+		r = -EBUSY;
+		goto err;
+	}
+
+	dbc = dbc_dev->dbc;
+	if (!dbc) {
+		r =  -ENODEV;
+		goto err;
+	}
+
+	dbc_dev->in_use = true;
+	fp->private_data = dbc_dev;
+
+	/* clear the error latch */
+	dbc_dev->error = 0;
+err:
+	mutex_unlock(&dbc_dev->dev_excl);
+
+	return r;
+}
+
+static int dbc_release(struct inode *ip, struct file *fp)
+{
+	struct dbc_dev *dbc_dev = (struct dbc_dev *) fp->private_data;
+
+
+	mutex_lock(&dbc_dev->dev_excl);
+	dbc_dev->in_use = false;
+	fp->private_data = NULL;
+	mutex_unlock(&dbc_dev->dev_excl);
+
+	return 0;
+}
+
+static const struct file_operations dbc_fops = {
+	.owner = THIS_MODULE,
+	.read = dbc_read,
+	.write = dbc_write,
+	.open = dbc_open,
+	.release = dbc_release,
+};
+
+static int dbc_raw_register_device(struct xhci_hcd *xhci)
+{
+	struct device *devm = xhci->main_hcd->self.controller;
+	struct xhci_dbc *dbc = xhci->dbc;
+	struct dbc_dev *dev;
+	int ret;
+	int id;
+
+	dev = devm_kzalloc(devm, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	mutex_init(&dev->dev_excl);
+	mutex_init(&dev->read_excl);
+	mutex_init(&dev->write_excl);
+
+	init_waitqueue_head(&dev->read_wq);
+	init_waitqueue_head(&dev->write_wq);
+
+	id = idr_alloc(&dbc_minors, dbc, 0, DBC_XHCI_MINORS,
+			GFP_KERNEL);
+	if (id < 0)
+		return id;
+	snprintf(dev->name, sizeof(dev->name), "dbc_raw%d", id);
+	dev->misc_dev.name = dev->name;
+	dev->misc_dev.minor = MISC_DYNAMIC_MINOR;
+
+	dev->misc_dev.fops = &dbc_fops;
+
+	ret = misc_register(&dev->misc_dev);
+	if (ret) {
+		kfree(dev->misc_dev.name);
+		pr_err("failed to register misc dev: %d\n", ret);
+		goto error;
+	}
+
+	dev->dbc = dbc;
+	dbc->func_priv = (void *) dev;
+	return ret;
+
+error:
+	idr_remove(&dbc_minors, id);
+
+	return ret;
+}
+
+static void dbc_raw_unregister_device(struct xhci_hcd *xhci)
+{
+	struct xhci_dbc *dbc = xhci->dbc;
+	struct dbc_dev *dbc_dev = (struct dbc_dev *) dbc->func_priv;
+
+	if (dbc_dev) {
+		idr_remove(&dbc_minors, dbc_dev->misc_dev.minor);
+		misc_deregister(&dbc_dev->misc_dev);
+	}
+	idr_destroy(&dbc_minors);
+}
+
+static int dbc_raw_run(struct xhci_dbc *dbc)
+{
+	return dbc_raw_register_device(dbc->xhci);
+}
+
+static int dbc_raw_stop(struct xhci_dbc *dbc)
+{
+	dbc_raw_unregister_device(dbc->xhci);
+	return 0;
+}
+
+static struct dbc_function raw_func = {
+	.owner = THIS_MODULE,
+	.string = {
+		.manufacturer = DBC_STR_MANUFACTURER,
+		.product = DBC_STR_PRODUCT,
+		.serial = DBC_STR_SERIAL,
+	},
+	.protocol = DBC_PROTOCOL,
+	.vid = DBC_VENDOR_ID,
+	.pid = DBC_PRODUCT_ID,
+	.device_rev = DBC_DEVICE_REV,
+	.func_name = DBC_STR_FUNC_RAW,
+
+	.run = dbc_raw_run,
+	.stop = dbc_raw_stop,
+};
+
+static int __init xhci_dbc_raw_init(void)
+{
+	return xhci_dbc_register_function(&raw_func);
+}
+
+static void __exit xhci_dbc_raw_fini(void)
+{
+	xhci_dbc_unregister_function();
+}
+
+late_initcall(xhci_dbc_raw_init);
+module_exit(xhci_dbc_raw_fini);
+
+MODULE_DESCRIPTION("xHCI DbC raw driver");
+MODULE_AUTHOR("Rajaram Regupathy");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Greg KH]

On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
> From: Abhilash K V <abhilash.k.v@intel.com>
> 
> This patch provides a raw device interface on xhci Debug capability.

What is a "raw device"?

> This abstracts dbc functionality to user space inorder to facilitate
> various frameworks to utilize xhci debug capability.

I do not understand this sentance at all.  Please provide a lot more
information.

> It helps to render the target as an usb debug class device on host and
> establish an usb connection by providing two bulk endpoints.

provide bulk endpoints where?  To send data where?  This is very
confusing and does not make any sense to me...


> 
> [don't dynamically allocate tiny space for name only -Mathias]
> Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/Kconfig       |   9 +
>  drivers/usb/host/Makefile      |   1 +
>  drivers/usb/host/xhci-dbgraw.c | 365 +++++++++++++++++++++++++++++++++
>  3 files changed, 375 insertions(+)
>  create mode 100644 drivers/usb/host/xhci-dbgraw.c
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index c29ed8a61dcb..0f801977cd1e 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -48,6 +48,15 @@ config USB_XHCI_DBGCAP_TTY
>  	  debug capability. This will expose a /dev/ttyDBC* device node on device
>  	  which may be used by the usb-debug driver on the debug host.
>  	  If unsure, say 'N'.
> +
> +config USB_XHCI_DBGCAP_RAW
> +       tristate "xHCI DbC raw driver support"
> +       depends on USB_XHCI_HCD && USB_XHCI_DBGCAP
> +       help
> +         Say 'Y' to enable the support for the raw driver interface to xHCI
> +         debug capability. This will expose a device node corresponding to
> +         1 bulk IN and 1 bulk OUT endpoints to be presented to debug host.
> +         If unsure, say 'N'.

module name?

>  endchoice
>  
>  config USB_XHCI_PCI
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index b21b0ea9e966..a4aee6a5daf0 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -20,6 +20,7 @@ ifneq ($(CONFIG_USB_XHCI_DBGCAP), )
>  endif
>  
>  obj-$(CONFIG_USB_XHCI_DBGCAP_TTY) += xhci-dbgtty.o
> +obj-$(CONFIG_USB_XHCI_DBGCAP_RAW) += xhci-dbgraw.o
>  
>  ifneq ($(CONFIG_USB_XHCI_MTK), )
>  	xhci-hcd-y += xhci-mtk-sch.o
> diff --git a/drivers/usb/host/xhci-dbgraw.c b/drivers/usb/host/xhci-dbgraw.c
> new file mode 100644
> index 000000000000..f7ca4b089dbd
> --- /dev/null
> +++ b/drivers/usb/host/xhci-dbgraw.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/**
> + * Raw DbC for xHCI debug capability
> + *
> + * Copyright (C) 2019 Intel Corporation
> + *
> + * Author: Rajaram Regupathy <rajaram.regupathy@intel.com>
> + * Author: Abhilash K V <abhilash.k.v@intel.com>
> + * Author: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> + */
> +
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +
> +#include "xhci.h"
> +#include "xhci-dbgcap.h"
> +
> +#define DBC_XHCI_MINORS     8
> +#define DBC_STR_FUNC_RAW    "RAW"
> +#define DBC_RAW_BULK_BUFFER_SIZE  (SZ_64K)
> +
> +static DEFINE_IDR(dbc_minors);

These are not minor numbers, they are semi-random device ids.  Don't
confuse them with a minor number.

> +
> +struct dbc_dev {
> +	struct mutex dev_excl;
> +	struct mutex read_excl;
> +	struct mutex write_excl;

What do these protect?

> +	wait_queue_head_t read_wq;
> +	wait_queue_head_t write_wq;
> +
> +	int error;

error of what?

> +	bool in_use;
> +	char name[16];

why do you need this?  What's wrong with the misc device name?

> +	struct xhci_dbc *dbc;
> +	struct miscdevice misc_dev;
> +};
> +
> +static void xhci_dbc_free_req(struct dbc_ep *dep, struct dbc_request *req)
> +{
> +	kfree(req->buf);
> +	dbc_free_request(dep, req);
> +}
> +
> +struct dbc_request *xhci_dbc_alloc_requests(struct dbc_ep *dep,
> +		void (*fn)(struct xhci_hcd *, struct dbc_request *))
> +{
> +	struct dbc_request *req;
> +
> +	req = dbc_alloc_request(dep, GFP_KERNEL);
> +	if (!req)
> +		return req;
> +
> +	req->length = DBC_RAW_BULK_BUFFER_SIZE;
> +	req->buf = kmalloc(req->length, GFP_KERNEL);
> +	if (!req->buf)
> +		xhci_dbc_free_req(dep, req);
> +
> +	req->complete = fn;
> +
> +	return req;
> +}
> +
> +static void dbc_complete_in(struct xhci_hcd *xhci,
> +				struct dbc_request *req)
> +{
> +	struct xhci_dbc *dbc = (struct xhci_dbc *) xhci->dbc;
> +	struct dbc_dev *dev = (struct dbc_dev *) dbc->func_priv;
> +
> +	if (req->status)
> +		dev->error = req->status;
> +
> +	wake_up(&dev->write_wq);
> +}
> +
> +static void dbc_complete_out(struct xhci_hcd *xhci,
> +				struct dbc_request *req)
> +{
> +	struct xhci_dbc *dbc = (struct xhci_dbc *) xhci->dbc;
> +	struct dbc_dev *dev = (struct dbc_dev *) dbc->func_priv;
> +
> +	if (req->status)
> +		dev->error = req->status;
> +
> +	wake_up(&dev->read_wq);
> +}
> +
> +static ssize_t dbc_read(struct file *fp, char __user *buf,
> +				size_t count, loff_t *pos)
> +{
> +	int status = 0;
> +	struct dbc_dev *dev = (struct dbc_dev *) fp->private_data;
> +	struct xhci_dbc   *dbc = dev->dbc;
> +	struct dbc_request *req;
> +	struct dbc_port   *port = &dbc->port;
> +	int r = count, xfer;
> +	int ret;
> +
> +	if (dbc->state != DS_CONFIGURED)
> +		return -EAGAIN;
> +
> +	port->in = get_in_ep(dbc->xhci);
> +
> +	mutex_lock(&dev->read_excl);
> +
> +	req = xhci_dbc_alloc_requests(port->in, dbc_complete_out);
> +	if (!req) {
> +		r = -ENOMEM;
> +		goto alloc_fail;
> +	}
> +
> +	req->actual = 0;
> +
> +	xfer = min_t(size_t, count, DBC_RAW_BULK_BUFFER_SIZE);
> +	req->length = xfer;
> +
> +	status = dbc_ep_queue(port->in, req, GFP_ATOMIC);
> +	if (status) {
> +		dev->error = status;
> +		r = status;
> +		goto request_fail;
> +	}
> +
> +	ret = wait_event_interruptible(dev->read_wq,
> +			(req->status != -EINPROGRESS));
> +
> +	if (ret < 0) {
> +		r = ret;
> +		goto request_fail;
> +	}
> +
> +	if (dev->error) {
> +		r = dev->error;
> +		goto request_fail;
> +	}
> +
> +	xfer = (req->actual < count) ? req->actual : count;
> +	if (!req->actual) {
> +		r = 0;
> +	} else {
> +		r = copy_to_user(buf, req->buf, xfer);
> +		if (!r)
> +			r = xfer;
> +	}
> +
> +request_fail:
> +	xhci_dbc_free_req(port->in, req);
> +alloc_fail:
> +	mutex_unlock(&dev->read_excl);
> +
> +	return r;
> +}
> +
> +static ssize_t dbc_write(struct file *fp, const char __user *buf,
> +				size_t count, loff_t *pos)
> +{
> +	int status = 0;
> +	struct dbc_dev *dev = (struct dbc_dev *) fp->private_data;
> +	struct xhci_dbc *dbc = dev->dbc;
> +	struct dbc_request *req = 0;
> +	struct dbc_port   *port = &dbc->port;
> +	int r = count, xfer;
> +	int ret;
> +
> +	if (dbc->state != DS_CONFIGURED)
> +		return -EAGAIN;

Why?  What does this mean to userspace?  How do they configure it?

> +
> +	port->out = get_out_ep(dbc->xhci);
> +
> +	mutex_lock(&dev->write_excl);
> +
> +	/* get an idle tx request to use */
> +	req = xhci_dbc_alloc_requests(port->out, dbc_complete_in);
> +	if (!req) {
> +		r = -ENOMEM;
> +		goto alloc_fail;
> +	}
> +
> +	req->actual = 0;
> +	xfer = min_t(size_t, count, DBC_RAW_BULK_BUFFER_SIZE);
> +
> +	ret = copy_from_user(req->buf, buf, xfer);
> +	if (ret) {
> +		r = ret;
> +		goto request_fail;
> +	}

Ok, I'm going to blame Mathias for not actually reading this code before
acking it.  This is just not right, come on.

> +	r = xfer;
> +	req->length = xfer;
> +	status = dbc_ep_queue(port->out, req, GFP_ATOMIC);
> +	if (status) {
> +		dev->error = status;
> +		r = status;
> +		goto request_fail;
> +	}
> +
> +	ret = wait_event_interruptible(dev->write_wq,
> +			(req->status != -EINPROGRESS));
> +	if (ret < 0)
> +		r = ret;
> +
> +request_fail:
> +	xhci_dbc_free_req(port->out, req);
> +alloc_fail:
> +	mutex_unlock(&dev->write_excl);
> +
> +	return r;
> +}
> +
> +static int dbc_open(struct inode *ip, struct file *fp)
> +{
> +	struct dbc_dev *dbc_dev;
> +	struct xhci_dbc *dbc;
> +	int r = 0;
> +
> +	dbc_dev = container_of(fp->private_data, struct dbc_dev, misc_dev);
> +
> +	mutex_lock(&dbc_dev->dev_excl);
> +	if (dbc_dev->in_use) {
> +		r = -EBUSY;
> +		goto err;
> +	}

No, just no.  Don't try to enforce "only one user" in the kernel, it
always fails, is a horrible mess, and can be trivially worked around.
If a user wants to open the device multiple times and do crazy things
with it, they deserve the mess it creates.

For example, we don't forbid tty devices from being opened multiple
times, if someone is foolish enough to do that, they get to keep the
pieces.

Also, this code is wrong, and doesn't really prevent it from happening
:)

> +
> +	dbc = dbc_dev->dbc;
> +	if (!dbc) {
> +		r =  -ENODEV;
> +		goto err;
> +	}
> +
> +	dbc_dev->in_use = true;
> +	fp->private_data = dbc_dev;
> +
> +	/* clear the error latch */
> +	dbc_dev->error = 0;
> +err:
> +	mutex_unlock(&dbc_dev->dev_excl);
> +
> +	return r;
> +}
> +
> +static int dbc_release(struct inode *ip, struct file *fp)
> +{
> +	struct dbc_dev *dbc_dev = (struct dbc_dev *) fp->private_data;
> +
> +

extra whitespace???

And again with the pointless casting :(

> +	mutex_lock(&dbc_dev->dev_excl);
> +	dbc_dev->in_use = false;
> +	fp->private_data = NULL;

Why?  the file descriptor is about to be freed, no need to care about
this anymore.

> +	mutex_unlock(&dbc_dev->dev_excl);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations dbc_fops = {
> +	.owner = THIS_MODULE,
> +	.read = dbc_read,
> +	.write = dbc_write,
> +	.open = dbc_open,
> +	.release = dbc_release,
> +};

So you have a new char device, with a undocumented and unknown format of
data flowing across it to the device.  How in the world are we supposed
to use this thing?  Where is it documented?  What does it do?  How can
you use it?

I don't mean to be so harsh here, but come on people, this stuff needs a
lot more background documentation, information, and explaination as to
exactly why in the world we need any of this, and what it even does!

Also, you need to fix the code, it doesn't work as pointed out in a few
places :)

greg k-h

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Mathias Nyman]

On 7.6.2019 17.21, Greg KH wrote:
> On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
>> From: Abhilash K V <abhilash.k.v@intel.com>
>>
>> This patch provides a raw device interface on xhci Debug capability.
> 
> What is a "raw device"?
> 
>> This abstracts dbc functionality to user space inorder to facilitate
>> various frameworks to utilize xhci debug capability.
> 
> I do not understand this sentance at all.  Please provide a lot more
> information.
> 
>> It helps to render the target as an usb debug class device on host and
>> establish an usb connection by providing two bulk endpoints.
> 
> provide bulk endpoints where?  To send data where?  This is very
> confusing and does not make any sense to me...
> 
> 
>>
>> [don't dynamically allocate tiny space for name only -Mathias]
>> Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
>> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
>> Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
>> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
...
> 
> So you have a new char device, with a undocumented and unknown format of
> data flowing across it to the device.  How in the world are we supposed
> to use this thing?  Where is it documented?  What does it do?  How can
> you use it?
> 
> I don't mean to be so harsh here, but come on people, this stuff needs a
> lot more background documentation, information, and explaination as to
> exactly why in the world we need any of this, and what it even does!
> 
> Also, you need to fix the code, it doesn't work as pointed out in a few
> places :)
> 

Thanks for going through this.
It's now clear this is far from ready.
I need to re-evaluate my position on this, not just the code and the documentation,
but the usefulness of it all.

-Mathias

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Greg KH]

On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> On 7.6.2019 17.21, Greg KH wrote:
> > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
> > > From: Abhilash K V <abhilash.k.v@intel.com>
> > > 
> > > This patch provides a raw device interface on xhci Debug capability.
> > 
> > What is a "raw device"?
> > 
> > > This abstracts dbc functionality to user space inorder to facilitate
> > > various frameworks to utilize xhci debug capability.
> > 
> > I do not understand this sentance at all.  Please provide a lot more
> > information.
> > 
> > > It helps to render the target as an usb debug class device on host and
> > > establish an usb connection by providing two bulk endpoints.
> > 
> > provide bulk endpoints where?  To send data where?  This is very
> > confusing and does not make any sense to me...
> > 
> > 
> > > 
> > > [don't dynamically allocate tiny space for name only -Mathias]
> > > Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> > > Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> > > Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> > > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> ...
> > 
> > So you have a new char device, with a undocumented and unknown format of
> > data flowing across it to the device.  How in the world are we supposed
> > to use this thing?  Where is it documented?  What does it do?  How can
> > you use it?
> > 
> > I don't mean to be so harsh here, but come on people, this stuff needs a
> > lot more background documentation, information, and explaination as to
> > exactly why in the world we need any of this, and what it even does!
> > 
> > Also, you need to fix the code, it doesn't work as pointed out in a few
> > places :)
> > 
> 
> Thanks for going through this.
> It's now clear this is far from ready.
> I need to re-evaluate my position on this, not just the code and the documentation,
> but the usefulness of it all.

What is this even supposed to be used for?  What is the application for
it?  I couldn't determine that at all, what am I missing?

thanks,

greg k-h

RE: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Regupathy, Rajaram]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Monday, June 10, 2019 7:46 PM
> To: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: Pandey, Prabhat Chand <prabhat.chand.pandey@intel.com>; linux-
> usb@vger.kernel.org; Nyman, Mathias <mathias.nyman@intel.com>;
> Regupathy, Rajaram <rajaram.regupathy@intel.com>; K V, Abhilash
> <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com>
> Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> interface on DbC
> Importance: High
> 
> On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> > On 7.6.2019 17.21, Greg KH wrote:
> > > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
> > > > From: Abhilash K V <abhilash.k.v@intel.com>
> > > >
> > > > This patch provides a raw device interface on xhci Debug capability.
> > >
> > > What is a "raw device"?
> > >
> > > > This abstracts dbc functionality to user space inorder to
> > > > facilitate various frameworks to utilize xhci debug capability.
> > >
> > > I do not understand this sentance at all.  Please provide a lot more
> > > information.
> > >
> > > > It helps to render the target as an usb debug class device on host
> > > > and establish an usb connection by providing two bulk endpoints.
> > >
> > > provide bulk endpoints where?  To send data where?  This is very
> > > confusing and does not make any sense to me...
> > >
> > >
> > > >
> > > > [don't dynamically allocate tiny space for name only -Mathias]
> > > > Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> > > > Signed-off-by: Prabhat Chand Pandey
> > > > <prabhat.chand.pandey@intel.com>
> > > > Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> > > > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > ---
> > ...
> > >
> > > So you have a new char device, with a undocumented and unknown
> > > format of data flowing across it to the device.  How in the world
> > > are we supposed to use this thing?  Where is it documented?  What
> > > does it do?  How can you use it?

We had captured all information in patch 5/5 patch in the documentation part.  
We could always improve the documentation. Please let us know

> > >
> > > I don't mean to be so harsh here, but come on people, this stuff
> > > needs a lot more background documentation, information, and
> > > explaination as to exactly why in the world we need any of this, and what it
> even does!
> > >
> > > Also, you need to fix the code, it doesn't work as pointed out in a
> > > few places :)
> > >
> >
> > Thanks for going through this.
> > It's now clear this is far from ready.
> > I need to re-evaluate my position on this, not just the code and the
> > documentation, but the usefulness of it all.
> 
> What is this even supposed to be used for?  What is the application for it?  I
> couldn't determine that at all, what am I missing?

A typical use case is ADB for x86 Android systems  or similar user space class(debug) drivers that can leverage xHCI.DbC capability for debug purpose.

The larger goal here is to have DbC as a unified debug infrastructure for different debug methods like KGDB or early printk and leverage the benefits of a dedicated debug infra (DbC) brings in. 

> 
> thanks,
> 
> greg k-h

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Greg KH]

On Tue, Jun 11, 2019 at 09:29:23AM +0000, Regupathy, Rajaram wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Monday, June 10, 2019 7:46 PM
> > To: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Cc: Pandey, Prabhat Chand <prabhat.chand.pandey@intel.com>; linux-
> > usb@vger.kernel.org; Nyman, Mathias <mathias.nyman@intel.com>;
> > Regupathy, Rajaram <rajaram.regupathy@intel.com>; K V, Abhilash
> > <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com>
> > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> > interface on DbC
> > Importance: High
> > 
> > On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> > > On 7.6.2019 17.21, Greg KH wrote:
> > > > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
> > > > > From: Abhilash K V <abhilash.k.v@intel.com>
> > > > >
> > > > > This patch provides a raw device interface on xhci Debug capability.
> > > >
> > > > What is a "raw device"?
> > > >
> > > > > This abstracts dbc functionality to user space inorder to
> > > > > facilitate various frameworks to utilize xhci debug capability.
> > > >
> > > > I do not understand this sentance at all.  Please provide a lot more
> > > > information.
> > > >
> > > > > It helps to render the target as an usb debug class device on host
> > > > > and establish an usb connection by providing two bulk endpoints.
> > > >
> > > > provide bulk endpoints where?  To send data where?  This is very
> > > > confusing and does not make any sense to me...
> > > >
> > > >
> > > > >
> > > > > [don't dynamically allocate tiny space for name only -Mathias]
> > > > > Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> > > > > Signed-off-by: Prabhat Chand Pandey
> > > > > <prabhat.chand.pandey@intel.com>
> > > > > Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> > > > > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > > ---
> > > ...
> > > >
> > > > So you have a new char device, with a undocumented and unknown
> > > > format of data flowing across it to the device.  How in the world
> > > > are we supposed to use this thing?  Where is it documented?  What
> > > > does it do?  How can you use it?
> 
> We had captured all information in patch 5/5 patch in the documentation part.  
> We could always improve the documentation. Please let us know

The documentation needs work, see my comments on that.

Also, I don't think you answered these basic questions there, like "what
is the data format", and "how is this supposed to be used".

> > > > I don't mean to be so harsh here, but come on people, this stuff
> > > > needs a lot more background documentation, information, and
> > > > explaination as to exactly why in the world we need any of this, and what it
> > even does!
> > > >
> > > > Also, you need to fix the code, it doesn't work as pointed out in a
> > > > few places :)
> > > >
> > >
> > > Thanks for going through this.
> > > It's now clear this is far from ready.
> > > I need to re-evaluate my position on this, not just the code and the
> > > documentation, but the usefulness of it all.
> > 
> > What is this even supposed to be used for?  What is the application for it?  I
> > couldn't determine that at all, what am I missing?
> 
> A typical use case is ADB for x86 Android systems  or similar user
> space class(debug) drivers that can leverage xHCI.DbC capability for
> debug purpose.

Why does adb need a "high speed" interface?

And do you need special hardware to access this?  Do you need patches on
the adb side for this?

> The larger goal here is to have DbC as a unified debug infrastructure for different debug methods like KGDB or early printk and leverage the benefits of a dedicated debug infra (DbC) brings in. 

Have you modified kgdb for this?  Do you have patches for that?

Who can use this interface in the "real world", is it only developers
that have access to the special hardware dongle?  Or can anyone use this
on their laptops for getting console access in a way that is somehow
"better" than the existing interface?

And just how much "faster" is all of this than the current tty
interface?  What is lacking in the tty interface today that you need
this new, special one?  Can you just not fix any bottleneck in the tty
driver if you are not properly saturating the bus?

thanks,

greg k-h

RE: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Regupathy, Rajaram]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, June 11, 2019 3:23 PM
> To: Regupathy, Rajaram <rajaram.regupathy@intel.com>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>; Pandey, Prabhat Chand
> <prabhat.chand.pandey@intel.com>; linux-usb@vger.kernel.org; Nyman,
> Mathias <mathias.nyman@intel.com>; K V, Abhilash <abhilash.k.v@intel.com>;
> Balaji, M <m.balaji@intel.com>
> Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> interface on DbC
> Importance: High
> 
> On Tue, Jun 11, 2019 at 09:29:23AM +0000, Regupathy, Rajaram wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Monday, June 10, 2019 7:46 PM
> > > To: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > Cc: Pandey, Prabhat Chand <prabhat.chand.pandey@intel.com>; linux-
> > > usb@vger.kernel.org; Nyman, Mathias <mathias.nyman@intel.com>;
> > > Regupathy, Rajaram <rajaram.regupathy@intel.com>; K V, Abhilash
> > > <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com>
> > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to
> > > provide a raw interface on DbC
> > > Importance: High
> > >
> > > On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> > > > On 7.6.2019 17.21, Greg KH wrote:
> > > > > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
> > > > > > From: Abhilash K V <abhilash.k.v@intel.com>
> > > > > >
> > > > > > This patch provides a raw device interface on xhci Debug capability.
> > > > >
> > > > > What is a "raw device"?
> > > > >
> > > > > > This abstracts dbc functionality to user space inorder to
> > > > > > facilitate various frameworks to utilize xhci debug capability.
> > > > >
> > > > > I do not understand this sentance at all.  Please provide a lot
> > > > > more information.
> > > > >
> > > > > > It helps to render the target as an usb debug class device on
> > > > > > host and establish an usb connection by providing two bulk endpoints.
> > > > >
> > > > > provide bulk endpoints where?  To send data where?  This is very
> > > > > confusing and does not make any sense to me...
> > > > >
> > > > >
> > > > > >
> > > > > > [don't dynamically allocate tiny space for name only -Mathias]
> > > > > > Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> > > > > > Signed-off-by: Prabhat Chand Pandey
> > > > > > <prabhat.chand.pandey@intel.com>
> > > > > > Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> > > > > > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > > > ---
> > > > ...
> > > > >
> > > > > So you have a new char device, with a undocumented and unknown
> > > > > format of data flowing across it to the device.  How in the
> > > > > world are we supposed to use this thing?  Where is it
> > > > > documented?  What does it do?  How can you use it?
> >
> > We had captured all information in patch 5/5 patch in the documentation
> part.
> > We could always improve the documentation. Please let us know
> 
> The documentation needs work, see my comments on that.
> 
> Also, I don't think you answered these basic questions there, like "what is the
> data format", and "how is this supposed to be used".

Sure. dbc_raw("dbcfs" could have been better) is a an interface similar to gadget(ffs) or host (usbfs) drivers of USB. These "*fs" provides user land to develop class drivers.  Thus the transport is agnostic on the data format, where we could implement an MTP class (ffs,usbfs) or ADB(dbcfs/dbc_raw).

But I agree we need to provide additional documentation similar to Documentation/usb/proc_usb_info.txt or Documentation/usb/functionfs.txt

> 
> > > > > I don't mean to be so harsh here, but come on people, this stuff
> > > > > needs a lot more background documentation, information, and
> > > > > explaination as to exactly why in the world we need any of this,
> > > > > and what it
> > > even does!
> > > > >
> > > > > Also, you need to fix the code, it doesn't work as pointed out
> > > > > in a few places :)
> > > > >
> > > >
> > > > Thanks for going through this.
> > > > It's now clear this is far from ready.
> > > > I need to re-evaluate my position on this, not just the code and
> > > > the documentation, but the usefulness of it all.
> > >
> > > What is this even supposed to be used for?  What is the application
> > > for it?  I couldn't determine that at all, what am I missing?
> >
> > A typical use case is ADB for x86 Android systems  or similar user
> > space class(debug) drivers that can leverage xHCI.DbC capability for
> > debug purpose.
> 
> Why does adb need a "high speed" interface?

Debug tools require high/super speed when large logs or test files have to be pushed or pulled from the device under test. ADB is one such tool.

> 
> And do you need special hardware to access this?  Do you need patches on the
> adb side for this?

No special hardware is required. We need a USB Type-AtoA debug cable and is a commonly used method in other OS tools as well .  Yes adb requires changes only in enumeration path to match the descriptors.

> 
> > The larger goal here is to have DbC as a unified debug infrastructure for
> different debug methods like KGDB or early printk and leverage the benefits of a
> dedicated debug infra (DbC) brings in.
> 
> Have you modified kgdb for this?  Do you have patches for that?
No kgdb changes. For kgdb to work we added necessary wrapper functions required on the dbc_tty interface which already part of kernel. We have functionally verified and shall push the patch subsequently. Am I missing something ?
> 
> Who can use this interface in the "real world", is it only developers that have
> access to the special hardware dongle?  Or can anyone use this on their laptops
> for getting console access in a way that is somehow "better" than the existing
> interface?

No special hardware is required. As indicated earlier developers need a USB A-A debug cable and anyone can use it to get console access. 
Yes it is better that existing usb-serial converters with each having proprietary drivers . This is a plug and play solution providing super speed interface.

> 
> And just how much "faster" is all of this than the current tty interface?  What is
> lacking in the tty interface today that you need this new, special one?  Can you
> just not fix any bottleneck in the tty driver if you are not properly saturating the
> bus?

IMHO, tty is a legacy interface designed for the purpose it serves for. Modern High speed IO will not fit into tty framework and refactoring it will not bring any real value.  We have captured the initial performance numbers in the documentation patch 5/5.  Please correct me if I am missing something 

> 
> thanks,
> 
> greg k-h

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Greg KH]

On Tue, Jun 11, 2019 at 12:17:40PM +0000, Regupathy, Rajaram wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, June 11, 2019 3:23 PM
> > To: Regupathy, Rajaram <rajaram.regupathy@intel.com>
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>; Pandey, Prabhat Chand
> > <prabhat.chand.pandey@intel.com>; linux-usb@vger.kernel.org; Nyman,
> > Mathias <mathias.nyman@intel.com>; K V, Abhilash <abhilash.k.v@intel.com>;
> > Balaji, M <m.balaji@intel.com>
> > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> > interface on DbC
> > Importance: High
> > 
> > On Tue, Jun 11, 2019 at 09:29:23AM +0000, Regupathy, Rajaram wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Monday, June 10, 2019 7:46 PM
> > > > To: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > Cc: Pandey, Prabhat Chand <prabhat.chand.pandey@intel.com>; linux-
> > > > usb@vger.kernel.org; Nyman, Mathias <mathias.nyman@intel.com>;
> > > > Regupathy, Rajaram <rajaram.regupathy@intel.com>; K V, Abhilash
> > > > <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com>
> > > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to
> > > > provide a raw interface on DbC
> > > > Importance: High
> > > >
> > > > On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> > > > > On 7.6.2019 17.21, Greg KH wrote:
> > > > > > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey wrote:
> > > > > > > From: Abhilash K V <abhilash.k.v@intel.com>
> > > > > > >
> > > > > > > This patch provides a raw device interface on xhci Debug capability.
> > > > > >
> > > > > > What is a "raw device"?
> > > > > >
> > > > > > > This abstracts dbc functionality to user space inorder to
> > > > > > > facilitate various frameworks to utilize xhci debug capability.
> > > > > >
> > > > > > I do not understand this sentance at all.  Please provide a lot
> > > > > > more information.
> > > > > >
> > > > > > > It helps to render the target as an usb debug class device on
> > > > > > > host and establish an usb connection by providing two bulk endpoints.
> > > > > >
> > > > > > provide bulk endpoints where?  To send data where?  This is very
> > > > > > confusing and does not make any sense to me...
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > [don't dynamically allocate tiny space for name only -Mathias]
> > > > > > > Signed-off-by: Rajaram Regupathy <rajaram.regupathy@intel.com>
> > > > > > > Signed-off-by: Prabhat Chand Pandey
> > > > > > > <prabhat.chand.pandey@intel.com>
> > > > > > > Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> > > > > > > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > > > > ---
> > > > > ...
> > > > > >
> > > > > > So you have a new char device, with a undocumented and unknown
> > > > > > format of data flowing across it to the device.  How in the
> > > > > > world are we supposed to use this thing?  Where is it
> > > > > > documented?  What does it do?  How can you use it?
> > >
> > > We had captured all information in patch 5/5 patch in the documentation
> > part.
> > > We could always improve the documentation. Please let us know
> > 
> > The documentation needs work, see my comments on that.
> > 
> > Also, I don't think you answered these basic questions there, like "what is the
> > data format", and "how is this supposed to be used".
> 
> Sure. dbc_raw("dbcfs" could have been better) is a an interface similar to gadget(ffs) or host (usbfs) drivers of USB. These "*fs" provides user land to develop class drivers.  Thus the transport is agnostic on the data format, where we could implement an MTP class (ffs,usbfs) or ADB(dbcfs/dbc_raw).

Please wrap your lines at 72 columns...

Anyway, this is not a filesystem interface, you have created a char
device.  Much like the tty char device node you have today, right?  Just
seems to use different ioctls, and requires a custom userspace program.

> But I agree we need to provide additional documentation similar to Documentation/usb/proc_usb_info.txt or Documentation/usb/functionfs.txt

You need some sort of documentation :)

> > > > > > I don't mean to be so harsh here, but come on people, this stuff
> > > > > > needs a lot more background documentation, information, and
> > > > > > explaination as to exactly why in the world we need any of this,
> > > > > > and what it
> > > > even does!
> > > > > >
> > > > > > Also, you need to fix the code, it doesn't work as pointed out
> > > > > > in a few places :)
> > > > > >
> > > > >
> > > > > Thanks for going through this.
> > > > > It's now clear this is far from ready.
> > > > > I need to re-evaluate my position on this, not just the code and
> > > > > the documentation, but the usefulness of it all.
> > > >
> > > > What is this even supposed to be used for?  What is the application
> > > > for it?  I couldn't determine that at all, what am I missing?
> > >
> > > A typical use case is ADB for x86 Android systems  or similar user
> > > space class(debug) drivers that can leverage xHCI.DbC capability for
> > > debug purpose.
> > 
> > Why does adb need a "high speed" interface?
> 
> Debug tools require high/super speed when large logs or test files have to be pushed or pulled from the device under test. ADB is one such tool.

And what is lacking with the tty interface you have today?  How can this
be "faster" when it is talking to the same exact hardware?  Where is the
overhead in the tty api that is now gone here?

> > And do you need special hardware to access this?  Do you need patches on the
> > adb side for this?
> 
> No special hardware is required. We need a USB Type-AtoA debug cable and is a commonly used method in other OS tools as well .  Yes adb requires changes only in enumeration path to match the descriptors.

Do you have a link to such cables?  I don't seem to have one anywhere...

> > > The larger goal here is to have DbC as a unified debug infrastructure for
> > different debug methods like KGDB or early printk and leverage the benefits of a
> > dedicated debug infra (DbC) brings in.
> > 
> > Have you modified kgdb for this?  Do you have patches for that?
> No kgdb changes. For kgdb to work we added necessary wrapper functions required on the dbc_tty interface which already part of kernel. We have functionally verified and shall push the patch subsequently. Am I missing something ?

You are missing the justification of a custom api that requires all
userspace tools to be modified instead of using the existing tty api
that everything "just works" with today.

> > Who can use this interface in the "real world", is it only developers that have
> > access to the special hardware dongle?  Or can anyone use this on their laptops
> > for getting console access in a way that is somehow "better" than the existing
> > interface?
> 
> No special hardware is required. As indicated earlier developers need a USB A-A debug cable and anyone can use it to get console access. 

Where can I get one of those?

> Yes it is better that existing usb-serial converters with each having proprietary drivers . This is a plug and play solution providing super speed interface.

I don't understand, what is "proprietary" about the existing tty api?
It's a generic tty device node, right?  What am I missing?

> > And just how much "faster" is all of this than the current tty interface?  What is
> > lacking in the tty interface today that you need this new, special one?  Can you
> > just not fix any bottleneck in the tty driver if you are not properly saturating the
> > bus?
> 
> IMHO, tty is a legacy interface designed for the purpose it serves for. Modern High speed IO will not fit into tty framework and refactoring it will not bring any real value.  We have captured the initial performance numbers in the documentation patch 5/5.  Please correct me if I am missing something 

Why will "modern high speed IO" not fit into the tty framework?  Where
is the bottleneck?  We have tty devices that seem to run at "line speed"
on a firewire connection today, and that should be faster than whatever
this host controller can pump out, right?

My main objection here is a lack of justification to require userspace
to write to a new, unknown and undocumented interface, because of an
unknown speed issue in the existing codebase.

Would you take patches submitted in such a way if you were in my place?

thanks,

greg k-h

RE: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Regupathy, Rajaram]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, June 11, 2019 6:04 PM
> To: Regupathy, Rajaram <rajaram.regupathy@intel.com>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>; Pandey, Prabhat Chand
> <prabhat.chand.pandey@intel.com>; linux-usb@vger.kernel.org; Nyman,
> Mathias <mathias.nyman@intel.com>; K V, Abhilash <abhilash.k.v@intel.com>;
> Balaji, M <m.balaji@intel.com>
> Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> interface on DbC
> Importance: High
> 
> On Tue, Jun 11, 2019 at 12:17:40PM +0000, Regupathy, Rajaram wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Tuesday, June 11, 2019 3:23 PM
> > > To: Regupathy, Rajaram <rajaram.regupathy@intel.com>
> > > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>; Pandey, Prabhat
> > > Chand <prabhat.chand.pandey@intel.com>; linux-usb@vger.kernel.org;
> > > Nyman, Mathias <mathias.nyman@intel.com>; K V, Abhilash
> > > <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com>
> > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to
> > > provide a raw interface on DbC
> > > Importance: High
> > >
> > > On Tue, Jun 11, 2019 at 09:29:23AM +0000, Regupathy, Rajaram wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > > Sent: Monday, June 10, 2019 7:46 PM
> > > > > To: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > > Cc: Pandey, Prabhat Chand <prabhat.chand.pandey@intel.com>;
> > > > > linux- usb@vger.kernel.org; Nyman, Mathias
> > > > > <mathias.nyman@intel.com>; Regupathy, Rajaram
> > > > > <rajaram.regupathy@intel.com>; K V, Abhilash
> > > > > <abhilash.k.v@intel.com>; Balaji, M <m.balaji@intel.com>
> > > > > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to
> > > > > provide a raw interface on DbC
> > > > > Importance: High
> > > > >
> > > > > On Mon, Jun 10, 2019 at 04:53:51PM +0300, Mathias Nyman wrote:
> > > > > > On 7.6.2019 17.21, Greg KH wrote:
> > > > > > > On Fri, Jun 07, 2019 at 12:03:05PM +0530, Prabhat Chand Pandey
> wrote:
> > > > > > > > From: Abhilash K V <abhilash.k.v@intel.com>
> > > > > > > >
> > > > > > > > This patch provides a raw device interface on xhci Debug capability.
> > > > > > >
> > > > > > > What is a "raw device"?
> > > > > > >
> > > > > > > > This abstracts dbc functionality to user space inorder to
> > > > > > > > facilitate various frameworks to utilize xhci debug capability.
> > > > > > >
> > > > > > > I do not understand this sentance at all.  Please provide a
> > > > > > > lot more information.
> > > > > > >
> > > > > > > > It helps to render the target as an usb debug class device
> > > > > > > > on host and establish an usb connection by providing two bulk
> endpoints.
> > > > > > >
> > > > > > > provide bulk endpoints where?  To send data where?  This is
> > > > > > > very confusing and does not make any sense to me...
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > [don't dynamically allocate tiny space for name only
> > > > > > > > -Mathias]
> > > > > > > > Signed-off-by: Rajaram Regupathy
> > > > > > > > <rajaram.regupathy@intel.com>
> > > > > > > > Signed-off-by: Prabhat Chand Pandey
> > > > > > > > <prabhat.chand.pandey@intel.com>
> > > > > > > > Signed-off-by: Abhilash K V <abhilash.k.v@intel.com>
> > > > > > > > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > > > > > > ---
> > > > > > ...
> > > > > > >
> > > > > > > So you have a new char device, with a undocumented and
> > > > > > > unknown format of data flowing across it to the device.  How
> > > > > > > in the world are we supposed to use this thing?  Where is it
> > > > > > > documented?  What does it do?  How can you use it?
> > > >
> > > > We had captured all information in patch 5/5 patch in the
> > > > documentation
> > > part.
> > > > We could always improve the documentation. Please let us know
> > >
> > > The documentation needs work, see my comments on that.
> > >
> > > Also, I don't think you answered these basic questions there, like
> > > "what is the data format", and "how is this supposed to be used".
> >
> > Sure. dbc_raw("dbcfs" could have been better) is a an interface similar to
> gadget(ffs) or host (usbfs) drivers of USB. These "*fs" provides user land to
> develop class drivers.  Thus the transport is agnostic on the data format, where
> we could implement an MTP class (ffs,usbfs) or ADB(dbcfs/dbc_raw).
> 
> Please wrap your lines at 72 columns...
> 
> Anyway, this is not a filesystem interface, you have created a char device.
> Much like the tty char device node you have today, right?  Just seems to use
> different ioctls, and requires a custom userspace program.
> 
> > But I agree we need to provide additional documentation similar to
> > Documentation/usb/proc_usb_info.txt or
> > Documentation/usb/functionfs.txt
> 
> You need some sort of documentation :)
> 
> > > > > > > I don't mean to be so harsh here, but come on people, this
> > > > > > > stuff needs a lot more background documentation,
> > > > > > > information, and explaination as to exactly why in the world
> > > > > > > we need any of this, and what it
> > > > > even does!
> > > > > > >
> > > > > > > Also, you need to fix the code, it doesn't work as pointed
> > > > > > > out in a few places :)
> > > > > > >
> > > > > >
> > > > > > Thanks for going through this.
> > > > > > It's now clear this is far from ready.
> > > > > > I need to re-evaluate my position on this, not just the code
> > > > > > and the documentation, but the usefulness of it all.
> > > > >
> > > > > What is this even supposed to be used for?  What is the
> > > > > application for it?  I couldn't determine that at all, what am I missing?
> > > >
> > > > A typical use case is ADB for x86 Android systems  or similar user
> > > > space class(debug) drivers that can leverage xHCI.DbC capability
> > > > for debug purpose.
> > >
> > > Why does adb need a "high speed" interface?
> >
> > Debug tools require high/super speed when large logs or test files have to be
> pushed or pulled from the device under test. ADB is one such tool.
> 
> And what is lacking with the tty interface you have today?  How can this be
> "faster" when it is talking to the same exact hardware?  Where is the overhead
> in the tty api that is now gone here?
> 
> > > And do you need special hardware to access this?  Do you need
> > > patches on the adb side for this?
> >
> > No special hardware is required. We need a USB Type-AtoA debug cable and is
> a commonly used method in other OS tools as well .  Yes adb requires changes
> only in enumeration path to match the descriptors.
> 
> Do you have a link to such cables?  I don't seem to have one anywhere...
> 
> > > > The larger goal here is to have DbC as a unified debug
> > > > infrastructure for
> > > different debug methods like KGDB or early printk and leverage the
> > > benefits of a dedicated debug infra (DbC) brings in.
> > >
> > > Have you modified kgdb for this?  Do you have patches for that?
> > No kgdb changes. For kgdb to work we added necessary wrapper functions
> required on the dbc_tty interface which already part of kernel. We have
> functionally verified and shall push the patch subsequently. Am I missing
> something ?
> 
> You are missing the justification of a custom api that requires all userspace
> tools to be modified instead of using the existing tty api that everything "just
> works" with today.

I was referring to the poll methods required for KGDB/GDB to work which is missing in dbc_tty driver in the kernel.

> 
> > > Who can use this interface in the "real world", is it only
> > > developers that have access to the special hardware dongle?  Or can
> > > anyone use this on their laptops for getting console access in a way
> > > that is somehow "better" than the existing interface?
> >
> > No special hardware is required. As indicated earlier developers need a USB A-
> A debug cable and anyone can use it to get console access.
> 
> Where can I get one of those?
Here is one example:  https://www.amazon.com/SIIG-SuperSpeed-Cable-Meters-CB-US0212-S1/dp/B0032ANCBO

> 
> > Yes it is better that existing usb-serial converters with each having proprietary
> drivers . This is a plug and play solution providing super speed interface.
> 
> I don't understand, what is "proprietary" about the existing tty api?
> It's a generic tty device node, right?  What am I missing?

I am referring to usb-serial controller drivers as in "drivers/usb/serial/Kconfig" which has vendors configs leading to much of kernel maintenance.  DbC driver would provide necessary functionality without any of those.

> 
> > > And just how much "faster" is all of this than the current tty
> > > interface?  What is lacking in the tty interface today that you need
> > > this new, special one?  Can you just not fix any bottleneck in the
> > > tty driver if you are not properly saturating the bus?
> >
> > IMHO, tty is a legacy interface designed for the purpose it serves
> > for. Modern High speed IO will not fit into tty framework and
> > refactoring it will not bring any real value.  We have captured the
> > initial performance numbers in the documentation patch 5/5.  Please
> > correct me if I am missing something
> 
> Why will "modern high speed IO" not fit into the tty framework?  Where is the
> bottleneck?  We have tty devices that seem to run at "line speed"
> on a firewire connection today, and that should be faster than whatever this
> host controller can pump out, right?

Though I am not aware of the design thoughts behind firewire-tty driver which is in staging, I see
the to do list and git logs indicate buffer over flow issues indicating the tty framework cannot handle high speed IO. 

Thus our rationale of why tty may not serve the purpose as indicated below gets ratified 
- Performance & stability ( multiple layers, >1GB file copy CRC errors)
- Error Recovery  ( lack of framework to propagate transport error handling to the "real" class drivers)

> 
> My main objection here is a lack of justification to require userspace to write to
> a new, unknown and undocumented interface, because of an unknown speed
> issue in the existing codebase.

As for us clarity on unknown/undocumented, ADB is adequately documented and known tool. Please note this is just one example and the interface can be leveraged by other debug tools for better performance..  Having a thinner kernel implementation has well known advantage including stability and maintenance and is not new to USB drivers

> Would you take patches submitted in such a way if you were in my place?
I would be happy to address all your concerns and open to adopt any better approach that solves the problem .

> 
> thanks,
> 
> greg k-h

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Greg KH]

On Wed, Jun 12, 2019 at 08:49:11AM +0000, Regupathy, Rajaram wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, June 11, 2019 6:04 PM
> > To: Regupathy, Rajaram <rajaram.regupathy@intel.com>
> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com>; Pandey, Prabhat Chand
> > <prabhat.chand.pandey@intel.com>; linux-usb@vger.kernel.org; Nyman,
> > Mathias <mathias.nyman@intel.com>; K V, Abhilash <abhilash.k.v@intel.com>;
> > Balaji, M <m.balaji@intel.com>
> > Subject: Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw
> > interface on DbC
> > Importance: High

Please fix your quoting logic, this is never needed in an email.

And I don't find these emails of "High" importance :)

> > > > > The larger goal here is to have DbC as a unified debug
> > > > > infrastructure for
> > > > different debug methods like KGDB or early printk and leverage the
> > > > benefits of a dedicated debug infra (DbC) brings in.
> > > >
> > > > Have you modified kgdb for this?  Do you have patches for that?
> > > No kgdb changes. For kgdb to work we added necessary wrapper functions
> > required on the dbc_tty interface which already part of kernel. We have
> > functionally verified and shall push the patch subsequently. Am I missing
> > something ?
> > 
> > You are missing the justification of a custom api that requires all userspace
> > tools to be modified instead of using the existing tty api that everything "just
> > works" with today.
> 
> I was referring to the poll methods required for KGDB/GDB to work which is missing in dbc_tty driver in the kernel.

Again, fix your line-wrapping :(

Anyway, just poing kgdb at the different tty device and it should "just
work", right?  No need to change anything that I can see.  If not,
please send patches to get that working so people can use that today.

> > > > Who can use this interface in the "real world", is it only
> > > > developers that have access to the special hardware dongle?  Or can
> > > > anyone use this on their laptops for getting console access in a way
> > > > that is somehow "better" than the existing interface?
> > >
> > > No special hardware is required. As indicated earlier developers need a USB A-
> > A debug cable and anyone can use it to get console access.
> > 
> > Where can I get one of those?
> Here is one example:  https://www.amazon.com/SIIG-SuperSpeed-Cable-Meters-CB-US0212-S1/dp/B0032ANCBO

Ah, nice!  I'll try to see if I can get that in my country...

Nope, not available in Europe from what I can tell, I'll have to wait
until the next time I'm in the US :(

> > > Yes it is better that existing usb-serial converters with each having proprietary
> > drivers . This is a plug and play solution providing super speed interface.
> > 
> > I don't understand, what is "proprietary" about the existing tty api?
> > It's a generic tty device node, right?  What am I missing?
> 
> I am referring to usb-serial controller drivers as in "drivers/usb/serial/Kconfig" which has vendors configs leading to much of kernel maintenance.  DbC driver would provide necessary functionality without any of those.

Again, line-wrapping...

I have no idea what you are referring to "leading to much of kernel
maintenance."  You don't need to include _all_ usb-serial drivers, just
the usb-serial core and the specific driver.  No different from any
other serial port device, which Linux has supported for decades now.

Use the functionality we have already, do not create duplicate code just
because you want to, that's a sure way to get your patches rejected.

And this _is_ a serial connection to the other device, why pretend that
it is not?  Again, what is the reason why you can not use what you have
today?

> > > > And just how much "faster" is all of this than the current tty
> > > > interface?  What is lacking in the tty interface today that you need
> > > > this new, special one?  Can you just not fix any bottleneck in the
> > > > tty driver if you are not properly saturating the bus?
> > >
> > > IMHO, tty is a legacy interface designed for the purpose it serves
> > > for. Modern High speed IO will not fit into tty framework and
> > > refactoring it will not bring any real value.  We have captured the
> > > initial performance numbers in the documentation patch 5/5.  Please
> > > correct me if I am missing something
> > 
> > Why will "modern high speed IO" not fit into the tty framework?  Where is the
> > bottleneck?  We have tty devices that seem to run at "line speed"
> > on a firewire connection today, and that should be faster than whatever this
> > host controller can pump out, right?
> 
> Though I am not aware of the design thoughts behind firewire-tty driver which is in staging, I see
> the to do list and git logs indicate buffer over flow issues indicating the tty framework cannot handle high speed IO. 

That is that one driver, not yours.

> Thus our rationale of why tty may not serve the purpose as indicated below gets ratified 
> - Performance & stability ( multiple layers, >1GB file copy CRC errors)
> - Error Recovery  ( lack of framework to propagate transport error handling to the "real" class drivers)

Do you have these problems today with the existing kernel driver?  If
so, please let us know and fix them!  Don't create a whole new thing
just because you don't like the tty layer, otherwise you will be forced
to end up creating all of the same logic again.

> > My main objection here is a lack of justification to require userspace to write to
> > a new, unknown and undocumented interface, because of an unknown speed
> > issue in the existing codebase.
> 
> As for us clarity on unknown/undocumented, ADB is adequately documented and known tool. Please note this is just one example and the interface can be leveraged by other debug tools for better performance..  Having a thinner kernel implementation has well known advantage including stability and maintenance and is not new to USB drivers

What "thinner" kernel implementation?  Nothing is as "thin" as the code
that is already written and is not going away :)

Again, do not duplicate functionality for no good reason, and I have yet
to see a definitive reason why the current in-kernel code is not
acceptable.

> > Would you take patches submitted in such a way if you were in my place?
> I would be happy to address all your concerns and open to adopt any better approach that solves the problem .

That was not the answer to my question.

greg k-h

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]


Hi,

Greg KH <gregkh@linuxfoundation.org> writes:
>> > > > Who can use this interface in the "real world", is it only
>> > > > developers that have access to the special hardware dongle?  Or can
>> > > > anyone use this on their laptops for getting console access in a way
>> > > > that is somehow "better" than the existing interface?
>> > >
>> > > No special hardware is required. As indicated earlier developers need a USB A-
>> > A debug cable and anyone can use it to get console access.
>> > 
>> > Where can I get one of those?
>> Here is one example:  https://www.amazon.com/SIIG-SuperSpeed-Cable-Meters-CB-US0212-S1/dp/B0032ANCBO
>
> Ah, nice!  I'll try to see if I can get that in my country...
>
> Nope, not available in Europe from what I can tell, I'll have to wait
> until the next time I'm in the US :(

here's one from amazon.de:

https://www.amazon.de/dp/B00WHZ6VEU/

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/5] usb: xhci: dbc: Add a dbc raw driver to provide a raw interface on DbC

[Greg KH]

On Thu, Jun 13, 2019 at 03:33:02PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH <gregkh@linuxfoundation.org> writes:
> >> > > > Who can use this interface in the "real world", is it only
> >> > > > developers that have access to the special hardware dongle?  Or can
> >> > > > anyone use this on their laptops for getting console access in a way
> >> > > > that is somehow "better" than the existing interface?
> >> > >
> >> > > No special hardware is required. As indicated earlier developers need a USB A-
> >> > A debug cable and anyone can use it to get console access.
> >> > 
> >> > Where can I get one of those?
> >> Here is one example:  https://www.amazon.com/SIIG-SuperSpeed-Cable-Meters-CB-US0212-S1/dp/B0032ANCBO
> >
> > Ah, nice!  I'll try to see if I can get that in my country...
> >
> > Nope, not available in Europe from what I can tell, I'll have to wait
> > until the next time I'm in the US :(
> 
> here's one from amazon.de:
> 
> https://www.amazon.de/dp/B00WHZ6VEU/

Dieser Artikel kann nicht in die Niederlande geliefert werden.

:(

[PATCH 5/5] usb: xhci: dbc: Document describe about dbc raw interface

[Prabhat Chand Pandey]

This patch have explaination about the new DBC interface called
dbc raw interface. This cover the capability, target setup and
use case info.

Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
---
 Documentation/usb/dbc_raw.rst | 136 ++++++++++++++++++++++++++++++++++
 Documentation/usb/index.rst   |  16 ++++
 2 files changed, 152 insertions(+)
 create mode 100644 Documentation/usb/dbc_raw.rst
 create mode 100644 Documentation/usb/index.rst

diff --git a/Documentation/usb/dbc_raw.rst b/Documentation/usb/dbc_raw.rst
new file mode 100644
index 000000000000..b7d472a478f5
--- /dev/null
+++ b/Documentation/usb/dbc_raw.rst
@@ -0,0 +1,136 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+======================================
+This described about DBC RAW Interface
+======================================
+
+:Author: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
+
+Content
+========
+
+- DBC Overview
+- Motivation
+- DBC RAW Capabilities
+- Target Build Setup
+- Target Test Setup
+- Host Target Connection
+- Experiment Test Result
+- DBC TTY Use Cases
+- DBC RAW Use Cases
+- Conclusion
+
+DBC Overview
+-------------
+xDBC stands for the USB Debug capability provided extensible Host Controller
+Interface. Universal Serial Bus is a host controlled Bus. Host Controller is
+a hardware whose functionality is to manage usb bus and usb host ports. It is
+responsible for initiating and managing all usb transfers. Extensible Host
+Controller Interface (xHCI) is a register-level interface which provides a
+mechanism by which the host controller (xHC) can communicate with the Operating
+System of the host computer. In addition to exposing register interfaces
+essential for proper functioning of the xHC it also supports many extended
+capabilities which can optionally be implemented by xHC.
+
+It includes Extended Power Management Capability, I/O Virtualization capability
+USB Legacy support capability among many others. USB Debug Capability is one of
+the main extended capabilities supported by xHCI.
+
+This functionality enables low-level system debug over USB. The xHCI debug
+capabilities (xDBC) provides a means of connecting two systems where the system
+is a Debug Host and the other is a Debug target. This is achieved through
+emulating a debug device by using xDBC on the debug target. The debug device
+presented by the debug target can be used by debug host for low level system
+debugging of target.
+
+Motivation
+-----------
+In this patch-set we learn the requirement of new DBC interface called DBC RAW,
+which can be used for platform debugging, large data transfer with comparatively
+10x faster data rate than DBC TTY and it have all the USB specific error handling
+mechanism enabled which DBC TTY doesn't have.
+
+DBC RAW Capabilities
+---------------------
+* Current transfer rate of up to 25.4 MB/s from host to target (using Blocking APIs).
+* Current transfer rate of up to 28.8 MB/s from target to host (using Blocking APIs).
+* Have further scope of improvement in transfer rate of up to USB 3.x speed.
+* This interface can bind with multiple target xHCI (i.e. /dev/dbc_raw0, raw1 etc).
+* It helps to render the target as an usb debug class device on host and establish
+  an usb connection by providing two bulk endpoints.
+* It has support to handle halt bit and send STALL packet.
+
+Target Build Setup
+-------------------
+* Make sure to use Kernel of version >= 4.13.
+* Enable USB_XHCI_DBGCAP in Kernel menuconfig.
+	Device Drivers  ---> USB support  ---> xHCI support for debug capability [Y]
+* Enable DBC RAW in kernel menuconfig.
+	Device Drivers  ---> USB support  ---> Select function for debug capability
+							(xHCI DbC raw driver support)
+* Build Image.
+
+Target Test Setup
+------------------
+* Flash binary with above changes in the target board.
+* Cross check the current DBC function interface, it should be RAW in this case
+  otherwise TTY, below is the sysfs entry for the dbc_function (it's a read only file,
+  switching b/w RAW and TTY is only possible at build time):
+	root@target:/ # cat /sys/bus/pci/devices/0000\:00\:14.0/dbc_function
+	RWA
+* Enable DBC with the following command:
+	root@target:/ # echo enable > /sys/bus/pci/devices/0000\:00\:14.0/dbc
+
+Host Target Connection
+-----------------------
+* Connect Type A to Type A cable between Target and Host.
+* Make sure it is an usb 3.0 cable.
+* At this point debug device should be enumerated on Host side.
+* On Target side dbc sysfs attribute should change to configured state and
+  dbc_raw0 character device should appear under dev directory.
+
+	On Target-:
+		root@target:/ # cat /sys/bus/pci/devices/0000\:00\:14.0/dbc
+		configured
+
+	Target dmesg-:
+		[   32.420018] xhci_hcd 0000:00:14.0: DbC connected
+		[   32.676014] xhci_hcd 0000:00:14.0: DbC configured
+
+	Host dmesg-:
+		[3613626.064257] usb 2-1: new SuperSpeed USB device number 46 using xhci_hcd
+		[3613626.084391] usb 2-1: LPM exit latency is zeroed, disabling LPM.
+		[3613626.084642] usb 2-1: New USB device found, idVendor=1d6b, idProduct=0010
+		[3613626.084647] usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
+		[3613626.084651] usb 2-1: Product: Linux USB Debug Target
+		[3613626.084655] usb 2-1: Manufacturer: Linux Foundation
+		[3613626.084658] usb 2-1: SerialNumber: 0001
+
+Experiment Test Result
+-----------------------
+* Transferred 32MB data from host to target with packet size 16KB using blocking
+  APIs and achieved 25.4 MB/s with no data loss.
+* Transferred 32MB data from target to host with packet size 16KB using blocking
+  APIs and achieved 28.8 MB/s with no data loss.
+
+DBC TTY Use Cases
+------------------
+* Used for platform debugging via USB interface, in the absence of serial port.
+* It replace the need of using external serial-to-USB device for console access
+  or for low level debugging.
+* Used to send/receive data between platforms with the limited bandwidth.
+
+DBC RAW Use Cases
+------------------
+* Used at different platform (i.e. Linux, Android, Chrome etc.) for debugging
+  via USB interface.
+* It replace the need of using external serial-to-USB device for console access
+  or for low level debugging.
+* Transfer data with high data rate between platforms via DBC RAW without using
+  USB device controller.
+
+Conclusion
+-----------
+DBC RAW interface transfer data with 10x time faster than DBC TTY. And handle
+all the USB specific error like STALL packet, when the endpoint has had an error
+and its halt bit has been set.
diff --git a/Documentation/usb/index.rst b/Documentation/usb/index.rst
new file mode 100644
index 000000000000..d30b1057996b
--- /dev/null
+++ b/Documentation/usb/index.rst
@@ -0,0 +1,16 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+===============================
+Welcome to USB's documentation!
+===============================
+
+.. toctree::
+
+   dbc_raw
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`
-- 
2.21.0

Re: [PATCH 5/5] usb: xhci: dbc: Document describe about dbc raw interface

[Greg KH]

On Fri, Jun 07, 2019 at 12:03:06PM +0530, Prabhat Chand Pandey wrote:
> This patch have explaination about the new DBC interface called
> dbc raw interface. This cover the capability, target setup and
> use case info.
> 
> Signed-off-by: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> ---

I want to see this signed off by Mathias first, before I review it...

>  Documentation/usb/dbc_raw.rst | 136 ++++++++++++++++++++++++++++++++++
>  Documentation/usb/index.rst   |  16 ++++
>  2 files changed, 152 insertions(+)
>  create mode 100644 Documentation/usb/dbc_raw.rst
>  create mode 100644 Documentation/usb/index.rst
> 
> diff --git a/Documentation/usb/dbc_raw.rst b/Documentation/usb/dbc_raw.rst
> new file mode 100644
> index 000000000000..b7d472a478f5
> --- /dev/null
> +++ b/Documentation/usb/dbc_raw.rst
> @@ -0,0 +1,136 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +======================================
> +This described about DBC RAW Interface
> +======================================
> +
> +:Author: Prabhat Chand Pandey <prabhat.chand.pandey@intel.com>
> +
> +Content
> +========
> +
> +- DBC Overview
> +- Motivation
> +- DBC RAW Capabilities
> +- Target Build Setup
> +- Target Test Setup
> +- Host Target Connection
> +- Experiment Test Result
> +- DBC TTY Use Cases
> +- DBC RAW Use Cases
> +- Conclusion
> +
> +DBC Overview
> +-------------
> +xDBC stands for the USB Debug capability provided extensible Host Controller
> +Interface. Universal Serial Bus is a host controlled Bus. Host Controller is
> +a hardware whose functionality is to manage usb bus and usb host ports. It is
> +responsible for initiating and managing all usb transfers. Extensible Host
> +Controller Interface (xHCI) is a register-level interface which provides a
> +mechanism by which the host controller (xHC) can communicate with the Operating
> +System of the host computer.

No need to capatalize Operating System.

> In addition to exposing register interfaces
> +essential for proper functioning of the xHC it also supports many extended
> +capabilities which can optionally be implemented by xHC.
> +
> +It includes Extended Power Management Capability, I/O Virtualization capability
> +USB Legacy support capability among many others.

What is "It" here?


> --- /dev/null
> +++ b/Documentation/usb/index.rst
> @@ -0,0 +1,16 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +===============================
> +Welcome to USB's documentation!
> +===============================
> +
> +.. toctree::
> +
> +   dbc_raw
> +
> +.. only::  subproject and html
> +
> +   Indices
> +   =======
> +
> +   * :ref:`genindex`

USB has more documentation than just this one file :(

greg k-h

Re: [PATCH 0/5] usb: xhci: dbc: make modular and add RAW interface

[Greg KH]

On Fri, Jun 07, 2019 at 12:03:01PM +0530, Prabhat Chand Pandey wrote:
> This patch-set adds the following features to dbc driver:
> 
> - show the active dbc function and dbc descriptors, allowing
>   user space to dynamically modify the descriptors.

Why would we want userspace to modify these?

> - modularize dbc core to enable it to expose different function
>   interfaces, till now only TTY interface was exposed.

exposed where?

confused,

greg k-h