| Message ID | 20240119054813.2851201-1-yuanlinyu@hihonor.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] usb: udc: run disconnect callback before pull up zero | expand |
On Fri, Jan 19, 2024 at 01:48:13PM +0800, yuan linyu wrote: > When write UDC to empty and unbind gadget driver from gadget device, it is > possible that there are many queue failures for mass storage function. > > The root cause is on platform like dwc3, if pull down called first, the > queue operation from mass storage main thread will fail as it is belong to > another thread context and always try to receive a command from host. > > In order to fix it, call gadget driver disconnect callback first, mass > storage function driver will disable endpoints and clear running flag, > so there will be no request queue to UDC. > > One note is when call disconnect callback first, it mean function will > disable endpoints before stop UDC controller. Exactly. So instead of getting a bunch of errors on the gadget, now you'll get a bunch of errors on the host. I don't think that's any better. Why don't you change the dwc3 driver instead? If it allowed ep_queue operations to succeed while the pull-up is off then this problem would go away. Alan Stern
On Fri, Jan 19, 2024, Alan Stern wrote: > On Fri, Jan 19, 2024 at 01:48:13PM +0800, yuan linyu wrote: > > When write UDC to empty and unbind gadget driver from gadget device, it is > > possible that there are many queue failures for mass storage function. That's expected right? > > > > The root cause is on platform like dwc3, if pull down called first, the > > queue operation from mass storage main thread will fail as it is belong to > > another thread context and always try to receive a command from host. > > > > In order to fix it, call gadget driver disconnect callback first, mass > > storage function driver will disable endpoints and clear running flag, > > so there will be no request queue to UDC. > > > > One note is when call disconnect callback first, it mean function will > > disable endpoints before stop UDC controller. > > Exactly. So instead of getting a bunch of errors on the gadget, now > you'll get a bunch of errors on the host. I don't think that's any > better. > > Why don't you change the dwc3 driver instead? If it allowed ep_queue > operations to succeed while the pull-up is off then this problem would > go away. > I don't think we should do that either. When pullup off occurs, the device is disconnected for dwc3. usb_ep_queue() doc noted that we should return error on disconnection. Beside, it will add unnecessary complication to dwc3 handling this. BR, Thinh
On Sat, Jan 20, 2024 at 01:12:04AM +0000, Thinh Nguyen wrote: > On Fri, Jan 19, 2024, Alan Stern wrote: > > On Fri, Jan 19, 2024 at 01:48:13PM +0800, yuan linyu wrote: > > > When write UDC to empty and unbind gadget driver from gadget device, it is > > > possible that there are many queue failures for mass storage function. > > That's expected right? Certainly. And not just for mass storage; for other gadget drivers too. > > > The root cause is on platform like dwc3, if pull down called first, the > > > queue operation from mass storage main thread will fail as it is belong to > > > another thread context and always try to receive a command from host. > > > > > > In order to fix it, call gadget driver disconnect callback first, mass > > > storage function driver will disable endpoints and clear running flag, > > > so there will be no request queue to UDC. > > > > > > One note is when call disconnect callback first, it mean function will > > > disable endpoints before stop UDC controller. > > > > Exactly. So instead of getting a bunch of errors on the gadget, now > > you'll get a bunch of errors on the host. I don't think that's any > > better. > > > > Why don't you change the dwc3 driver instead? If it allowed ep_queue > > operations to succeed while the pull-up is off then this problem would > > go away. > > > > I don't think we should do that either. When pullup off occurs, the > device is disconnected for dwc3. usb_ep_queue() doc noted that we > should return error on disconnection. Oh yes, so it does. Okay, forget that idea. > Beside, it will add unnecessary > complication to dwc3 handling this. How about instead just reducing the visibility of these error messages to make them less annoying? They aren't very important, after all -- they don't indicate that anything has gone seriously wrong. Alan Stern
> From: Alan Stern <stern@rowland.harvard.edu> > Sent: Saturday, January 20, 2024 11:00 PM > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Cc: yuanlinyu 00030184 <yuanlinyu@hihonor.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org > Subject: Re: [RFC PATCH] usb: udc: run disconnect callback before pull up zero > > On Sat, Jan 20, 2024 at 01:12:04AM +0000, Thinh Nguyen wrote: > > On Fri, Jan 19, 2024, Alan Stern wrote: > > > On Fri, Jan 19, 2024 at 01:48:13PM +0800, yuan linyu wrote: > > > > When write UDC to empty and unbind gadget driver from gadget device, it > is > > > > possible that there are many queue failures for mass storage function. > > > > That's expected right? > > Certainly. And not just for mass storage; for other gadget drivers too. > > > > > The root cause is on platform like dwc3, if pull down called first, the > > > > queue operation from mass storage main thread will fail as it is belong to > > > > another thread context and always try to receive a command from host. > > > > > > > > In order to fix it, call gadget driver disconnect callback first, mass > > > > storage function driver will disable endpoints and clear running flag, > > > > so there will be no request queue to UDC. > > > > > > > > One note is when call disconnect callback first, it mean function will > > > > disable endpoints before stop UDC controller. > > > > > > Exactly. So instead of getting a bunch of errors on the gadget, now > > > you'll get a bunch of errors on the host. I don't think that's any What is host error ? Seem when host do nothing, but mass storage driver try to queue request which want to receive command from host. > > > better. > > > > > > Why don't you change the dwc3 driver instead? If it allowed ep_queue > > > operations to succeed while the pull-up is off then this problem would > > > go away. > > > > > > > I don't think we should do that either. When pullup off occurs, the > > device is disconnected for dwc3. usb_ep_queue() doc noted that we > > should return error on disconnection. > > Oh yes, so it does. Okay, forget that idea. > > > Beside, it will add unnecessary > > complication to dwc3 handling this. > > How about instead just reducing the visibility of these error messages It is already change to dev_dbg() by Wesley Cheng for dwc3. But it also can enable by change log level. Only delete it will not show again. this thread just want to discuss if disable eps first then pull up zero acceptable or good (reduce mode switch time ???) for all UDCs ? > to make them less annoying? They aren't very important, after all -- > they don't indicate that anything has gone seriously wrong. > > Alan Stern
On Mon, Jan 22, 2024 at 01:32:39AM +0000, yuanlinyu 00030184 wrote: > > From: Alan Stern <stern@rowland.harvard.edu> > > Sent: Saturday, January 20, 2024 11:00 PM > > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Cc: yuanlinyu 00030184 <yuanlinyu@hihonor.com>; Greg Kroah-Hartman > > <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org > > Subject: Re: [RFC PATCH] usb: udc: run disconnect callback before pull up zero > > > > On Sat, Jan 20, 2024 at 01:12:04AM +0000, Thinh Nguyen wrote: > > > On Fri, Jan 19, 2024, Alan Stern wrote: > > > > On Fri, Jan 19, 2024 at 01:48:13PM +0800, yuan linyu wrote: > > > > > When write UDC to empty and unbind gadget driver from gadget device, it > > is > > > > > possible that there are many queue failures for mass storage function. > > > > > > That's expected right? > > > > Certainly. And not just for mass storage; for other gadget drivers too. > > > > > > > The root cause is on platform like dwc3, if pull down called first, the > > > > > queue operation from mass storage main thread will fail as it is belong to > > > > > another thread context and always try to receive a command from host. > > > > > > > > > > In order to fix it, call gadget driver disconnect callback first, mass > > > > > storage function driver will disable endpoints and clear running flag, > > > > > so there will be no request queue to UDC. > > > > > > > > > > One note is when call disconnect callback first, it mean function will > > > > > disable endpoints before stop UDC controller. > > > > > > > > Exactly. So instead of getting a bunch of errors on the gadget, now > > > > you'll get a bunch of errors on the host. I don't think that's any > > What is host error ? If the host is trying to communicate with the gadget at the time of the mode switch, it will see that something is wrong because the gadget will still be connected (the pullup will still be on) but it won't reply to any USB packets. > Seem when host do nothing, but mass storage driver try to queue request > which want to receive command from host. That's true. If the host is not trying to communicate with the gadget, it won't get any errors. > > > > better. > > > > > > > > Why don't you change the dwc3 driver instead? If it allowed ep_queue > > > > operations to succeed while the pull-up is off then this problem would > > > > go away. > > > > > > > > > > I don't think we should do that either. When pullup off occurs, the > > > device is disconnected for dwc3. usb_ep_queue() doc noted that we > > > should return error on disconnection. > > > > Oh yes, so it does. Okay, forget that idea. > > > > > Beside, it will add unnecessary > > > complication to dwc3 handling this. > > > > How about instead just reducing the visibility of these error messages > > It is already change to dev_dbg() by Wesley Cheng for dwc3. > But it also can enable by change log level. Only delete it will not show again. I think it's good to have those messages show up when the log level is set to debug. They let developers know what's happening when they test changes to the gadget drivers. If the messages really bother somebody, all they have to do is reduce the log level to info. If you're still concerned about the behavior of the mass-storage function, you could change it. Make it disable itself whenever it gets a -ESHUTDOWN error, either while submitting a request or as a completion status. This should reduce the number of error messages, although it won't eliminate them. (Of course, this still leaves the possibility of floods of debugging messages from all the other function drivers...) > this thread just want to discuss if disable eps first then pull up zero acceptable or > good (reduce mode switch time ???) for all UDCs ? I think it's not a good thing to do. And I don't think it will reduce the mode switch time, because both operations still have to occur so they will require the same total amount of time. Alan Stern
> From: Alan Stern <stern@rowland.harvard.edu> > Sent: Monday, January 22, 2024 10:31 AM > To: yuan linyu <yuanlinyu@hihonor.com> > Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org > Subject: Re: [RFC PATCH] usb: udc: run disconnect callback before pull up zero > > > > > What is host error ? > > If the host is trying to communicate with the gadget at the time of the > mode switch, it will see that something is wrong because the gadget will > still be connected (the pullup will still be on) but it won't reply to > any USB packets. > Host will lost device soon. > > > > It is already change to dev_dbg() by Wesley Cheng for dwc3. > > But it also can enable by change log level. Only delete it will not show again. > > I think it's good to have those messages show up when the log level is > set to debug. They let developers know what's happening when they test > changes to the gadget drivers. If the messages really bother somebody, > all they have to do is reduce the log level to info. > > If you're still concerned about the behavior of the mass-storage > function, you could change it. Make it disable itself whenever it gets > a -ESHUTDOWN error, either while submitting a request or as a completion > status. This should reduce the number of error messages, although it > won't eliminate them. Thanks, will send a patch for mass storage only. > > (Of course, this still leaves the possibility of floods of debugging > messages from all the other function drivers...) > > > this thread just want to discuss if disable eps first then pull up zero acceptable > or > > good (reduce mode switch time ???) for all UDCs ? > > I think it's not a good thing to do. And I don't think it will reduce > the mode switch time, because both operations still have to occur so > they will require the same total amount of time. > > Alan Stern
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index d59f94464b87..9c48cec89e35 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -774,15 +774,15 @@ static int usb_gadget_disconnect_locked(struct usb_gadget *gadget) goto out; } - ret = gadget->ops->pullup(gadget, 0); - if (!ret) - gadget->connected = 0; - mutex_lock(&udc_lock); if (gadget->udc->driver) gadget->udc->driver->disconnect(gadget); mutex_unlock(&udc_lock); + ret = gadget->ops->pullup(gadget, 0); + if (!ret) + gadget->connected = 0; + out: trace_usb_gadget_disconnect(gadget, ret);
When write UDC to empty and unbind gadget driver from gadget device, it is possible that there are many queue failures for mass storage function. The root cause is on platform like dwc3, if pull down called first, the queue operation from mass storage main thread will fail as it is belong to another thread context and always try to receive a command from host. In order to fix it, call gadget driver disconnect callback first, mass storage function driver will disable endpoints and clear running flag, so there will be no request queue to UDC. One note is when call disconnect callback first, it mean function will disable endpoints before stop UDC controller. Signed-off-by: yuan linyu <yuanlinyu@hihonor.com> --- drivers/usb/gadget/udc/core.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)