Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-11-02 Thread Lu, Baolu 



On 10/30/2015 10:45 PM, Greg Kroah-Hartman wrote:

On Fri, Oct 30, 2015 at 08:09:17PM +0800, Lu, Baolu wrote:


On 10/28/2015 08:40 PM, Greg Kroah-Hartman wrote:

+static const char *get_extcap_desc(u32 cap_id)

+{
+   switch (cap_id) {
+   case XHCI_EXT_CAPS_LEGACY:
+   return "USB Legacy Support";
+   case XHCI_EXT_CAPS_PROTOCOL:
+   return "Supported Protocol";
+   case XHCI_EXT_CAPS_PM:
+   return "Extended Power Management";
+   case XHCI_EXT_CAPS_VIRT:
+   return "I/O Virtualization (xHCI-IOV)";
+   case XHCI_EXT_CAPS_ROUTE:
+   return "Message Interrupt";
+   case XHCI_EXT_CAPS_LOCALMEM:
+   return "Local Memory";
+   case XHCI_EXT_CAPS_DEBUG:
+   return "USB Debug Capability";

This is a lot more stuff than just debug port, it should be in sysfs
as individual files, not one big one that you somehow have to parse in
order to determine this information.


Hi Greg,

It's hard to put each extended capability into a individual sysfs file.

Agreed.


The extended capabilities are optional. One extended capability
might be supported in one hardware, but not in another. Also,
there are many "vendor defined" capabilities (ID range 192-255).
The vendor defined capabilities are not defined in xhci spec and
they could be used by the hardware vendor for various purposes.

The purpose of this patch is to let user know what kind of extended
capabilities does a host controller supported. For example, on
one of my develop machines, it prints,

@addr(virt)CAP_IDDescription
@c90001c8800002Supported Protocol
@c90001c8802002Supported Protocol
@c90001c88070c0Vendor Defined
@c90001c8846c01USB Legacy Support
@c90001c884f4c6Vendor Defined
@c90001c88500c7Vendor Defined
@c90001c88600c2Vendor Defined
@c90001c887000aUSB Debug Capability
@c90001c88740c3Vendor Defined
@c90001c88800c4Vendor Defined
@c90001c88900c5Vendor Defined

With this output I know that "USB Debug Capability" is supported
in my machine.

First off, why are you printing the address out?  Userspace never needs
to see that.  Why not just iterate through the protocols and export the
information as different files:
protocol_XX
and if the file is present or not describes if the hardware supports it
or not.

The issue with debugfs is that it is not enabled on all systems, and
only can be read by the root user, so it is hard for people to find this
information out if they want to do normal things with the hardware.


I agree with you that we should do this thru sysfs, not debugfs.



But, if this really is only a debug thing, then it can say a debugfs
file, but realize that almost no one will be able to see it (and even
then, don't export the kernel addresses of the hardware.)


I realized that not all extended capabilities are valuable to users.

As far as I can see, debug capability (hw implements USB3 debug
port) is one that users should be interested at. Another one that
could be valuable is USB 3.1 speed protocol (root ports that support
USB 3.1 enhanced super speed).

For this time being, I will only implement sysfs node for debug
port. I will make a separated patch for USB 3.1 speed port after
I have Mathias' opinion.



thanks,

greg k-h


Thanks,
Baolu


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-30 Thread Lu, Baolu 



On 10/28/2015 08:40 PM, Greg Kroah-Hartman wrote:

+static const char *get_extcap_desc(u32 cap_id)
>+{
>+   switch (cap_id) {
>+   case XHCI_EXT_CAPS_LEGACY:
>+   return "USB Legacy Support";
>+   case XHCI_EXT_CAPS_PROTOCOL:
>+   return "Supported Protocol";
>+   case XHCI_EXT_CAPS_PM:
>+   return "Extended Power Management";
>+   case XHCI_EXT_CAPS_VIRT:
>+   return "I/O Virtualization (xHCI-IOV)";
>+   case XHCI_EXT_CAPS_ROUTE:
>+   return "Message Interrupt";
>+   case XHCI_EXT_CAPS_LOCALMEM:
>+   return "Local Memory";
>+   case XHCI_EXT_CAPS_DEBUG:
>+   return "USB Debug Capability";

This is a lot more stuff than just debug port, it should be in sysfs
as individual files, not one big one that you somehow have to parse in
order to determine this information.



Hi Greg,

It's hard to put each extended capability into a individual sysfs file.

The extended capabilities are optional. One extended capability
might be supported in one hardware, but not in another. Also,
there are many "vendor defined" capabilities (ID range 192-255).
The vendor defined capabilities are not defined in xhci spec and
they could be used by the hardware vendor for various purposes.

The purpose of this patch is to let user know what kind of extended
capabilities does a host controller supported. For example, on
one of my develop machines, it prints,

@addr(virt)CAP_IDDescription
@c90001c8800002Supported Protocol
@c90001c8802002Supported Protocol
@c90001c88070c0Vendor Defined
@c90001c8846c01USB Legacy Support
@c90001c884f4c6Vendor Defined
@c90001c88500c7Vendor Defined
@c90001c88600c2Vendor Defined
@c90001c887000aUSB Debug Capability
@c90001c88740c3Vendor Defined
@c90001c88800c4Vendor Defined
@c90001c88900c5Vendor Defined

With this output I know that "USB Debug Capability" is supported
in my machine.

Thanks,
Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-30 Thread Greg Kroah-Hartman 
On Fri, Oct 30, 2015 at 08:09:17PM +0800, Lu, Baolu wrote:
> 
> 
> On 10/28/2015 08:40 PM, Greg Kroah-Hartman wrote:
> >>+static const char *get_extcap_desc(u32 cap_id)
> >>>+{
> >>>+  switch (cap_id) {
> >>>+  case XHCI_EXT_CAPS_LEGACY:
> >>>+  return "USB Legacy Support";
> >>>+  case XHCI_EXT_CAPS_PROTOCOL:
> >>>+  return "Supported Protocol";
> >>>+  case XHCI_EXT_CAPS_PM:
> >>>+  return "Extended Power Management";
> >>>+  case XHCI_EXT_CAPS_VIRT:
> >>>+  return "I/O Virtualization (xHCI-IOV)";
> >>>+  case XHCI_EXT_CAPS_ROUTE:
> >>>+  return "Message Interrupt";
> >>>+  case XHCI_EXT_CAPS_LOCALMEM:
> >>>+  return "Local Memory";
> >>>+  case XHCI_EXT_CAPS_DEBUG:
> >>>+  return "USB Debug Capability";
> >This is a lot more stuff than just debug port, it should be in sysfs
> >as individual files, not one big one that you somehow have to parse in
> >order to determine this information.
> >
> 
> Hi Greg,
> 
> It's hard to put each extended capability into a individual sysfs file.

Agreed.

> The extended capabilities are optional. One extended capability
> might be supported in one hardware, but not in another. Also,
> there are many "vendor defined" capabilities (ID range 192-255).
> The vendor defined capabilities are not defined in xhci spec and
> they could be used by the hardware vendor for various purposes.
> 
> The purpose of this patch is to let user know what kind of extended
> capabilities does a host controller supported. For example, on
> one of my develop machines, it prints,
> 
> @addr(virt)CAP_IDDescription
> @c90001c8800002Supported Protocol
> @c90001c8802002Supported Protocol
> @c90001c88070c0Vendor Defined
> @c90001c8846c01USB Legacy Support
> @c90001c884f4c6Vendor Defined
> @c90001c88500c7Vendor Defined
> @c90001c88600c2Vendor Defined
> @c90001c887000aUSB Debug Capability
> @c90001c88740c3Vendor Defined
> @c90001c88800c4Vendor Defined
> @c90001c88900c5Vendor Defined
> 
> With this output I know that "USB Debug Capability" is supported
> in my machine.

First off, why are you printing the address out?  Userspace never needs
to see that.  Why not just iterate through the protocols and export the
information as different files:
protocol_XX
and if the file is present or not describes if the hardware supports it
or not.

The issue with debugfs is that it is not enabled on all systems, and
only can be read by the root user, so it is hard for people to find this
information out if they want to do normal things with the hardware.

But, if this really is only a debug thing, then it can say a debugfs
file, but realize that almost no one will be able to see it (and even
then, don't export the kernel addresses of the hardware.)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-28 Thread Greg Kroah-Hartman 
On Wed, Oct 28, 2015 at 04:00:32PM +0800, Lu Baolu wrote:
> The xHCI host exports xHCI-specific extended capabilities utilizing
> a method similar to PCI extended capabilities. In many cases, users
> want to know whether a specific extended capability is supported by
> a host. Unfortunately, currently there's no existing mechanisms in
> the kernel to do this.
> 
> This patch exposes extended capabilities supported by the xHCI host
> via debugfs.

Why not through sysfs so that all users can see them?  Why debugfs,
which really is only for "debugging"?


> 
> Signed-off-by: Lu Baolu 
> ---
>  drivers/usb/host/xhci-dbg.c  | 212 
> +++
>  drivers/usb/host/xhci-ext-caps.h |   9 +-
>  drivers/usb/host/xhci.c  |  27 -
>  drivers/usb/host/xhci.h  |  10 ++
>  4 files changed, 256 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
> index 74c42f7..d3dcfed 100644
> --- a/drivers/usb/host/xhci-dbg.c
> +++ b/drivers/usb/host/xhci-dbg.c
> @@ -20,6 +20,11 @@
>   * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include "xhci.h"
>  
>  #define XHCI_INIT_VALUE 0x0
> @@ -612,3 +617,210 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void 
> (*trace)(struct va_format *),
>   va_end(args);
>  }
>  EXPORT_SYMBOL_GPL(xhci_dbg_trace);
> +
> +#ifdef CONFIG_DEBUG_FS

You shouldn't ever need to #ifdef your code for CONFIG_DEBUG_FS, the
functions work the same if it's enabled or not.

> +struct debug_buffer {
> + ssize_t (*fill_func)(struct debug_buffer *);
> + struct usb_bus *bus;
> + struct mutex mutex;
> + size_t count;
> + char *output_buf;
> + size_t alloc_size;
> +};
> +
> +static const char *get_extcap_desc(u32 cap_id)
> +{
> + switch (cap_id) {
> + case XHCI_EXT_CAPS_LEGACY:
> + return "USB Legacy Support";
> + case XHCI_EXT_CAPS_PROTOCOL:
> + return "Supported Protocol";
> + case XHCI_EXT_CAPS_PM:
> + return "Extended Power Management";
> + case XHCI_EXT_CAPS_VIRT:
> + return "I/O Virtualization (xHCI-IOV)";
> + case XHCI_EXT_CAPS_ROUTE:
> + return "Message Interrupt";
> + case XHCI_EXT_CAPS_LOCALMEM:
> + return "Local Memory";
> + case XHCI_EXT_CAPS_DEBUG:
> + return "USB Debug Capability";

This is a lot more stuff than just debug port, it should be in sysfs
as individual files, not one big one that you somehow have to parse in
order to determine this information.

> + default:
> + if (XHCI_EXT_CAPS_VENDOR(XHCI_EXT_CAPS_ID(cap_id)))
> + return "Vendor Defined";
> + else
> + return "Unknown";
> + }
> +}
> +
> +static ssize_t fill_extcap_buffer(struct debug_buffer *buf)
> +{
> + __le32 __iomem  *addr;
> + struct usb_hcd  *hcd;
> + struct xhci_hcd *xhci;
> + u32 offset, cap_id;
> + char*next;
> + int size, temp;
> + unsigned long   flags;
> + int time_to_leave;
> +
> + hcd = bus_to_hcd(buf->bus);
> + xhci = hcd_to_xhci(hcd);
> + next = buf->output_buf;
> + size = buf->alloc_size;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + addr = >cap_regs->hcc_params;
> + offset = XHCI_HCC_EXT_CAPS(readl(addr));
> + if (!HCD_HW_ACCESSIBLE(hcd) || !offset) {
> + size = scnprintf(next, size,
> + "bus %s, device %s\n%s\nNo extended capabilities\n",
> + hcd->self.controller->bus->name,
> + dev_name(hcd->self.controller),
> + hcd->product_desc);
> + goto done;
> + }
> +
> + temp = scnprintf(next, size, "@addr(virt)\t\tCAP_ID\tDescription\n");
> + size -= temp;
> + next += temp;
> +
> + addr = >cap_regs->hc_capbase + offset;
> + time_to_leave = XHCI_EXT_MAX_CAPID;
> + while (time_to_leave--) {
> + cap_id = readl(addr);
> + temp = scnprintf(next, size, "@%p\t%02x\t%s\n",
> + addr, XHCI_EXT_CAPS_ID(cap_id),
> + get_extcap_desc(XHCI_EXT_CAPS_ID(cap_id)));
> + size -= temp;
> + next += temp;
> +
> + offset = XHCI_EXT_CAPS_NEXT(cap_id);
> + if (!offset)
> + break;
> + addr += offset;
> + }
> +
> +done:
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return buf->alloc_size - size;
> +}
> +
> +static struct debug_buffer *buffer_init(struct usb_bus *bus,
> + ssize_t (*fill_func)(struct debug_buffer *))
> +{
> + struct debug_buffer *buf;
> +
> + buf = kzalloc(sizeof(struct debug_buffer), GFP_KERNEL);
> + if (!buf)
> + return NULL;
> +
> + buf->bus = bus;
> + buf->fill_func =

[PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-28 Thread Lu Baolu 
The xHCI host exports xHCI-specific extended capabilities utilizing
a method similar to PCI extended capabilities. In many cases, users
want to know whether a specific extended capability is supported by
a host. Unfortunately, currently there's no existing mechanisms in
the kernel to do this.

This patch exposes extended capabilities supported by the xHCI host
via debugfs.

Signed-off-by: Lu Baolu 
---
 drivers/usb/host/xhci-dbg.c  | 212 +++
 drivers/usb/host/xhci-ext-caps.h |   9 +-
 drivers/usb/host/xhci.c  |  27 -
 drivers/usb/host/xhci.h  |  10 ++
 4 files changed, 256 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 74c42f7..d3dcfed 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -20,6 +20,11 @@
  * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include 
+#include 
+#include 
+#include 
+
 #include "xhci.h"
 
 #define XHCI_INIT_VALUE 0x0
@@ -612,3 +617,210 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void 
(*trace)(struct va_format *),
va_end(args);
 }
 EXPORT_SYMBOL_GPL(xhci_dbg_trace);
+
+#ifdef CONFIG_DEBUG_FS
+struct debug_buffer {
+   ssize_t (*fill_func)(struct debug_buffer *);
+   struct usb_bus *bus;
+   struct mutex mutex;
+   size_t count;
+   char *output_buf;
+   size_t alloc_size;
+};
+
+static const char *get_extcap_desc(u32 cap_id)
+{
+   switch (cap_id) {
+   case XHCI_EXT_CAPS_LEGACY:
+   return "USB Legacy Support";
+   case XHCI_EXT_CAPS_PROTOCOL:
+   return "Supported Protocol";
+   case XHCI_EXT_CAPS_PM:
+   return "Extended Power Management";
+   case XHCI_EXT_CAPS_VIRT:
+   return "I/O Virtualization (xHCI-IOV)";
+   case XHCI_EXT_CAPS_ROUTE:
+   return "Message Interrupt";
+   case XHCI_EXT_CAPS_LOCALMEM:
+   return "Local Memory";
+   case XHCI_EXT_CAPS_DEBUG:
+   return "USB Debug Capability";
+   default:
+   if (XHCI_EXT_CAPS_VENDOR(XHCI_EXT_CAPS_ID(cap_id)))
+   return "Vendor Defined";
+   else
+   return "Unknown";
+   }
+}
+
+static ssize_t fill_extcap_buffer(struct debug_buffer *buf)
+{
+   __le32 __iomem  *addr;
+   struct usb_hcd  *hcd;
+   struct xhci_hcd *xhci;
+   u32 offset, cap_id;
+   char*next;
+   int size, temp;
+   unsigned long   flags;
+   int time_to_leave;
+
+   hcd = bus_to_hcd(buf->bus);
+   xhci = hcd_to_xhci(hcd);
+   next = buf->output_buf;
+   size = buf->alloc_size;
+
+   spin_lock_irqsave(>lock, flags);
+
+   addr = >cap_regs->hcc_params;
+   offset = XHCI_HCC_EXT_CAPS(readl(addr));
+   if (!HCD_HW_ACCESSIBLE(hcd) || !offset) {
+   size = scnprintf(next, size,
+   "bus %s, device %s\n%s\nNo extended capabilities\n",
+   hcd->self.controller->bus->name,
+   dev_name(hcd->self.controller),
+   hcd->product_desc);
+   goto done;
+   }
+
+   temp = scnprintf(next, size, "@addr(virt)\t\tCAP_ID\tDescription\n");
+   size -= temp;
+   next += temp;
+
+   addr = >cap_regs->hc_capbase + offset;
+   time_to_leave = XHCI_EXT_MAX_CAPID;
+   while (time_to_leave--) {
+   cap_id = readl(addr);
+   temp = scnprintf(next, size, "@%p\t%02x\t%s\n",
+   addr, XHCI_EXT_CAPS_ID(cap_id),
+   get_extcap_desc(XHCI_EXT_CAPS_ID(cap_id)));
+   size -= temp;
+   next += temp;
+
+   offset = XHCI_EXT_CAPS_NEXT(cap_id);
+   if (!offset)
+   break;
+   addr += offset;
+   }
+
+done:
+   spin_unlock_irqrestore(>lock, flags);
+
+   return buf->alloc_size - size;
+}
+
+static struct debug_buffer *buffer_init(struct usb_bus *bus,
+   ssize_t (*fill_func)(struct debug_buffer *))
+{
+   struct debug_buffer *buf;
+
+   buf = kzalloc(sizeof(struct debug_buffer), GFP_KERNEL);
+   if (!buf)
+   return NULL;
+
+   buf->bus = bus;
+   buf->fill_func = fill_func;
+   mutex_init(>mutex);
+
+   return buf;
+}
+
+static int fill_buffer(struct debug_buffer *buf)
+{
+   int ret;
+
+   if (buf->output_buf)
+   return -EINVAL;
+
+   buf->alloc_size = PAGE_SIZE;
+   buf->output_buf = vmalloc(buf->alloc_size);
+
+   if (!buf->output_buf)
+   return -ENOMEM;
+
+   ret = buf->fill_func(buf);
+   if (ret < 0)
+   return ret;
+
+   buf->count = ret;
+
+   return 0;
+}
+
+static ssize_t debug_output(struct file *file, char __user *user_buf,
+   size_t

Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-28 Thread Oliver Neukum 
On Wed, 2015-10-28 at 16:00 +0800, Lu Baolu wrote:
> The xHCI host exports xHCI-specific extended capabilities utilizing
> a method similar to PCI extended capabilities. In many cases, users
> want to know whether a specific extended capability is supported by
> a host. Unfortunately, currently there's no existing mechanisms in
> the kernel to do this.
> 
> This patch exposes extended capabilities supported by the xHCI host
> via debugfs.
> 
> Signed-off-by: Lu Baolu 
> ---
>  drivers/usb/host/xhci-dbg.c  | 212 
> +++
>  drivers/usb/host/xhci-ext-caps.h |   9 +-
>  drivers/usb/host/xhci.c  |  27 -
>  drivers/usb/host/xhci.h  |  10 ++
>  4 files changed, 256 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
> index 74c42f7..d3dcfed 100644
> --- a/drivers/usb/host/xhci-dbg.c
> +++ b/drivers/usb/host/xhci-dbg.c
> @@ -20,6 +20,11 @@
>   * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
> +#include 
> +#include 
> +#include 
> +#include 
> +
>  #include "xhci.h"
>  
>  #define XHCI_INIT_VALUE 0x0
> @@ -612,3 +617,210 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void 
> (*trace)(struct va_format *),
>   va_end(args);
>  }
>  EXPORT_SYMBOL_GPL(xhci_dbg_trace);
> +
> +#ifdef CONFIG_DEBUG_FS
> +struct debug_buffer {
> + ssize_t (*fill_func)(struct debug_buffer *);
> + struct usb_bus *bus;
> + struct mutex mutex;
> + size_t count;
> + char *output_buf;
> + size_t alloc_size;
> +};
> +
> +static const char *get_extcap_desc(u32 cap_id)
> +{
> + switch (cap_id) {
> + case XHCI_EXT_CAPS_LEGACY:
> + return "USB Legacy Support";
> + case XHCI_EXT_CAPS_PROTOCOL:
> + return "Supported Protocol";
> + case XHCI_EXT_CAPS_PM:
> + return "Extended Power Management";
> + case XHCI_EXT_CAPS_VIRT:
> + return "I/O Virtualization (xHCI-IOV)";
> + case XHCI_EXT_CAPS_ROUTE:
> + return "Message Interrupt";
> + case XHCI_EXT_CAPS_LOCALMEM:
> + return "Local Memory";
> + case XHCI_EXT_CAPS_DEBUG:
> + return "USB Debug Capability";
> + default:
> + if (XHCI_EXT_CAPS_VENDOR(XHCI_EXT_CAPS_ID(cap_id)))
> + return "Vendor Defined";
> + else
> + return "Unknown";
> + }
> +}
> +
> +static ssize_t fill_extcap_buffer(struct debug_buffer *buf)
> +{
> + __le32 __iomem  *addr;
> + struct usb_hcd  *hcd;
> + struct xhci_hcd *xhci;
> + u32 offset, cap_id;
> + char*next;
> + int size, temp;
> + unsigned long   flags;
> + int time_to_leave;
> +
> + hcd = bus_to_hcd(buf->bus);
> + xhci = hcd_to_xhci(hcd);
> + next = buf->output_buf;
> + size = buf->alloc_size;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + addr = >cap_regs->hcc_params;
> + offset = XHCI_HCC_EXT_CAPS(readl(addr));
> + if (!HCD_HW_ACCESSIBLE(hcd) || !offset) {
> + size = scnprintf(next, size,
> + "bus %s, device %s\n%s\nNo extended capabilities\n",
> + hcd->self.controller->bus->name,
> + dev_name(hcd->self.controller),
> + hcd->product_desc);
> + goto done;
> + }
> +
> + temp = scnprintf(next, size, "@addr(virt)\t\tCAP_ID\tDescription\n");
> + size -= temp;
> + next += temp;
> +
> + addr = >cap_regs->hc_capbase + offset;
> + time_to_leave = XHCI_EXT_MAX_CAPID;
> + while (time_to_leave--) {
> + cap_id = readl(addr);
> + temp = scnprintf(next, size, "@%p\t%02x\t%s\n",
> + addr, XHCI_EXT_CAPS_ID(cap_id),
> + get_extcap_desc(XHCI_EXT_CAPS_ID(cap_id)));
> + size -= temp;
> + next += temp;
> +
> + offset = XHCI_EXT_CAPS_NEXT(cap_id);
> + if (!offset)
> + break;
> + addr += offset;
> + }
> +
> +done:
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return buf->alloc_size - size;
> +}
> +
> +static struct debug_buffer *buffer_init(struct usb_bus *bus,
> + ssize_t (*fill_func)(struct debug_buffer *))
> +{
> + struct debug_buffer *buf;
> +
> + buf = kzalloc(sizeof(struct debug_buffer), GFP_KERNEL);
> + if (!buf)
> + return NULL;
> +
> + buf->bus = bus;
> + buf->fill_func = fill_func;
> + mutex_init(>mutex);
> +
> + return buf;
> +}
> +
> +static int fill_buffer(struct debug_buffer *buf)
> +{
> + int ret;
> +
> + if (buf->output_buf)
> + return -EINVAL;
> +
> + buf->alloc_size = PAGE_SIZE;
> + buf->output_buf = vmalloc(buf->alloc_size);

That really makes no sense. If you allocate exactly
PAGE_SIZE, you should allocate a page.

Regards

Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-28 Thread Lu, Baolu 



On 10/28/2015 05:27 PM, Oliver Neukum wrote:

+static int fill_buffer(struct debug_buffer *buf)
>+{
>+   int ret;
>+
>+   if (buf->output_buf)
>+   return -EINVAL;
>+
>+   buf->alloc_size = PAGE_SIZE;
>+   buf->output_buf = vmalloc(buf->alloc_size);

That really makes no sense. If you allocate exactly
PAGE_SIZE, you should allocate a page.


Yes, I will change it in v2.



Regards
Oliver


Thanks,
Baolu







--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

2015-10-28 Thread Lu, Baolu 



On 10/28/2015 08:40 PM, Greg Kroah-Hartman wrote:

>+struct debug_buffer {
>+   ssize_t (*fill_func)(struct debug_buffer *);
>+   struct usb_bus *bus;
>+   struct mutex mutex;
>+   size_t count;
>+   char *output_buf;
>+   size_t alloc_size;
>+};
>+
>+static const char *get_extcap_desc(u32 cap_id)
>+{
>+   switch (cap_id) {
>+   case XHCI_EXT_CAPS_LEGACY:
>+   return "USB Legacy Support";
>+   case XHCI_EXT_CAPS_PROTOCOL:
>+   return "Supported Protocol";
>+   case XHCI_EXT_CAPS_PM:
>+   return "Extended Power Management";
>+   case XHCI_EXT_CAPS_VIRT:
>+   return "I/O Virtualization (xHCI-IOV)";
>+   case XHCI_EXT_CAPS_ROUTE:
>+   return "Message Interrupt";
>+   case XHCI_EXT_CAPS_LOCALMEM:
>+   return "Local Memory";
>+   case XHCI_EXT_CAPS_DEBUG:
>+   return "USB Debug Capability";

This is a lot more stuff than just debug port, it should be in sysfs
as individual files, not one big one that you somehow have to parse in
order to determine this information.



I will move it into sysfs in v2.

Thanks,
Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html