[PATCH] usb: core: add debugobjects support for urb object

[PATCH] usb: core: add debugobjects support for urb object

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Add debugobject support to track the life time of struct urb.
This feature help us detect violation of urb operations by
generating a warning message from debugobject core. And we fix
the possible issues at runtime to avoid oops if we can.

I have done some tests with some class drivers, no violation
found in them which is good. Expect this feature can be used
for debugging future problems.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/core/hcd.c |   1 +
 drivers/usb/core/urb.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/usb.h    |   8 ++++
 lib/Kconfig.debug      |   8 ++++
 4 files changed, 130 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34b837a..a8ea128 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1750,6 +1750,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
 	/* pass ownership to the completion handler */
 	urb->status = status;
 
+	debug_urb_deactivate(urb);
 	/*
 	 * We disable local IRQs here avoid possible deadlock because
 	 * drivers may call spin_lock() to hold lock which might be
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index c601e25..0d1eccb 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -10,6 +10,100 @@
 
 #define to_urb(d) container_of(d, struct urb, kref)
 
+#ifdef CONFIG_DEBUG_OBJECTS_URB
+static struct debug_obj_descr urb_debug_descr;
+
+static void *urb_debug_hint(void *addr)
+{
+	return ((struct urb *) addr)->complete;
+}
+
+/*
+ * fixup_init is called when:
+ * - an active object is initialized
+ */
+static bool urb_fixup_init(void *addr, enum debug_obj_state state)
+{
+	struct urb *urb = addr;
+
+	switch (state) {
+	case ODEBUG_STATE_ACTIVE:
+		usb_kill_urb(urb);
+		debug_object_init(urb, &urb_debug_descr);
+		return true;
+	default:
+		return false;
+	}
+}
+
+/*
+ * fixup_activate is called when:
+ * - an active object is activated
+ * - an unknown non-static object is activated
+ */
+static bool urb_fixup_activate(void *addr, enum debug_obj_state state)
+{
+	struct urb *urb = urb;
+
+	switch (state) {
+	case ODEBUG_STATE_ACTIVE:
+		usb_kill_urb(urb);
+		debug_object_activate(urb, &urb_debug_descr);
+		return true;
+	default:
+		return false;
+	}
+}
+
+/*
+ * fixup_free is called when:
+ * - an active object is freed
+ */
+static bool urb_fixup_free(void *addr, enum debug_obj_state state)
+{
+	struct urb *urb = addr;
+
+	switch (state) {
+	case ODEBUG_STATE_ACTIVE:
+		usb_kill_urb(urb);
+		debug_object_free(urb, &urb_debug_descr);
+		return true;
+	default:
+		return false;
+	}
+}
+
+static struct debug_obj_descr urb_debug_descr = {
+	.name		= "urb",
+	.debug_hint	= urb_debug_hint,
+	.fixup_init	= urb_fixup_init,
+	.fixup_activate	= urb_fixup_activate,
+	.fixup_free	= urb_fixup_free,
+};
+
+static void debug_urb_init(struct urb *urb)
+{
+	/**
+	 * The struct urb structure must never be
+	 * created statically, so no init object
+	 * on stack case.
+	 */
+	debug_object_init(urb, &urb_debug_descr);
+}
+
+int debug_urb_activate(struct urb *urb)
+{
+	return debug_object_activate(urb, &urb_debug_descr);
+}
+
+void debug_urb_deactivate(struct urb *urb)
+{
+	debug_object_deactivate(urb, &urb_debug_descr);
+}
+
+#else
+static inline void debug_urb_init(struct urb *urb) { }
+#endif
 
 static void urb_destroy(struct kref *kref)
 {
@@ -41,6 +135,7 @@ void usb_init_urb(struct urb *urb)
 		memset(urb, 0, sizeof(*urb));
 		kref_init(&urb->kref);
 		INIT_LIST_HEAD(&urb->anchor_list);
+		debug_urb_init(urb);
 	}
 }
 EXPORT_SYMBOL_GPL(usb_init_urb);
@@ -331,6 +426,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	struct usb_host_endpoint	*ep;
 	int				is_out;
 	unsigned int			allowed;
+	int				ret;
 
 	if (!urb || !urb->complete)
 		return -EINVAL;
