[RFC PATCH 1/2] usb: urb: show pipe information of warning message in usb_submit_urb()

[RFC PATCH 1/2] usb: urb: show pipe information of warning message in usb_submit_urb()

[Linyu Yuan]

it will show the urb information for debugging.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/core/urb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 9f3c540..b258203 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -376,7 +376,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	if (!urb || !urb->complete)
 		return -EINVAL;
 	if (urb->hcpriv) {
-		WARN_ONCE(1, "URB %pK submitted while active\n", urb);
+		WARN_ONCE(1, "URB %pK pipe %x submitted while active\n", urb, urb->pipe);
 		return -EBUSY;
 	}
 
-- 
2.7.4

[RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Linyu Yuan]

When start probe hub, during INIT, INTT2, INIT3 stage, when link state
change to inactive, currently it will reset the device, maybe it will
trigger warning in usb_submit_urb() due to urb->hcpriv is still active.

Add a flag name init_stage to avoid reset the device.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/core/hub.c | 20 +++++++++++++++++++-
 drivers/usb/core/hub.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 97a0f8f..3cb1137 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1290,6 +1290,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 	if (type == HUB_INIT2 || type == HUB_INIT3) {
 		/* Allow autosuspend if it was suppressed */
  disconnected:
+		hub->init_stage = 0;
 		usb_autopm_put_interface_async(to_usb_interface(hub->intfdev));
 		device_unlock(&hdev->dev);
 	}
@@ -1872,6 +1873,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	kref_init(&hub->kref);
 	hub->intfdev = &intf->dev;
 	hub->hdev = hdev;
+	hub->init_stage = 1;
 	INIT_DELAYED_WORK(&hub->leds, led_work);
 	INIT_DELAYED_WORK(&hub->init_work, NULL);
 	INIT_WORK(&hub->events, hub_event);
@@ -5587,6 +5589,21 @@ static void port_over_current_notify(struct usb_port *port_dev)
 	kfree(port_dev_path);
 }
 
