Message-ID: <20151028124017.GB6856@kroah.com>
Date:	Wed, 28 Oct 2015 21:40:17 +0900
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Lu Baolu <baolu.lu@...ux.intel.com>
Cc:	Mathias Nyman <mathias.nyman@...el.com>,
	Alan Stern <stern@...land.harvard.edu>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via
 debugfs

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 <baolu.lu@...ux.intel.com>
> ---
>  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 <linux/vmalloc.h>
> +#include <linux/slab.h>
> +#include <linux/debugfs.h>
> +#include <linux/usb.h>
> +
>  #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(&xhci->lock, flags);
> +
> +	addr = &xhci->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 = &xhci->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(&xhci->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(&buf->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 len, loff_t *offset)
> +{
> +	struct debug_buffer *buf = file->private_data;
> +	int ret = 0;
> +
> +	mutex_lock(&buf->mutex);
> +	if (!buf->count) {
> +		ret = fill_buffer(buf);
> +		if (ret) {
> +			mutex_unlock(&buf->mutex);
> +			return ret;
> +		}
> +	}
> +	mutex_unlock(&buf->mutex);
> +
> +	return simple_read_from_buffer(user_buf, len, offset,
> +				      buf->output_buf, buf->count);
> +}
> +
> +static int debug_close(struct inode *inode, struct file *file)
> +{
> +	struct debug_buffer *buf = file->private_data;
> +
> +	if (buf) {
> +		vfree(buf->output_buf);
> +		kfree(buf);
> +	}
> +
> +	return 0;
> +}
> +
> +static int debug_extcap_open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = buffer_init(inode->i_private,
> +					  fill_extcap_buffer);
> +
> +	return file->private_data ? 0 : -ENOMEM;
> +}
> +
> +static const struct file_operations debug_extcap_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= debug_extcap_open,
> +	.read		= debug_output,
> +	.release	= debug_close,
> +	.llseek		= default_llseek,
> +};
> +
> +struct dentry *xhci_debug_root;
> +
> +void xhci_create_debug_files(struct xhci_hcd *xhci)
> +{
> +	struct usb_bus *bus = &xhci_to_hcd(xhci)->self;
> +	struct dentry *entry;
> +
> +	if (!xhci_debug_root)
> +		return;
> +
> +	entry = debugfs_create_dir(bus->bus_name, xhci_debug_root);
> +	if (!entry || IS_ERR(entry)) {
> +		xhci_info(xhci, "failed to create debug dir");

Why are "errors" an info level?

> +		return;

No, you just "failed" if debugfs is not enabled.

You don't need to do any real error checking here, just make the call
and save off the dentry.  If an error happened, it's NULL and you can't
really do anything with that, and it's safe to pass back into debugfs
for other things, so you are fine.  Don't make debugfs interactions more
complex than they have to be.


> +	}
> +	xhci->debug_dir = entry;
> +
> +	if (!debugfs_create_file("extcap", S_IRUGO,
> +				xhci->debug_dir, bus,
> +				&debug_extcap_fops))
> +		xhci_info(xhci, "failed to create extcap debug file");

Again, info level?

> +}
> +
> +void xhci_remove_debug_files(struct xhci_hcd *xhci)
> +{
> +	debugfs_remove_recursive(xhci->debug_dir);
> +	xhci->debug_dir = NULL;
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
> index 9fe3225..e233c90 100644
> --- a/drivers/usb/host/xhci-ext-caps.h
> +++ b/drivers/usb/host/xhci-ext-caps.h
> @@ -49,8 +49,15 @@
>  #define XHCI_EXT_CAPS_PM	3
>  #define XHCI_EXT_CAPS_VIRT	4
>  #define XHCI_EXT_CAPS_ROUTE	5
> -/* IDs 6-9 reserved */
> +#define	XHCI_EXT_CAPS_LOCALMEM	6
> +/* IDs 7-9 reserved */
>  #define XHCI_EXT_CAPS_DEBUG	10
> +/* IDs 192-255 vendor specific */
> +#define	XHCI_EXT_CAPS_VEN_START	192
> +#define	XHCI_EXT_CAPS_VEN_END	255
> +#define	XHCI_EXT_CAPS_VENDOR(p)	(((p) >= XHCI_EXT_CAPS_VEN_START) && \
> +				((p) <= XHCI_EXT_CAPS_VEN_END))
> +#define	XHCI_EXT_MAX_CAPID	XHCI_EXT_CAPS_VEN_END
>  /* USB Legacy Support Capability - section 7.1.1 */
>  #define XHCI_HC_BIOS_OWNED	(1 << 16)
>  #define XHCI_HC_OS_OWNED	(1 << 24)
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6e7dc6f..ddcb4b7 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  #include <linux/dmi.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/debugfs.h>
>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
> @@ -651,6 +652,11 @@ int xhci_run(struct usb_hcd *hcd)
>  	}
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>  			"Finished xhci_run for USB2 roothub");
> +
> +#ifdef CONFIG_DEBUG_FS
> +	xhci_create_debug_files(xhci);
> +#endif

