usb: core: add debugobjects support for urb object
Subsystems (2)
PATCH 1/1 usb: core: add debugobjects support for urb object
From: "Du, Changbin" <[email protected]>
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.
--- 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(-)
drivers/usb/core/hcd.c +1 -0
@@ -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
drivers/usb/core/urb.c +113 -4
162 lines not on this page · raw message
include/linux/usb.h +8 -0
@@ -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);
lib/Kconfig.debug +8 -0
@@ -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
Signature · 1 line
2.7.4
Quoted 5 lines · On Tue, 24 May 2016 [email protected] wrote:
From: "Du, Changbin" <[email protected]>
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?
Quoted 2 lines
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?
Quoted 48 lines
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 <[email protected]>--- 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(-)drivers/usb/core/hcd.c +1
-0@@ -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 drivers/usb/core/urb.c +15
-0@@ -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;
drivers/usb/core/urb.c line 37
Quoted 10 lines
drivers/usb/core/urb.c +10
-0+ +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.
Quoted 11 lines
drivers/usb/core/urb.c +6
-1@@ -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);
drivers/usb/core/urb.c int usb_submit_urb(struct urb *urb, gfp_t mem_flags) · line 653
Quoted 10 lines
drivers/usb/core/urb.c +7
-0+ +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
Greg KH
maintainerQuoted 7 lines · On Tue, May 24, 2016 at 03:53:53PM +0800, [email protected] wrote:
From: "Du, Changbin" <[email protected]>
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.
Quoted 5 lines
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 <[email protected]>
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
Quoted 21 lines · On Tue, May 24, 2016 at 03:53:53PM +0800, [email protected] wrote:
Quoted 7 lines
From: "Du, Changbin" <[email protected]>
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.
Quoted 5 lines
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 <[email protected]>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?
Quoted 4 lines
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