Re: [PATCH] usb: dwc2: host: use hrtimer for NAK retries
Hi, On Tue, Aug 28, 2018 at 11:04 AM, Terin Stock wrote: >> It would also be good to document what device you were plugging in >> that you were having problems with and what system you were running >> on. That would help someone else if they ever wanted to modify the >> same area of code and re-test. They'd have a better chance of not >> re-breaking you. > > This is a rather generic USB floppy drive. Is there a preference of > vid and pid and/or product and manufacturer strings? Whichever seems better to you is probably fine. This just gives folks a clue at trying to replicate your setup. ;-) >> Overall nit: please CC LKML on patches. If nothing else that makes >> them easier to find in lore.kernel.org/patchwork > > To be clear, is this CC on the email envelope? Don't add "Cc:" of LKML in the patch description itself, just make sure it's CCed on the message in git-send-email. I'll put in my usual plug for considering "patman" to help post patches. That calls get_maintainer for you which always suggests CCing LKML. http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README
Re: [PATCH] usb: dwc2: host: use hrtimer for NAK retries
Thanks Alan and Doug for your feedback. They've both been extremely helpful in understanding what you're looking for in messages. A few responses below. > I think your commit will be more compelling with additional data. As > Allen says it looks like you're not actually changing the delay. You > could include things like: > > * On systems with 100 HZ in the ideal case we'd end up delaying for 10 > ms - 20 ms when we used jiffies. Now we'll get much closer to 1 ms. > > * Timer functions are not very high priority, so if we were running at > a high load then we'd sometimes see much longer delays. (NOTE: if you > say this then please back it up with data--I think I've heard > anecdotally that the normal timer functions can be quite delayed but I > haven't done the research to back it up). Presumably you could use > ktime to measure delays before and after your patch and you could > include this in the commit message. Yes, this change decreases the delay, from a variable 10-20ms to 1ms. I'll be sure to state this more clearly in the next version. I read Alan's feedback as generally asking for the question. I'll run some tests with ktime between now and then. > It would also be good to document what device you were plugging in > that you were having problems with and what system you were running > on. That would help someone else if they ever wanted to modify the > same area of code and re-test. They'd have a better chance of not > re-breaking you. This is a rather generic USB floppy drive. Is there a preference of vid and pid and/or product and manufacturer strings? > Overall nit: please CC LKML on patches. If nothing else that makes > them easier to find in lore.kernel.org/patchwork To be clear, is this CC on the email envelope? >> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c >> index 301ced1618f8..2d0cfd7f2cfe 100644 >> --- a/drivers/usb/dwc2/hcd_queue.c >> +++ b/drivers/usb/dwc2/hcd_queue.c >> @@ -59,7 +59,7 @@ >> #define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5)) >> >> /* If we get a NAK, wait this long before retrying */ >> -#define DWC2_RETRY_WAIT_DELAY (msecs_to_jiffies(1)) >> +#define DWC2_RETRY_WAIT_DELAY 1*1E6L >> >> /** >> * dwc2_periodic_channel_available() - Checks that a channel is available >> for a >> @@ -1465,9 +1465,9 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg >> *hsotg, >> * >> * @t: Pointer to wait_timer in a qh. >> */ >> -static void dwc2_wait_timer_fn(struct timer_list *t) >> +static enum hrtimer_restart dwc2_wait_timer_fn(struct hrtimer *t) > > nit: please update function docstring to include a "Return:" clause now. Sure, I'll fix this in the next version.
Re: [PATCH] usb: dwc2: host: use hrtimer for NAK retries
Hi,
On Sat, Aug 25, 2018 at 8:45 PM, Terin Stock wrote:
> Upon upgrading a Raspberry Pi 3B-based project from vanilla 4.14,
> attempts to mount a floppy disk in a generic USB floppy drive would hang
> until the floppy drive was removed from the system.
>
> Tracing shows that during mounting the drive produces a large amount of
> NAKed transactions, but would eventually continue. A previous commit
> added a retry delay on NAKed transactions, using jiffies, that results
> in indefinite NAKs in this scenario.
>
> Modify the wait delay utilize the high resolution timer API to allow for
> more accurately scheduled callbacks.
I think your commit will be more compelling with additional data. As
Allen says it looks like you're not actually changing the delay. You
could include things like:
* On systems with 100 HZ in the ideal case we'd end up delaying for 10
ms - 20 ms when we used jiffies. Now we'll get much closer to 1 ms.
* Timer functions are not very high priority, so if we were running at
a high load then we'd sometimes see much longer delays. (NOTE: if you
say this then please back it up with data--I think I've heard
anecdotally that the normal timer functions can be quite delayed but I
haven't done the research to back it up). Presumably you could use
ktime to measure delays before and after your patch and you could
include this in the commit message.
It would also be good to document what device you were plugging in
that you were having problems with and what system you were running
on. That would help someone else if they ever wanted to modify the
same area of code and re-test. They'd have a better chance of not
re-breaking you.
NOTE: it's possible that the problem here is just that the USB device
you're plugging in is not very forgiving to the kernel taking a long
time to talk to it again after the NAK. Having such a long delay here
isn't common and presumably the device you have just doesn't handle
it. Possibly the device is non-compliant (I'm not enough of an expert
on the spec), but it's still nice to try to support it.
> Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right
> away")
> Signed-off-by: Terin Stock
> ---
> drivers/usb/dwc2/hcd.h |2 +-
> drivers/usb/dwc2/hcd_queue.c | 17 ++---
> 2 files changed, 11 insertions(+), 8 deletions(-)
Overall nit: please CC LKML on patches. If nothing else that makes
them easier to find in lore.kernel.org/patchwork
> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
> index 5502a501f516..93483dc37801 100644
> --- a/drivers/usb/dwc2/hcd.h
> +++ b/drivers/usb/dwc2/hcd.h
> @@ -366,7 +366,7 @@ struct dwc2_qh {
> u32 desc_list_sz;
> u32 *n_bytes;
> struct timer_list unreserve_timer;
> - struct timer_list wait_timer;
> + struct hrtimer wait_timer;
> struct dwc2_tt *dwc_tt;
> int ttport;
> unsigned tt_buffer_dirty:1;
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index 301ced1618f8..2d0cfd7f2cfe 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -59,7 +59,7 @@
> #define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5))
>
> /* If we get a NAK, wait this long before retrying */
> -#define DWC2_RETRY_WAIT_DELAY (msecs_to_jiffies(1))
> +#define DWC2_RETRY_WAIT_DELAY 1*1E6L
>
> /**
> * dwc2_periodic_channel_available() - Checks that a channel is available
> for a
> @@ -1465,9 +1465,9 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg
> *hsotg,
> *
> * @t: Pointer to wait_timer in a qh.
> */
> -static void dwc2_wait_timer_fn(struct timer_list *t)
> +static enum hrtimer_restart dwc2_wait_timer_fn(struct hrtimer *t)
nit: please update function docstring to include a "Return:" clause now.
Other than the above things look pretty good here to me. Thanks for the patch!
-Doug
Re: usb: gadget: f_mass_storage: detect eject
On Tue, 28 Aug 2018, Nuno Gonçalves wrote: > On Mon, Aug 20, 2018 at 4:23 PM Alan Stern wrote: > > It is expected. No notifications for host-initiated ejects were ever > > put into the f_mass_storage driver. > > > > I have never tried to use f_mass_storage under configfs. When you do, > > does the driver create its normal sysfs files? > > Is there any other way to use it? Isn't config fs the way to > instantiate GadgetFS/FunctionFS modules (f_mass_storage)? There is another way: modprobe g_file_storage with appropriate parameters. > The only other files I find in sysfs are the ones referring to the > module being loaded, but those do not change with eject. > > What should I look for? Somewhere under /sys/devices will be a directory for the system's USB device controller. Beneath that will be the gadget directory for f_mass_storage, and below that will be directories for the individual Logical Units. Each LUN will have a sysfs entry named "file", which contains the name of the file or device being used as the backing store. Alan Stern
[PATCH] USB: OHCI: Remove USB bus reset delay from OHCI handover code
Paul pointed out that the 50-ms sleep during OHCI initialization takes
up a large fraction of a system's boot time. Things get worse when
there are two OHCI controllers present, each requiring 50 ms.
However, there really is no need to send a 50-ms reset signal out all
the root-hub ports during initialization. The ports themselves will
be disabled, and the only way to enable a port is to reset it.
Therefore all attached USB devices will receive a proper reset in any
case. The controller reset does not need to be long enough to reset
those other devices, so the 50-ms delay isn't necessary.
Without the delay, there is no remaining incentive for skipping the
reset when the controller is already in the RESET state. This patch
removes the test, issuing the command unconditionally, and removes the
following delay.
Signed-off-by: Alan Stern
Suggested-by: Paul Menzel
Tested-by: Paul Menzel
---
[as1876]
drivers/usb/host/pci-quirks.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
Index: usb-4.x/drivers/usb/host/pci-quirks.c
===
--- usb-4.x.orig/drivers/usb/host/pci-quirks.c
+++ usb-4.x/drivers/usb/host/pci-quirks.c
@@ -783,15 +783,9 @@ static void quirk_usb_handoff_ohci(struc
/* disable interrupts */
writel((u32) ~0, base + OHCI_INTRDISABLE);
- /* Reset the USB bus, if the controller isn't already in RESET */
- if (control & OHCI_HCFS) {
- /* Go into RESET, preserving RWC (and possibly IR) */
- writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
- readl(base + OHCI_CONTROL);
-
- /* drive bus reset for at least 50 ms (7.1.7.5) */
- msleep(50);
- }
+ /* Go into the USB_RESET state, preserving RWC (and possibly IR) */
+ writel(control & OHCI_CTRL_MASK, base + OHCI_CONTROL);
+ readl(base + OHCI_CONTROL);
/* software reset of the controller, preserving HcFmInterval */
if (!no_fminterval)
Re: [RFT] Remove USB bus reset delay from OHCI handover code
On Sat, 25 Aug 2018, Paul Menzel wrote: > Dear Alan, > > > Am 03.07.2018 um 20:40 schrieb Alan Stern: > > I no longer have suitable hardware for testing this patch. If anyone > > with an OHCI host controller could try it out, I would like to hear if > > it causes any problems. > > Do you already have plans how to proceed with this? Did you get any replies? No response. I'll go ahead and submit the patch. Alan Stern
Re: usb: gadget: f_mass_storage: detect eject
On Mon, Aug 20, 2018 at 4:23 PM Alan Stern wrote: > It is expected. No notifications for host-initiated ejects were ever > put into the f_mass_storage driver. > > I have never tried to use f_mass_storage under configfs. When you do, > does the driver create its normal sysfs files? Is there any other way to use it? Isn't config fs the way to instantiate GadgetFS/FunctionFS modules (f_mass_storage)? The only other files I find in sysfs are the ones referring to the module being loaded, but those do not change with eject. What should I look for? Thanks!
RE: [PATCH v2 2/7] doc: usb: ci-hdrc-usb2: Add pinctrl properties definition
> USB role (host/device), this can be an update of the pins routing or a simple > GPIO > value change. > > This patch introduces new optional "host" and "device" pinctrls. > If these pinctrls are defined by the device, they are respectively selected on > host/device role start. > > Signed-off-by: Loic Poulain > --- > v2: Add new pin modes documentation (host, device) > > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 0e03344..ea7033c 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -76,6 +76,8 @@ Optional properties: >needs to make sure it does not send more than 90% >maximum_periodic_data_per_frame. The use case is multiple transactions, but >less frame rate. > +- pinctrl-names: Names for optional pin modes in "default", "host", "device" > +- pinctrl-n: alternate pin modes > > i.mx specific properties > - fsl,usbmisc: phandler of non-core register device, with one > -- Please rebase to the latest usb-next, there is a conflict when apply this patch. Peter
RE: [PATCH v2 3/7] usb: chipidea: Support generic usb extcon
> Add compatibility for extcon-usb-gpio which can handle more than one cable per
> instance, allowing coherency of USB cable states (USB/USB-HOST). These states
> can be generated from ID or/and VBUS pins.
>
> In case only one extcon device is associated to the USB device, and this
> device
> supports USB and USB-HOST cable states, we now use it for both VBUS (USB) and
> ID (USB-HOST) notifier.
>
> Signed-off-by: Loic Poulain
> ---
> v2: no change
>
> drivers/usb/chipidea/core.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
> cdac778..afe85e2 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -702,6 +702,17 @@ static int ci_get_platdata(struct device *dev,
> ext_id = extcon_get_edev_by_phandle(dev, 1);
> if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> return PTR_ERR(ext_id);
> +
> + /*
> + * Some extcon devices like extcon-usb-gpio have only one
> + * instance for both USB and USB-HOST cable states.
> + */
> + if (!IS_ERR(ext_vbus) && IS_ERR(ext_id)) {
> + if (extcon_get_state(ext_vbus, EXTCON_USB) >= 0 &&
> + extcon_get_state(ext_vbus, EXTCON_USB_HOST) >= 0) {
> + ext_id = ext_vbus;
> + }
> + }
> }
>
Hi Loic,
For your case, I suggest changing dts instead of changing source code, you
would have both
vbus and id phandle at your controller dts, and both point to the same extcon
device.
Other patches are ok for me.
Peter
