On Tue, 24 May 2016 changbin...@intel.com wrote:
> From: "Du, Changbin" <changbin...@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...@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
--
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
