[PATCH] usb: xhci-dbg: Display endpoint number and direction in context dump

[PATCH] usb: xhci-dbg: Display endpoint number and direction in context dump

[Julius Werner]

When CONFIG_XHCI_HCD_DEBUGGING is activated, the XHCI driver can dump
device and input contexts to the console. The endpoint contexts in that
dump are labeled "Endpoint N Context", where N is DCI - 1... this is
very confusing, especially for people who are not that familiar with
the XHCI specification. Let's change this to display the endpoint number
and direction, which are much more commonly used concepts in USB (and
map to XHCI DCIs 1-to-1).

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/host/xhci-dbg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 5f3a7c7..98b1bee 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -507,7 +507,8 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
 		dma_addr_t dma = ctx->dma +
 			((unsigned long)ep_ctx - (unsigned long)ctx->bytes);
 
-		xhci_dbg(xhci, "Endpoint %02d Context:\n", i);
+		xhci_dbg(xhci, "Endpoint %02d %s Context:\n",
+				DIV_ROUND_UP(i, 2), i % 2 ? "OUT" : "IN");
 		xhci_dbg(xhci, "@%p (virt) @%08llx (dma) %#08x - ep_info\n",
 				&ep_ctx->ep_info,
 				(unsigned long long)dma, ep_ctx->ep_info);
-- 
1.8.1.3

Re: [PATCH] usb: xhci-dbg: Display endpoint number and direction in context dump

[Sarah Sharp]

On Thu, Apr 04, 2013 at 02:03:04PM -0700, Julius Werner wrote:
> When CONFIG_XHCI_HCD_DEBUGGING is activated, the XHCI driver can dump
> device and input contexts to the console. The endpoint contexts in that
> dump are labeled "Endpoint N Context", where N is DCI - 1... this is
> very confusing, especially for people who are not that familiar with
> the XHCI specification. Let's change this to display the endpoint number
> and direction, which are much more commonly used concepts in USB (and
> map to XHCI DCIs 1-to-1).

Thanks for the patch, I think it's a good idea, however...

> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/host/xhci-dbg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
> index 5f3a7c7..98b1bee 100644
> --- a/drivers/usb/host/xhci-dbg.c
> +++ b/drivers/usb/host/xhci-dbg.c
> @@ -507,7 +507,8 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
>  		dma_addr_t dma = ctx->dma +
>  			((unsigned long)ep_ctx - (unsigned long)ctx->bytes);
>  
> -		xhci_dbg(xhci, "Endpoint %02d Context:\n", i);
> +		xhci_dbg(xhci, "Endpoint %02d %s Context:\n",
> +				DIV_ROUND_UP(i, 2), i % 2 ? "OUT" : "IN");

...could you create two macros in xhci.h to translate the xHCI endpoint
context number to USB formated endpoint numbers, and IN vs. OUT?  I
suspect that there will be other places in the code where you'll want to
print the USB formatted endpoint numbers.  Having macros for later use
would be helpful.

Also, this patch is too late for the 3.10 merge window, so it will
have to wait for 3.11.

Sarah Sharp

[PATCH] usb: xhci-dbg: Display endpoint number and direction in context dump

[Julius Werner]

When CONFIG_XHCI_HCD_DEBUGGING is activated, the XHCI driver can dump
device and input contexts to the console. The endpoint contexts in that
dump are labeled "Endpoint N Context", where N is the XHCI endpoint
index (DCI - 1). This can be very confusing, especially for people who
are not that familiar with the XHCI specification. This patch introduces
an xhci_get_endpoint_address function (as a counterpart to the reverse
xhci_get_endpoint_index), and uses it to additionally display the
endpoint number and direction when dumping contexts, which are much more
commonly used concepts in USB.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/host/xhci-dbg.c |  5 ++++-
 drivers/usb/host/xhci.c     | 10 ++++++++++
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 5f3a7c7..16a8272 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -503,11 +503,14 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
 	if (last_ep < 31)
 		last_ep_ctx = last_ep + 1;
 	for (i = 0; i < last_ep_ctx; ++i) {
+		unsigned int epaddr = xhci_get_endpoint_address(i);
 		struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, ctx, i);
 		dma_addr_t dma = ctx->dma +
 			((unsigned long)ep_ctx - (unsigned long)ctx->bytes);
 
-		xhci_dbg(xhci, "Endpoint %02d Context:\n", i);
+		xhci_dbg(xhci, "Endpoint %02d %s Context (index %02d):\n",
+				epaddr & USB_ENDPOINT_NUMBER_MASK,
+				usb_endpoint_out(epaddr) ? "OUT" : "IN", i);
 		xhci_dbg(xhci, "@%p (virt) @%08llx (dma) %#08x - ep_info\n",
 				&ep_ctx->ep_info,
 				(unsigned long long)dma, ep_ctx->ep_info);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 53b8f89..3a3ac92 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1107,6 +1107,16 @@ unsigned int xhci_get_endpoint_index(struct usb_endpoint_descriptor *desc)
 	return index;
 }
 
