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

[Alan Stern]

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

> From: "Du, Changbin" <changbin.du@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
>  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