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

[Du, Changbin]

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

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

[Du, Changbin]

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

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

[Greg KH]

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

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

[Greg KH]

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

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

[Alan Stern]

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

> From: "Du, Changbin" 
> 
> 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 
> ---
>  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, _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

[Alan Stern]

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

> From: "Du, Changbin" 
> 
> 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 
> ---
>  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, _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