Don't ever put ifdefs in .c code if you can prevent it.  Hint, you can
prevent it here...

> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(xhci_run);
> @@ -669,6 +675,10 @@ void xhci_stop(struct usb_hcd *hcd)
>  	u32 temp;
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>  
> +#ifdef CONFIG_DEBUG_FS
> +	xhci_remove_debug_files(xhci);
> +#endif

Same here.

> +
>  	if (xhci->xhc_state & XHCI_STATE_HALTED)
>  		return;
>  
> @@ -5041,6 +5051,15 @@ static int __init xhci_hcd_init(void)
>  	BUILD_BUG_ON(sizeof(struct xhci_intr_reg) != 8*32/8);
>  	/* xhci_run_regs has eight fields and embeds 128 xhci_intr_regs */
>  	BUILD_BUG_ON(sizeof(struct xhci_run_regs) != (8+8*128)*32/8);
> +
> +#ifdef CONFIG_DEBUG_FS
> +	xhci_debug_root = debugfs_create_dir("xhci", usb_debug_root);
> +	if (!xhci_debug_root || IS_ERR(xhci_debug_root)) {
> +		debugfs_remove(xhci_debug_root);
> +		xhci_debug_root = NULL;
> +	}
> +#endif

And same here.

> +
>  	return 0;
>  }
>  
> @@ -5048,7 +5067,13 @@ static int __init xhci_hcd_init(void)
>   * If an init function is provided, an exit function must also be provided
>   * to allow module unload.
>   */
> -static void __exit xhci_hcd_fini(void) { }
> +static void __exit xhci_hcd_fini(void)
> +{
> +#ifdef CONFIG_DEBUG_FS
> +	debugfs_remove(xhci_debug_root);
> +	xhci_debug_root = NULL;
> +#endif

And here.


> +}
>  
>  module_init(xhci_hcd_init);
>  module_exit(xhci_hcd_fini);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index be9048e..dc3a5f8 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1657,6 +1657,10 @@ struct xhci_hcd {
>  	u32			port_status_u0;
>  /* Compliance Mode Timer Triggered every 2 seconds */
>  #define COMP_MODE_RCVRY_MSECS 2000
> +	/* debug files */
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry		*debug_dir;
> +#endif /* CONFIG_DEBUG_FS */

No need for ifdef here as well.

>  };
>  
>  /* Platform specific overrides to generic XHCI hc_driver ops */
> @@ -1743,6 +1747,12 @@ void xhci_dbg_ep_rings(struct xhci_hcd *xhci,
>  void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
>  			const char *fmt, ...);
>  
> +#ifdef CONFIG_DEBUG_FS
> +extern struct dentry *xhci_debug_root;
> +void xhci_create_debug_files(struct xhci_hcd *xhci);
> +void xhci_remove_debug_files(struct xhci_hcd *xhci);
> +#endif /* CONFIG_DEBUG_FS */

Or here like this...

But as you are moving this to sysfs, it's kind of moot...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists