Hi Jack, Jack Pham <jackp@xxxxxxxxxxxxxx> writes: >> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> > index 804b50548163..c647c76d7361 100644 >> > --- a/drivers/usb/dwc3/gadget.c >> > +++ b/drivers/usb/dwc3/gadget.c >> > @@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> > if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) >> > return 0; >> > >> > + /* bail if already resized */ >> > + if (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1))) >> > + return 0; >> > + >> >> heh, not to say "I told you so", but... >> >> That being said, your test is not very good. The whole idea for resizing >> the FIFOs is that in some applications we only use e.g. 2 endpoints and >> there is considerable FIFO space left unused. >> >> The goal is to use that unused FIFO space to squeeze more throughput out >> of the pipe, since it amortizes SW latency. >> >> This patch is essentially the same as reverting the original commit :-) > > No, it's not quite the same as nullifying the resizing algorithm. This > patch is predicated on a key part of the resizing algorithm where > dwc3_gadget_clear_tx_fifos() occurs upon receiving Set_Configuration in > ep0.c. Which means that each new connection starts off with a blank > slate with all the GTXFIFOSIZ(n) registers cleared. Then each EP gets > resized one at a time when usb_ep_enable() is called. > > The problem this patch is fixing is avoiding *re-resizing*, the idea > being that if an EP was already resized once during a session (from > Set Configuration until the next reset or disconnect), then > it should be good to go even if it gets disabled and re-enabled again. that's not a safe assumption, though. What happens in cases where Configuration 1 is wildly different from Configuration 2? Say Config 1 is a mass storage device and Config 2 is a collection of several CDC interfaces? > Since we lack any boolean state variable in struct dwc3_ep reflecting > whether it had already been resized, re-reading the GTXFIFOSIZ register it might be a better idea to introduce such a flag and make the intention clear. But in any case, I still think the assumption you're making is not very good. > is the next best equivalent. Note also that this check occurs after > the if (!dwc->do_fifo_resize) check so this is applicable only if the > entire "tx-fifo-resize" mechanism is enabled. Right, that's fine :-) -- balbi
