On Wed, Aug 15, 2012 at 10:31:27AM -0400, Alan Stern wrote:
> On Wed, 15 Aug 2012, Chen Peter-B29397 wrote:
>
> > Hi Alan,
> >
> > One of my colleagues reports a problem that the enumeration will fail if
> > the storage has some problems. I can re-produce this problem if I add a
> > big delay (like 500ms) at handle_exception, at condition FSG_STATE_DISCONNECT,
> > please see below code:
> >
> > 3050 case FSG_STATE_DISCONNECT:
> > 3051 for (i = 0; i < fsg->nluns; ++i)
> > 3052 fsg_lun_fsync_sub(fsg->luns + i);
> > 3053 mdelay(500);
> > 3054 do_set_config(fsg, 0); // Unconfigured state
> > 3055 break;
> >
> > When the problem occurs, the call sequence like below:
> >
> > reset interrupt-> fsg->disconnect-> handle_exception
> > reset interrupt-> fsg->disconnect
> > ...
> > udc setup package -> USB_REQ_SET_CONFIGURATION -> raise_exception(fsg, FSG_STATE_CONFIG_CHANGE);
> >
> > The reason why this problem occurs is that the first fsg_lun_fsync_sub(fsg->luns + i) consumes too
> > much times, the fsg->state is changed by raise_exception (2nd reset interrupt), but the handle_exception
> > (2nd reset interrupt) is not called after raise_exception(fsg, FSG_STATE_CONFIG_CHANGE) is called.
> > The fsg->state is still FSG_STATE_DISCONNECT when raise_exception(fsg, FSG_STATE_CONFIG_CHANGE) is called,
> > Since FSG_STATE_DISCONNECT > FSG_STATE_CONFIG_CHANGE, the SET_CONFIGURATION will never be handled,
> > the enumeration will be failed.
>
> Yes, I see. But that's the right thing to do, isn't it? If the gadget
> is so busy writing data to the backing storage that it can't carry out
> a Set-Config request, then the configuration attempt _should_ fail.
>
> > A workaround for this problem is:
> >
> > 3050 case FSG_STATE_DISCONNECT:
> > 3051 if (fsg->config != 0)
> > 3052 for (i = 0; i < fsg->nluns; ++i)
> > 3053 fsg_lun_fsync_sub(fsg->luns + i);
> > 3054 do_set_config(fsg, 0); // Unconfigured state
> > 3055 break;
> >
> > But it can't handle the problem that the reset occurs during the transfer due to some timeout.
> > (bus reset -> bus reset -> SET_CONFIGURATION)
>
> Indeed. Well, if the gadget is too slow to keep up with the host,
> eventually it will stop working no matter what the driver does.
> However note that the second time it gets called, fsg_lun_fsync_sub()
> should run very quickly because all the dirty pages will already have
> been written out.
>
>
> A better solution would be to have separate gadget driver callbacks for
> Reset and Disconnect. Then we could do the fsg_lun_fsync_sub() only
> during disconnect, not during reset.
>
> Alternatively, we could keep the single existing Disconnect callback
> and give the gadget driver a way to ask whether or not VBus power is
> present. Then we could do the fsg_lun_fsync_sub() only when there is
> no power.
>
> Either one of these would be a major change to the Gadget API, but they
> seem like reasonable things to add.
>
> Felipe, what do you think?
I would feel better about splitting disconnect from reset. We already
have a weird vbus_session callback which is actually the other way
around (it's used by a gadget driver to tell controller vbus is alive,
go figure). It should be fairly simple to implement and have less impact
on the controllers themselves. But as you say, it'll be quite invasive,
so we should try to make sure that gets properly tested.
I could keep it in my tree on a separate branch and only send upstream
if I get enough Acked-bys. How does that sound ?
FYI, dwc3 would look like this:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e09a7c4..723a530 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1846,6 +1846,15 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
}
}
+static void dwc3_reset_gadget(struct dwc3 *dwc)
+{
+ if (dwc->gadget_driver && dwc->gadget_driver->reset) {
+ spin_unlock(&dwc->lock);
+ dwc->gadget_driver->reset(&dwc->gadget);
+ spin_lock(&dwc->lock);
+ }
+}
+
static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum)
{
struct dwc3_ep *dep;
@@ -2021,8 +2030,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
dwc3_gadget_usb3_phy_suspend(dwc, false);
}
- if (dwc->gadget.speed != USB_SPEED_UNKNOWN)
- dwc3_disconnect_gadget(dwc);
+ dwc3_reset_gadget(dwc);
reg = dwc3_readl(dwc->regs, DWC3_DCTL);
reg &= ~DWC3_DCTL_TSTCTRL_MASK;
When doing that, I thought we could also drop the "don't call disconnect
if we get a reset when gadget never enumerated" thing. What do you say ?
--
balbi
Attachment:
signature.asc
Description: Digital signature