+static bool port_child_avoid_reset(struct usb_device *udev)
+{
+	struct usb_hub *hub;
+
+	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB &&
+	    udev->state == USB_STATE_CONFIGURED) {
+		hub = usb_get_intfdata(udev->actconfig->interface[0]);
+
+		if (hub && hub->init_stage)
+			return true;
+	}
+
+	return false;
+}
+
 static void port_event(struct usb_hub *hub, int port1)
 		__must_hold(&port_dev->status_lock)
 {
@@ -5699,7 +5716,8 @@ static void port_event(struct usb_hub *hub, int port1)
 			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
 			usb_unlock_port(port_dev);
 			usb_lock_device(udev);
-			usb_reset_device(udev);
+			if (!port_child_avoid_reset(udev))
+				usb_reset_device(udev);
 			usb_unlock_device(udev);
 			usb_lock_port(port_dev);
 			connect_change = 0;
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index e238335..040524f 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -66,6 +66,7 @@ struct usb_hub {
 	unsigned		quirk_check_port_auto_suspend:1;
 
 	unsigned		has_indicators:1;
+	unsigned		init_stage:1;
 	u8			indicator[USB_MAXCHILDREN];
 	struct delayed_work	leds;
 	struct delayed_work	init_work;
-- 
2.7.4

Re: [RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Greg Kroah-Hartman]

On Fri, Mar 03, 2023 at 05:28:38PM +0800, Linyu Yuan wrote:
> When start probe hub, during INIT, INTT2, INIT3 stage, when link state
> change to inactive, currently it will reset the device, maybe it will
> trigger warning in usb_submit_urb() due to urb->hcpriv is still active.

I am sorry, but I do not understand this text at all.  Can you reword
it?

> Add a flag name init_stage to avoid reset the device.

I do not understand, what is "flag name"?

thanks,

greg k-h

Re: [RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Alan Stern]

On Fri, Mar 03, 2023 at 05:28:38PM +0800, Linyu Yuan wrote:
> When start probe hub, during INIT, INTT2, INIT3 stage, when link state
> change to inactive, currently it will reset the device, maybe it will
> trigger warning in usb_submit_urb() due to urb->hcpriv is still active.

You need to explain this in much greater detail.

	What will reset the device?

	What is the code path for this reset?

	Why will urb->hcpriv still be active?

> Add a flag name init_stage to avoid reset the device.

Why do you want to avoid resetting the device?

Doesn't the reset code already include a check for whether the device is 
disconnected?

> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>  drivers/usb/core/hub.c | 20 +++++++++++++++++++-
>  drivers/usb/core/hub.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 97a0f8f..3cb1137 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1290,6 +1290,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  	if (type == HUB_INIT2 || type == HUB_INIT3) {
>  		/* Allow autosuspend if it was suppressed */
>   disconnected:
> +		hub->init_stage = 0;
>  		usb_autopm_put_interface_async(to_usb_interface(hub->intfdev));
>  		device_unlock(&hdev->dev);
>  	}
> @@ -1872,6 +1873,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	kref_init(&hub->kref);
>  	hub->intfdev = &intf->dev;
>  	hub->hdev = hdev;
> +	hub->init_stage = 1;
>  	INIT_DELAYED_WORK(&hub->leds, led_work);
>  	INIT_DELAYED_WORK(&hub->init_work, NULL);
>  	INIT_WORK(&hub->events, hub_event);
> @@ -5587,6 +5589,21 @@ static void port_over_current_notify(struct usb_port *port_dev)
>  	kfree(port_dev_path);
>  }
>  
> +static bool port_child_avoid_reset(struct usb_device *udev)
> +{
> +	struct usb_hub *hub;
> +
> +	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB &&
> +	    udev->state == USB_STATE_CONFIGURED) {
> +		hub = usb_get_intfdata(udev->actconfig->interface[0]);
> +
> +		if (hub && hub->init_stage)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static void port_event(struct usb_hub *hub, int port1)
>  		__must_hold(&port_dev->status_lock)
>  {
> @@ -5699,7 +5716,8 @@ static void port_event(struct usb_hub *hub, int port1)
>  			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
>  			usb_unlock_port(port_dev);
>  			usb_lock_device(udev);
> -			usb_reset_device(udev);
> +			if (!port_child_avoid_reset(udev))
> +				usb_reset_device(udev);
>  			usb_unlock_device(udev);

Doesn't usb_lock_device() already prevent this code from running during 
the INIT, INIT2, and INIT3 stages of hub preparation?

Alan Stern

Re: [RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Linyu Yuan]

On 3/4/2023 12:05 AM, Alan Stern wrote:
> On Fri, Mar 03, 2023 at 05:28:38PM +0800, Linyu Yuan wrote:
>> When start probe hub, during INIT, INTT2, INIT3 stage, when link state
>> change to inactive, currently it will reset the device, maybe it will
>> trigger warning in usb_submit_urb() due to urb->hcpriv is still active.
> You need to explain this in much greater detail.
>
> 	What will reset the device?
>
> 	What is the code path for this reset?

will share more code path.


>
> 	Why will urb->hcpriv still be active?


still can't explain, that's why add patch#1 to get more urb infol


>> Add a flag name init_stage to avoid reset the device.
> Why do you want to avoid resetting the device?


at INIT stage, external hub still under enumeration process, i think 
there is no need to reset.


>
> Doesn't the reset code already include a check for whether the device is
> disconnected?


the problem is port is inactive state, but device still in software 
connect state,

there is no disconnect check in reset code.


>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>>   drivers/usb/core/hub.c | 20 +++++++++++++++++++-
>>   drivers/usb/core/hub.h |  1 +
>>   2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 97a0f8f..3cb1137 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -1290,6 +1290,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>>   	if (type == HUB_INIT2 || type == HUB_INIT3) {
>>   		/* Allow autosuspend if it was suppressed */
>>    disconnected:
>> +		hub->init_stage = 0;
>>   		usb_autopm_put_interface_async(to_usb_interface(hub->intfdev));
>>   		device_unlock(&hdev->dev);
>>   	}
>> @@ -1872,6 +1873,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>   	kref_init(&hub->kref);
>>   	hub->intfdev = &intf->dev;
>>   	hub->hdev = hdev;
>> +	hub->init_stage = 1;
>>   	INIT_DELAYED_WORK(&hub->leds, led_work);
>>   	INIT_DELAYED_WORK(&hub->init_work, NULL);
>>   	INIT_WORK(&hub->events, hub_event);
>> @@ -5587,6 +5589,21 @@ static void port_over_current_notify(struct usb_port *port_dev)
>>   	kfree(port_dev_path);
>>   }
>>   
>> +static bool port_child_avoid_reset(struct usb_device *udev)
>> +{
>> +	struct usb_hub *hub;
>> +
>> +	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB &&
>> +	    udev->state == USB_STATE_CONFIGURED) {
>> +		hub = usb_get_intfdata(udev->actconfig->interface[0]);
>> +
>> +		if (hub && hub->init_stage)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static void port_event(struct usb_hub *hub, int port1)
>>   		__must_hold(&port_dev->status_lock)
>>   {
>> @@ -5699,7 +5716,8 @@ static void port_event(struct usb_hub *hub, int port1)
>>   			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
>>   			usb_unlock_port(port_dev);
>>   			usb_lock_device(udev);
>> -			usb_reset_device(udev);
>> +			if (!port_child_avoid_reset(udev))
>> +				usb_reset_device(udev);
>>   			usb_unlock_device(udev);
> Doesn't usb_lock_device() already prevent this code from running during
> the INIT, INIT2, and INIT3 stages of hub preparation?


as it use some delay worker to complete the INIT stage, as i know it 
will not lock device

when worker is not start.

do you have any better suggestion about this point ?


>
> Alan Stern

Re: [RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Alan Stern]

On Wed, Mar 08, 2023 at 01:54:15PM +0800, Linyu Yuan wrote:
> 
> On 3/4/2023 12:05 AM, Alan Stern wrote:
> > On Fri, Mar 03, 2023 at 05:28:38PM +0800, Linyu Yuan wrote:
> > > When start probe hub, during INIT, INTT2, INIT3 stage, when link state
> > > change to inactive, currently it will reset the device, maybe it will
> > > trigger warning in usb_submit_urb() due to urb->hcpriv is still active.
> > You need to explain this in much greater detail.
> > 
> > 	What will reset the device?
> > 
> > 	What is the code path for this reset?
> 
> will share more code path.
> 
> 
> > 
> > 	Why will urb->hcpriv still be active?
> 
> 
> still can't explain, that's why add patch#1 to get more urb infol
> 
> 
> > > Add a flag name init_stage to avoid reset the device.
> > Why do you want to avoid resetting the device?
> 
> 
> at INIT stage, external hub still under enumeration process, i think there
> is no need to reset.
> 
> 
> > 
> > Doesn't the reset code already include a check for whether the device is
> > disconnected?
> 
> 
> the problem is port is inactive state, but device still in software connect
> state,
> 
> there is no disconnect check in reset code.
> 
> 
> > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---

> > > @@ -5699,7 +5716,8 @@ static void port_event(struct usb_hub *hub, int port1)
> > >   			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
> > >   			usb_unlock_port(port_dev);
> > >   			usb_lock_device(udev);
> > > -			usb_reset_device(udev);
> > > +			if (!port_child_avoid_reset(udev))
> > > +				usb_reset_device(udev);
> > >   			usb_unlock_device(udev);
> > Doesn't usb_lock_device() already prevent this code from running during
> > the INIT, INIT2, and INIT3 stages of hub preparation?
> 
> 
> as it use some delay worker to complete the INIT stage, as i know it will
> not lock device
> 
> when worker is not start.
> 
> do you have any better suggestion about this point ?

I can't offer any suggestions because I don't understand the problem you 
want to fix, or how your patch is meant to work.

Alan Stern

Re: [RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Linyu Yuan]

On 3/9/2023 12:04 AM, Alan Stern wrote:
> On Wed, Mar 08, 2023 at 01:54:15PM +0800, Linyu Yuan wrote:
>> On 3/4/2023 12:05 AM, Alan Stern wrote:
>>> On Fri, Mar 03, 2023 at 05:28:38PM +0800, Linyu Yuan wrote:
>>>> When start probe hub, during INIT, INTT2, INIT3 stage, when link state
>>>> change to inactive, currently it will reset the device, maybe it will
>>>> trigger warning in usb_submit_urb() due to urb->hcpriv is still active.
>>> You need to explain this in much greater detail.
>>>
>>> 	What will reset the device?
>>>
>>> 	What is the code path for this reset?
>> will share more code path.
>>
>>
>>> 	Why will urb->hcpriv still be active?
>>
>> still can't explain, that's why add patch#1 to get more urb infol
>>
>>
>>>> Add a flag name init_stage to avoid reset the device.
>>> Why do you want to avoid resetting the device?
>>
>> at INIT stage, external hub still under enumeration process, i think there
>> is no need to reset.
>>
>>
>>> Doesn't the reset code already include a check for whether the device is
>>> disconnected?
>>
>> the problem is port is inactive state, but device still in software connect
>> state,
>>
>> there is no disconnect check in reset code.
>>
>>
>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>> ---
>>>> @@ -5699,7 +5716,8 @@ static void port_event(struct usb_hub *hub, int port1)
>>>>    			dev_dbg(&port_dev->dev, "do warm reset, full device\n");
>>>>    			usb_unlock_port(port_dev);
>>>>    			usb_lock_device(udev);
>>>> -			usb_reset_device(udev);
>>>> +			if (!port_child_avoid_reset(udev))
>>>> +				usb_reset_device(udev);
>>>>    			usb_unlock_device(udev);
>>> Doesn't usb_lock_device() already prevent this code from running during
>>> the INIT, INIT2, and INIT3 stages of hub preparation?
>>
>> as it use some delay worker to complete the INIT stage, as i know it will
>> not lock device
>>
>> when worker is not start.
>>
>> do you have any better suggestion about this point ?
> I can't offer any suggestions because I don't understand the problem you
> want to fix, or how your patch is meant to work.


Just do not think there is any problem,

do you think if we can avoid reset a hub device before it complete 
enumeration ?


>
> Alan Stern

Re: [RFC PATCH 2/2] usb: core: hub: avoid reset hub during probe

[Alan Stern]

On Thu, Mar 09, 2023 at 10:18:22AM +0800, Linyu Yuan wrote:
> 
> On 3/9/2023 12:04 AM, Alan Stern wrote:
> > I can't offer any suggestions because I don't understand the problem you
> > want to fix, or how your patch is meant to work.
> 
> 
> Just do not think there is any problem,

If you don't think there is any problem, why did you submit your 
patches?

> do you think if we can avoid reset a hub device before it complete
> enumeration ?

No.  And I don't think there's any reason to avoid it.

On the other hand, most hubs don't get reset before they are enumerated.

Alan Stern

Re: [RFC PATCH 1/2] usb: urb: show pipe information of warning message in usb_submit_urb()

[Greg Kroah-Hartman]

On Fri, Mar 03, 2023 at 05:28:37PM +0800, Linyu Yuan wrote:
> it will show the urb information for debugging.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>  drivers/usb/core/urb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 9f3c540..b258203 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -376,7 +376,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  	if (!urb || !urb->complete)
>  		return -EINVAL;
>  	if (urb->hcpriv) {
> -		WARN_ONCE(1, "URB %pK submitted while active\n", urb);
> +		WARN_ONCE(1, "URB %pK pipe %x submitted while active\n", urb, urb->pipe);
>  		return -EBUSY;
>  	}
>  
> -- 
> 2.7.4
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what is needed in
  order to properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what a proper
  Subject: line should look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot