| Message ID | 1677835718-7405-2-git-send-email-quic_linyyuan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/2] usb: urb: show pipe information of warning message in usb_submit_urb() | expand |
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
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
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
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
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
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
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;
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(-)