@@ -539,10 +635,23 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 		}
 	}
 
-	return usb_hcd_submit_urb(urb, mem_flags);
+	ret = debug_urb_activate(urb);
+	if (ret)
+		return ret;
+	ret = usb_hcd_submit_urb(urb, mem_flags);
+	if (ret)
+		debug_urb_deactivate(urb);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(usb_submit_urb);
 
+static inline int __usb_unlink_urb(struct urb *urb, int status)
+{
+	debug_urb_deactivate(urb);
+	return usb_hcd_unlink_urb(urb, status);
+}
+
 /*-------------------------------------------------------------------*/
 
 /**
@@ -626,7 +735,7 @@ int usb_unlink_urb(struct urb *urb)
 		return -ENODEV;
 	if (!urb->ep)
 		return -EIDRM;
-	return usb_hcd_unlink_urb(urb, -ECONNRESET);
+	return __usb_unlink_urb(urb, -ECONNRESET);
 }
 EXPORT_SYMBOL_GPL(usb_unlink_urb);
 
@@ -664,7 +773,7 @@ void usb_kill_urb(struct urb *urb)
 		return;
 	atomic_inc(&urb->reject);
 
-	usb_hcd_unlink_urb(urb, -ENOENT);
+	__usb_unlink_urb(urb, -ENOENT);
 	wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
 
 	atomic_dec(&urb->reject);
@@ -708,7 +817,7 @@ void usb_poison_urb(struct urb *urb)
 	if (!urb->dev || !urb->ep)
 		return;
 
-	usb_hcd_unlink_urb(urb, -ENOENT);
+	__usb_unlink_urb(urb, -ENOENT);
 	wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
 }
 EXPORT_SYMBOL_GPL(usb_poison_urb);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index eba1f10..16f2dee 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1606,6 +1606,14 @@ static inline void usb_fill_int_urb(struct urb *urb,
 	urb->start_frame = -1;
 }
 
+#ifdef CONFIG_DEBUG_OBJECTS_URB
+extern int debug_urb_activate(struct urb *urb);
+extern void debug_urb_deactivate(struct urb *urb);
+#else
+static inline int debug_urb_activate(struct urb *urb) { return 0; }
+static inline void debug_urb_deactivate(struct urb *urb) { }
+#endif
+
 extern void usb_init_urb(struct urb *urb);
 extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags);
 extern void usb_free_urb(struct urb *urb);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e707ab3..4f5bd59 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -458,6 +458,14 @@ config DEBUG_OBJECTS_PERCPU_COUNTER
 	  percpu counter routines to track the life time of percpu counter
 	  objects and validate the percpu counter operations.
 
+config DEBUG_OBJECTS_URB
+	bool "Debug usb urb objects"
+	depends on DEBUG_OBJECTS
+	help
+	  If you say Y here, additional code will be inserted into the
+	  usb core routines to track the life time of urb objects and
+	  validate the urb operations.
+
 config DEBUG_OBJECTS_ENABLE_DEFAULT
 	int "debug_objects bootup default value (0-1)"
         range 0 1
-- 
2.7.4

Re: [PATCH] usb: core: add debugobjects support for urb object

[Alan Stern]

On Tue, 24 May 2016 changbin.du@intel.com wrote:

> From: "Du, Changbin" <changbin.du@intel.com>
> 
> Add debugobject support to track the life time of struct urb.
> This feature help us detect violation of urb operations by
> generating a warning message from debugobject core.

I'm pretty sure the USB core already does this tracking.  If it
doesn't, why not make the appropriate changes instead of adding a whole
new infrastructure?

>  And we fix
> the possible issues at runtime to avoid oops if we can.

This is questionable.  Under what conditions do you think you can fix
up a possible issue?

> I have done some tests with some class drivers, no violation
> found in them which is good. Expect this feature can be used
> for debugging future problems.
> 
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
>  drivers/usb/core/hcd.c |   1 +
>  drivers/usb/core/urb.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/usb.h    |   8 ++++
>  lib/Kconfig.debug      |   8 ++++
>  4 files changed, 130 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..a8ea128 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1750,6 +1750,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>  	/* pass ownership to the completion handler */
>  	urb->status = status;
>  
> +	debug_urb_deactivate(urb);
>  	/*
>  	 * We disable local IRQs here avoid possible deadlock because
>  	 * drivers may call spin_lock() to hold lock which might be

> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index c601e25..0d1eccb 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -10,6 +10,100 @@
>  
>  #define to_urb(d) container_of(d, struct urb, kref)
>  
> +#ifdef CONFIG_DEBUG_OBJECTS_URB
> +static struct debug_obj_descr urb_debug_descr;
> +
> +static void *urb_debug_hint(void *addr)
> +{
> +	return ((struct urb *) addr)->complete;
> +}
> +
> +/*
> + * fixup_init is called when:
> + * - an active object is initialized
> + */
> +static bool urb_fixup_init(void *addr, enum debug_obj_state state)
> +{
> +	struct urb *urb = addr;
> +
> +	switch (state) {
> +	case ODEBUG_STATE_ACTIVE:
> +		usb_kill_urb(urb);
> +		debug_object_init(urb, &urb_debug_descr);
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

Is it guaranteed that this routine and the other new ones can be called
only in process context?  I don't think so -- but usb_kill_urb will 
oops if it is called with interrupts disabled.

> @@ -539,10 +635,23 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  		}
>  	}
>  
> -	return usb_hcd_submit_urb(urb, mem_flags);
> +	ret = debug_urb_activate(urb);
> +	if (ret)
> +		return ret;
> +	ret = usb_hcd_submit_urb(urb, mem_flags);
> +	if (ret)
> +		debug_urb_deactivate(urb);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_submit_urb);
>  
> +static inline int __usb_unlink_urb(struct urb *urb, int status)
> +{
> +	debug_urb_deactivate(urb);
> +	return usb_hcd_unlink_urb(urb, status);
> +}

This is a mistake.  The URB is still active until it is given back
to the completion routine.

Alan Stern

Re: [PATCH] usb: core: add debugobjects support for urb object

[Greg KH]

On Tue, May 24, 2016 at 03:53:53PM +0800, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
> 
> Add debugobject support to track the life time of struct urb.
> This feature help us detect violation of urb operations by
> generating a warning message from debugobject core. And we fix
> the possible issues at runtime to avoid oops if we can.
> 
> I have done some tests with some class drivers, no violation
> found in them which is good. Expect this feature can be used
> for debugging future problems.
> 
> Signed-off-by: Du, Changbin <changbin.du@intel.com>

I agree with Alan, what use is this code?  Existing drivers all work
properly because the reference counting of urbs is already present, why
add duplicate counters?  That actually leads to bugs.  I don't see the
need for this code, please explain it better if you wish for it to be
accepted.  What has it found/fixed that we have not found yet?  What
does it allow you to do that you can't do today with the existing code?

thanks,

greg k-h

RE: [PATCH] usb: core: add debugobjects support for urb object

[Du, Changbin]

> On Tue, May 24, 2016 at 03:53:53PM +0800, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > Add debugobject support to track the life time of struct urb.
> > This feature help us detect violation of urb operations by
> > generating a warning message from debugobject core. And we fix
> > the possible issues at runtime to avoid oops if we can.
> >
> > I have done some tests with some class drivers, no violation
> > found in them which is good. Expect this feature can be used
> > for debugging future problems.
> >
> > Signed-off-by: Du, Changbin <changbin.du@intel.com>
> 
> I agree with Alan, what use is this code?  Existing drivers all work
> properly because the reference counting of urbs is already present, why
> add duplicate counters?  That actually leads to bugs.  I don't see the
> need for this code, please explain it better if you wish for it to be
> accepted.  What has it found/fixed that we have not found yet?  What
> does it allow you to do that you can't do today with the existing code?
> 
> thanks,
> 
> greg k-h
>
I agree with you two now after checking all urb code. I am sorry to disturb you.
I did have an object lifetime issue, but it is of usb_request. I expect the
cause is usb_request is freed but still pending in udc core. Then I cannot
reproduce it, I even cannot know which function driver the usb_request came
from. I originally want to use debugojects to debug that issue, then I though
this can also help urb debugging.  Obviously I am wrong. As you said reference
counting of urbs is already present. :)
I may add debugojects for device mode usb_request farther. Sometimes it helps.

Thank you,
Du, Changbin