+/* The reverse operation to xhci_get_endpoint_index. Calculate the USB endpoint
+ * address from the XHCI endpoint index.
+ */
+unsigned int xhci_get_endpoint_address(unsigned int ep_index)
+{
+	unsigned int number = DIV_ROUND_UP(ep_index, 2);
+	unsigned int direction = ep_index % 2 ? USB_DIR_OUT : USB_DIR_IN;
+	return direction | number;
+}
+
 /* Find the flag for this endpoint (for use in the control context).  Use the
  * endpoint index to create a bitmask.  The slot context is bit 0, endpoint 0 is
  * bit 1, etc.
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6358271..555e911 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1641,6 +1641,7 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *ud
 void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
 		struct usb_device *udev);
 unsigned int xhci_get_endpoint_index(struct usb_endpoint_descriptor *desc);
+unsigned int xhci_get_endpoint_address(unsigned int ep_index);
 unsigned int xhci_get_endpoint_flag(struct usb_endpoint_descriptor *desc);
 unsigned int xhci_get_endpoint_flag_from_index(unsigned int ep_index);
 unsigned int xhci_last_valid_endpoint(u32 added_ctxs);
-- 
1.7.12.4

Re: [PATCH] usb: xhci-dbg: Display endpoint number and direction in context dump

[Sarah Sharp]

On Mon, Apr 08, 2013 at 05:18:25PM -0700, Julius Werner wrote:
> diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
> index 5f3a7c7..16a8272 100644
> --- a/drivers/usb/host/xhci-dbg.c
> +++ b/drivers/usb/host/xhci-dbg.c
> @@ -503,11 +503,14 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
>  	if (last_ep < 31)
>  		last_ep_ctx = last_ep + 1;
>  	for (i = 0; i < last_ep_ctx; ++i) {
> +		unsigned int epaddr = xhci_get_endpoint_address(i);
>  		struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, ctx, i);
>  		dma_addr_t dma = ctx->dma +
>  			((unsigned long)ep_ctx - (unsigned long)ctx->bytes);
>  
> -		xhci_dbg(xhci, "Endpoint %02d Context:\n", i);
> +		xhci_dbg(xhci, "Endpoint %02d %s Context (index %02d):\n",

Sorry, one more nitpick.  Can you reword this to be like "Out Endpoint 0x80
Context i" instead of "Endpoint 0x80 OUT Context"?  There are input
contexts and output contexts in xHCI, and I want to imply the endpoint
is either IN or OUT, not the context.

Sarah Sharp

[PATCH] usb: xhci-dbg: Display endpoint number and direction in context dump

[Julius Werner]

When CONFIG_XHCI_HCD_DEBUGGING is activated, the XHCI driver can dump
device and input contexts to the console. The endpoint contexts in that
dump are labeled "Endpoint N Context", where N is the XHCI endpoint
index (DCI - 1). This can be very confusing, especially for people who
are not that familiar with the XHCI specification. This patch introduces
an xhci_get_endpoint_address function (as a counterpart to the reverse
xhci_get_endpoint_index), and uses it to additionally display the
endpoint number and direction when dumping contexts, which are much more
commonly used concepts in USB.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/host/xhci-dbg.c |  5 ++++-
 drivers/usb/host/xhci.c     | 10 ++++++++++
 drivers/usb/host/xhci.h     |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 5f3a7c7..f2e7689 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -503,11 +503,14 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
 	if (last_ep < 31)
 		last_ep_ctx = last_ep + 1;
 	for (i = 0; i < last_ep_ctx; ++i) {
+		unsigned int epaddr = xhci_get_endpoint_address(i);
 		struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, ctx, i);
 		dma_addr_t dma = ctx->dma +
 			((unsigned long)ep_ctx - (unsigned long)ctx->bytes);
 
-		xhci_dbg(xhci, "Endpoint %02d Context:\n", i);
+		xhci_dbg(xhci, "%s Endpoint %02d Context (ep_index %02d):\n",
+				usb_endpoint_out(epaddr) ? "OUT" : "IN",
+				epaddr & USB_ENDPOINT_NUMBER_MASK, i);
 		xhci_dbg(xhci, "@%p (virt) @%08llx (dma) %#08x - ep_info\n",
 				&ep_ctx->ep_info,
 				(unsigned long long)dma, ep_ctx->ep_info);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 53b8f89..3a3ac92 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1107,6 +1107,16 @@ unsigned int xhci_get_endpoint_index(struct usb_endpoint_descriptor *desc)
 	return index;
 }
 
+/* The reverse operation to xhci_get_endpoint_index. Calculate the USB endpoint
+ * address from the XHCI endpoint index.
+ */
+unsigned int xhci_get_endpoint_address(unsigned int ep_index)
+{
+	unsigned int number = DIV_ROUND_UP(ep_index, 2);
+	unsigned int direction = ep_index % 2 ? USB_DIR_OUT : USB_DIR_IN;
+	return direction | number;
+}
+
 /* Find the flag for this endpoint (for use in the control context).  Use the
  * endpoint index to create a bitmask.  The slot context is bit 0, endpoint 0 is
  * bit 1, etc.
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6358271..555e911 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1641,6 +1641,7 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *ud
 void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
 		struct usb_device *udev);
 unsigned int xhci_get_endpoint_index(struct usb_endpoint_descriptor *desc);
+unsigned int xhci_get_endpoint_address(unsigned int ep_index);
 unsigned int xhci_get_endpoint_flag(struct usb_endpoint_descriptor *desc);
 unsigned int xhci_get_endpoint_flag_from_index(unsigned int ep_index);
 unsigned int xhci_last_valid_endpoint(u32 added_ctxs);
-- 
1.7.12.4