Re: [PATCH] usb: dwc3: gadget: only unmap requests from DMA if mapped

[Thinh Nguyen]

Hi Felipe,

On 9/7/2017 12:16 AM, Felipe Balbi wrote:
drivers/usb/dwc3/gadget.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)

 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 9e41605a..6b299c7 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -191,14 +191,16 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, 
 struct dwc3_request *req,

req->started = false;
list_del(>list);
 -  req->trb = NULL;
req->remaining = 0;

if (req->request.status == -EINPROGRESS)
req->request.status = status;

 -  usb_gadget_unmap_request_by_dev(dwc->sysdev,
 -  >request, req->direction);
 +  if (req->trb)
>>> This check does not account for control data transfer. TRBs for ep0 are
>>> not set to its req->trb. ep0out request needs to be unmapped, otherwise
>>> device will receive bogus data.
>>>
>>> Our internal test showed that the device failed to interpret control
>>> data from host. I bisected to this patch.
> 
> what was the testing? How can I reproduce it?

This is a regression. The internal test found the issue when it does a 
3-stage Control Write Transfer. You can reproduce this issue with just 1 
out data packet of size > 0. Read and check the data on control request 
completion.

> 
>> Hi Thinh,
>>
>> Thanks for catching this. I can think of two ways to address this:
>>
>> 1. Make sure req->trb is populated for ep0/1 as well. This should be
>> easily done since the TRB corresponding to the mapped buffer is always
>> dwc->ep0_trb. We can assign the pointer after each of the map_request()
>> calls in __dwc3_ep0_do_control_data. And req->trb already gets zeroed
>> in dwc3_giveback() already. Hopefully this can be taken for 4.13.y
>> stable as well.
>>
>> 2. In 4.14-rc1 there is now commit 31fe084ffaaf ("usb: gadget: core:
>> unmap request from DMA only if previously mapped") which handles
>> $SUBJECT in a generic way so obviates the need for this patch, so
>> maybe this patch can simply be reverted. However this might not backport
>> so well for 4.13.y since reverting would bring us back to the behavior I
>> originally intended to fix.
>>
>> Felipe, what do you think?
> 
> I think we need to make sure req->trb is set in control transfers,
> too. But let's see, I want to be able to reproduce it first.
> 

Let me know if you need more info.

BR,
Thinh

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Bin Liu]

On Mon, Sep 04, 2017 at 06:32:11PM +0530, Sekhar Nori wrote:
> On Monday 14 August 2017 07:06 PM, Sekhar Nori wrote:
> > On Monday 14 August 2017 07:01 PM, Sekhar Nori wrote:
> >> Hi,
> >>
> >> On Sunday 13 August 2017 05:34 PM, Alexandre Bailon wrote:
> >>> The DA8xx and DSPS platforms don't use the same address for few registers.
> >>> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
> >>> Configure the address of the register during the init and use them instead
> >>> of constants.
> >>>
> >>> Reported-by: nsek...@ti.com
> > 
> > Reported-by: Sekhar Nori 
> 
> Tested-by: Sekhar Nori 
> 
> Hi Bin,
> 
> Do you have any additional comments on this series or are you waiting
> for v2 to be posted?

I don't have other comments, just am waiting for v2.

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Hans de Goede]

Hi,

On 07-09-17 15:14, Mathias Nyman wrote:

On 05.09.2017 19:42, Hans de Goede wrote:

The Intel cherrytrail xhci controller has an extended cap mmio-range
which contains registers to control the muxing to the xhci (host mode)
or the dwc3 (device mode) and vbus-detection for the otg usb-phy.

Having a mux driver included in the xhci code (or under drivers/usb/host)
is not desirable. So this commit adds a simple handler for this extended
capability, which creates a platform device with the caps mmio region as
resource, this allows us to write a separate platform mux driver for the
mux.

Signed-off-by: Hans de Goede 
---
Changes in v2:
-Check xHCI controller PCI device-id instead of only checking for the
  Intel Extended capability ID, as the Extended capability ID is used on
  other model Intel xHCI controllers too
---
  drivers/usb/host/Makefile|  2 +-
  drivers/usb/host/xhci-intel-quirks.c | 85 
  drivers/usb/host/xhci-pci.c  |  4 ++
  drivers/usb/host/xhci.h  |  2 +
  4 files changed, 92 insertions(+), 1 deletion(-)
  create mode 100644 drivers/usb/host/xhci-intel-quirks.c

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index cf2691fffcc0..441edf82eb1c 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)+= ohci-da8xx.o
  obj-$(CONFIG_USB_UHCI_HCD)+= uhci-hcd.o
  obj-$(CONFIG_USB_FHCI_HCD)+= fhci.o
  obj-$(CONFIG_USB_XHCI_HCD)+= xhci-hcd.o
-obj-$(CONFIG_USB_XHCI_PCI)+= xhci-pci.o
+obj-$(CONFIG_USB_XHCI_PCI)+= xhci-pci.o xhci-intel-quirks.o
  obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
  obj-$(CONFIG_USB_XHCI_MTK)+= xhci-mtk.o
  obj-$(CONFIG_USB_XHCI_TEGRA)+= xhci-tegra.o
diff --git a/drivers/usb/host/xhci-intel-quirks.c 
b/drivers/usb/host/xhci-intel-quirks.c
new file mode 100644


I think it would be better to have one place where we add handlers for
vendor specific extended capabilities.

Something like xhci-vendor-ext-caps.c, or just xhci-ext-caps.c as
there's a xhci-ext-caps.h header already

We could walk through the capability list once and add the needed handlers.
Something like:

+int xhci_ext_cap_init(void __iomem *base)


This will need to take a struct xhci_hcd *xhci param instead
as some of the ext_cap handling (including the cht mux code)
will need access to this.


+{
+u32 val;
+   u32 cap_offset;
+
+cap_offset = xhci_next_ext_cap(base, 0);
+
+while (cap_offset) {
+val = readl(base + cap_offset);
+
+switch (XHCI_EXT_CAPS_ID(val)) {
+case XHCI_EXT_CAPS_VENDOR_INTEL:
+/* check hw/id/something, and call what's needed */


So I see 2 options here (without making this function PCI specific)
1) Add an u32 product_id field to struct xhci_hcd; or
2) Use a quirk flag as my current code is doing.

I'm fine with doing this either way, please let me know your preference.


+break;
+case XHCI_EXT_CAPS_VENDOR_XYZ:
+/* do something */
+break;
+default:
+break;
+}
+
+   printk(KERN_ERR "MATTU EXT_CAP id %d\n", XHCI_EXT_CAPS_ID(val));
+
+   cap_offset = xhci_next_ext_cap(base, cap_offset);
+}
+
+return 0;
+}

xhci_next_ext_cap() doesn't exist anywhere else than my local sandbox branch 
yet.


Can you do a "git format-patch" of that and send it to me? If you
can give me that + your preference for how to check if we're
dealing with a cht xhci hcd in xhci_ext_cap_init I can do a v3
with your suggestions applied.




+
+/* Extended capability IDs for Intel Vendor Defined */
+#define XHCI_EXT_CAPS_INTEL_HOST_CAP192


XHCI_EXT_CAPS_VENDOR_INTEL
and should be in xhci-ext-caps.h


Ok.

Regards,

Hans





+
+static void xhci_intel_unregister_pdev(void *arg)
+{
+platform_device_unregister(arg);
+}
+
+int xhci_create_intel_cht_mux_pdev(struct xhci_hcd *xhci)
+{
+struct usb_hcd *hcd = xhci_to_hcd(xhci);
+struct device *dev = hcd->self.controller;
+struct platform_device *pdev;
+struct resourceres = { 0, };
+int ret, ext_offset;
+
+ext_offset = xhci_find_next_ext_cap(>cap_regs->hc_capbase, 0,
+XHCI_EXT_CAPS_INTEL_HOST_CAP);
+if (!ext_offset) {
+xhci_err(xhci, "couldn't find Intel ext caps\n");
+return -ENODEV;
+}
+
+pdev = platform_device_alloc("intel_cht_usb_mux", PLATFORM_DEVID_NONE);
+if (!pdev) {
+xhci_err(xhci, "couldn't allocate intel_cht_usb_mux pdev\n");
+return -ENOMEM;
+}
+
+res.start = hcd->rsrc_start + ext_offset;
+res.end  = res.start + 0x3ff;
+res.name  = "intel_cht_usb_mux";
+res.flags = IORESOURCE_MEM;
+
+ret = platform_device_add_resources(pdev, , 1);
+if (ret) {
+ 

Re: Bug 196559: xhci_hcd: kernel panic using thunderbolt dell dock TB16 on 4.12.4

[Greg KH]

On Thu, Sep 07, 2017 at 09:12:53AM -0600, Jose Marino wrote:
> I have tested Mathias' patch on top of v4.13 and it fixes the problem. I was
> able to suspend/resume a few times with no kernel panics.

Yeah!  Thanks for testing.  Mathias, care to send me a "real" patch for
this so I can get it to Linus and then the stable releases?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug 196559: xhci_hcd: kernel panic using thunderbolt dell dock TB16 on 4.12.4

[Jose Marino]

I have tested Mathias' patch on top of v4.13 and it fixes the problem. I 
was able to suspend/resume a few times with no kernel panics.


Jose

On 09/07/2017 08:09 AM, Mathias Nyman wrote:

On 07.09.2017 14:59, Mathias Nyman wrote:

On 07.09.2017 12:23, Greg KH wrote:

On Wed, Sep 06, 2017 at 10:04:08AM -0600, Jose Marino wrote:
The bug is still present in kernel 4.13. The panic logs look pretty 
much the
same as with 4.12.4. I have attached the pstore and journald 
messages to the

bugzilla bug report just in case.

I reverted commit 5cc9b698a494827 on top of v4.13 and I can confirm 
that it

fixes the problem.


Mathias, any thoughts here?  Should we just revert this patch for now to
resolve the issue, or do you know of a fix for it?

thanks,

greg k-h



Adding patch authors to CC

5cc9b698a494827 in stable,  (9da5a1092b13 upstream) has some magic 
workaround for

ASMEDIA ASM1042A xHCI host.
It does some pci config space reading, polling (sleeping) and writing.

My first guess is that this ASmedia workaround somehow gets called in 
interrupt context

when host is pci hotplug removed.


Turns out we xhci_stop() will call that sleeping workaround with 
spin_lock_irq() held


Same usleep_range() -> udelay() fix below should work



Maybe worth trying to use udelay() instead of usleep_range(), or
make sure workaround is not called in interrupt context before 
reverting the patch


Does this help:

index 658d9d1..98a866f 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -447,7 +447,7 @@ static int usb_asmedia_wait_write(struct pci_dev 
*pdev)

 if ((value & ASMT_CONTROL_WRITE_BIT) == 0)
 return 0;

-   usleep_range(40, 60);
+   udelay(50);
 }

 dev_warn(>dev, "%s: check_write_ready timeout", __func__);

-Mathias



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [v2] usb: gadget: dummy: fix nonsensical comparisons

[Alan Stern]

On Thu, 7 Sep 2017, Arnd Bergmann wrote:

> gcc-8 points out two comparisons that are clearly bogus
> and almost certainly not what the author intended to write:
> 
> drivers/usb/gadget/udc/dummy_hcd.c: In function 'set_link_state_by_speed':
> drivers/usb/gadget/udc/dummy_hcd.c:379:31: error: bitwise comparison always 
> evaluates to false [-Werror=tautological-compare]
>  USB_PORT_STAT_ENABLE) == 1 &&
>^~
> drivers/usb/gadget/udc/dummy_hcd.c:381:25: error: bitwise comparison always 
> evaluates to false [-Werror=tautological-compare]
>   USB_SS_PORT_LS_U0) == 1 &&
>  ^~
> 
> I looked at the code for a bit and came up with a change that makes
> it look like what the author probably meant here. This makes it
> look reasonable to me and to gcc, shutting up the warning.
> 
> It does of course change behavior as the two conditions are actually
> evaluated rather than being hardcoded to false, and I have made no
> attempt at verifying that the changed logic makes sense in the context
> of a USB HCD, so that part needs to be reviewed carefully.
> 
> Fixes: 1cd8fd2887e1 ("usb: gadget: dummy_hcd: add SuperSpeed support")
> Cc: Tatyana Brokhman 
> Cc: Felipe Balbi 
> Signed-off-by: Arnd Bergmann 
> ---
> v2: simplify the expression as suggested by Alan Stern
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
> b/drivers/usb/gadget/udc/dummy_hcd.c
> index a030d7923d7d..b1e21b3be6e1 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -375,11 +375,10 @@ static void set_link_state_by_speed(struct dummy_hcd 
> *dum_hcd)
>USB_PORT_STAT_CONNECTION) == 0)
>   dum_hcd->port_status |=
>   (USB_PORT_STAT_C_CONNECTION << 16);
> - if ((dum_hcd->port_status &
> -  USB_PORT_STAT_ENABLE) == 1 &&
> - (dum_hcd->port_status &
> -  USB_SS_PORT_LS_U0) == 1 &&
> - dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
> + if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) &&
> + (dum_hcd->port_status &
> +  USB_PORT_STAT_LINK_STATE) == USB_SS_PORT_LS_U0 &&
> + dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
>   dum_hcd->active = 1;
>   }
>   } else {

Acked-by: Alan Stern 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [v2] usb: gadget: dummy: fix nonsensical comparisons

[Arnd Bergmann]

gcc-8 points out two comparisons that are clearly bogus
and almost certainly not what the author intended to write:

drivers/usb/gadget/udc/dummy_hcd.c: In function 'set_link_state_by_speed':
drivers/usb/gadget/udc/dummy_hcd.c:379:31: error: bitwise comparison always 
evaluates to false [-Werror=tautological-compare]
 USB_PORT_STAT_ENABLE) == 1 &&
   ^~
drivers/usb/gadget/udc/dummy_hcd.c:381:25: error: bitwise comparison always 
evaluates to false [-Werror=tautological-compare]
  USB_SS_PORT_LS_U0) == 1 &&
 ^~

I looked at the code for a bit and came up with a change that makes
it look like what the author probably meant here. This makes it
look reasonable to me and to gcc, shutting up the warning.

It does of course change behavior as the two conditions are actually
evaluated rather than being hardcoded to false, and I have made no
attempt at verifying that the changed logic makes sense in the context
of a USB HCD, so that part needs to be reviewed carefully.

Fixes: 1cd8fd2887e1 ("usb: gadget: dummy_hcd: add SuperSpeed support")
Cc: Tatyana Brokhman 
Cc: Felipe Balbi 
Signed-off-by: Arnd Bergmann 
---
v2: simplify the expression as suggested by Alan Stern
---
 drivers/usb/gadget/udc/dummy_hcd.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c 
b/drivers/usb/gadget/udc/dummy_hcd.c
index a030d7923d7d..b1e21b3be6e1 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -375,11 +375,10 @@ static void set_link_state_by_speed(struct dummy_hcd 
*dum_hcd)
 USB_PORT_STAT_CONNECTION) == 0)
dum_hcd->port_status |=
(USB_PORT_STAT_C_CONNECTION << 16);
-   if ((dum_hcd->port_status &
-USB_PORT_STAT_ENABLE) == 1 &&
-   (dum_hcd->port_status &
-USB_SS_PORT_LS_U0) == 1 &&
-   dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
+   if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) &&
+   (dum_hcd->port_status &
+USB_PORT_STAT_LINK_STATE) == USB_SS_PORT_LS_U0 &&
+   dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
dum_hcd->active = 1;
}
} else {
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug 196559: xhci_hcd: kernel panic using thunderbolt dell dock TB16 on 4.12.4

[Mathias Nyman]

On 07.09.2017 14:59, Mathias Nyman wrote:

On 07.09.2017 12:23, Greg KH wrote:

On Wed, Sep 06, 2017 at 10:04:08AM -0600, Jose Marino wrote:

The bug is still present in kernel 4.13. The panic logs look pretty much the
same as with 4.12.4. I have attached the pstore and journald messages to the
bugzilla bug report just in case.

I reverted commit 5cc9b698a494827 on top of v4.13 and I can confirm that it
fixes the problem.


Mathias, any thoughts here?  Should we just revert this patch for now to
resolve the issue, or do you know of a fix for it?

thanks,

greg k-h



Adding patch authors to CC

5cc9b698a494827 in stable,  (9da5a1092b13 upstream) has some magic workaround 
for
ASMEDIA ASM1042A xHCI host.
It does some pci config space reading, polling (sleeping) and writing.

My first guess is that this ASmedia workaround somehow gets called in interrupt 
context
when host is pci hotplug removed.


Turns out we xhci_stop() will call that sleeping workaround with 
spin_lock_irq() held

Same usleep_range() -> udelay() fix below should work



Maybe worth trying to use udelay() instead of usleep_range(), or
make sure workaround is not called in interrupt context before reverting the 
patch

Does this help:

index 658d9d1..98a866f 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -447,7 +447,7 @@ static int usb_asmedia_wait_write(struct pci_dev *pdev)
 if ((value & ASMT_CONTROL_WRITE_BIT) == 0)
 return 0;

-   usleep_range(40, 60);
+   udelay(50);
 }

 dev_warn(>dev, "%s: check_write_ready timeout", __func__);

-Mathias



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/2] USB: musb: PM fixes

[Johan Hovold]

These patches fix a couple of bugs introduced by the recent runtime-PM
work (details in the individual commit messages).

Note that the external abort was due to the irq work never being flushed
on suspend, and that we may need similar fixes for the delayed reset and
resume work which are likewise never cancelled on suspend.

As I just mentioned in the v1 thread, there are a number of further issues with
musb suspend (e.g. on BBB):

 1. System suspend appears to break any active gadget (which then can be
restarted manually).

 2. The early_tx polling in musb_cppi41 lacks a timeout, something which can
lead to the hrtimer rescheduling itself indefinitely if the fifo never
empties (e.g. if a transfer is is initiated post suspend due to issue 1).

See commit a655f481d83d ("usb: musb: musb_cppi41: handle pre-mature TX
complete interrupt").

 3. In host mode, if a device is disconnected while the system is suspended,
this will keep the controller runtime active after resume as the session
bit is always set.

Johan


Changes in v2
 - reorder the two patches
 - flush rather than cancel the irq work, and set a flag before flushing so that
   a detected disconnect is processed immeditaly (instead of unconditionally
   clearing the session flag on suspend)


Johan Hovold (2):
  USB: musb: fix session-bit runtime-PM quirk
  USB: musb: fix late external abort on suspend

 drivers/usb/musb/musb_core.c | 15 +++
 drivers/usb/musb/musb_core.h |  2 ++
 2 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: musb: fix late external abort on suspend

[Johan Hovold]

The musb delayed irq work was never flushed on suspend, something which
since 4.9 can lead to an external abort if the work is scheduled after
the grandparent's clock has been disabled:

PM: Suspending system (mem)
PM: suspend of devices complete after 125.224 msecs
PM: suspend devices took 0.132 seconds
PM: late suspend of devices complete after 7.423 msecs
PM: noirq suspend of devices complete after 7.083 msecs
suspend debug: Waiting for 5 second(s).
Unhandled fault: external abort on non-linefetch (0x1008) at 0xd0262c60
...
[] (musb_default_readb) from [] (musb_irq_work+0x48/0x220)
[] (musb_irq_work) from [] (process_one_work+0x1f4/0x758)
[] (process_one_work) from [] (worker_thread+0x54/0x514)
[] (worker_thread) from [] (kthread+0x128/0x158)
[] (kthread) from [] (ret_from_fork+0x14/0x24)

Commit 2bff3916fda9 ("usb: musb: Fix PM for hub disconnect") started
scheduling musb_irq_work with a delay of up to a second and with
retries thereby making this easy to trigger, for example, by suspending
shortly after a disconnect.

Note that we set a flag to prevent the irq work from rescheduling itself
during suspend and instead process a disconnect immediately. This takes
care of the case where we are disconnected shortly before suspending.

However, when in host mode, a disconnect while suspended will still
go unnoticed and thus prevent the controller from runtime suspending
upon resume as the session bit is always set. This will need to be
addressed separately.

Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
Fixes: 467d5c980709 ("usb: musb: Implement session bit based runtime PM for 
musb-core")
Fixes: 2bff3916fda9 ("usb: musb: Fix PM for hub disconnect")
Cc: stable  # 4.9
Cc: Felipe Balbi 
Cc: Tony Lindgren 
Signed-off-by: Johan Hovold 
---
 drivers/usb/musb/musb_core.c | 11 +--
 drivers/usb/musb/musb_core.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 38fa3603554f..e083d0cce670 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1861,7 +1861,7 @@ static void musb_pm_runtime_check_session(struct musb 
*musb)
MUSB_DEVCTL_HR;
switch (devctl & ~s) {
case MUSB_QUIRK_B_INVALID_VBUS_91:
-   if (musb->quirk_retries) {
+   if (musb->quirk_retries && !musb->flush_irq_work) {
musb_dbg(musb,
 "Poll devctl on invalid vbus, assume no 
session");
schedule_delayed_work(>irq_work,
@@ -1871,7 +1871,7 @@ static void musb_pm_runtime_check_session(struct musb 
*musb)
}
/* fall through */
case MUSB_QUIRK_A_DISCONNECT_19:
-   if (musb->quirk_retries) {
+   if (musb->quirk_retries && !musb->flush_irq_work) {
musb_dbg(musb,
 "Poll devctl on possible host mode 
disconnect");
schedule_delayed_work(>irq_work,
@@ -2681,8 +2681,15 @@ static int musb_suspend(struct device *dev)
 
musb_platform_disable(musb);
musb_disable_interrupts(musb);
+
+   musb->flush_irq_work = true;
+   while (flush_delayed_work(>irq_work))
+   ;
+   musb->flush_irq_work = false;
+
if (!(musb->io.quirks & MUSB_PRESERVE_SESSION))
musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
+
WARN_ON(!list_empty(>pending_list));
 
spin_lock_irqsave(>lock, flags);
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 9f22c5b8ce37..1830a571d025 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -428,6 +428,8 @@ struct musb {
unsignedtest_mode:1;
unsignedsoftconnect:1;
 
+   unsignedflush_irq_work:1;
+
u8  address;
u8  test_mode_nr;
u16 ackpend;/* ep0 */
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] USB: musb: fix session-bit runtime-PM quirk

[Johan Hovold]

The current session-bit quirk implementation does not prevent the retry
counter from underflowing, something which could break runtime PM and
keep the device active for a very long time (about 2^32 seconds) after a
disconnect.

This notably breaks the B-device timeout case, but could potentially
cause problems also when the controller is operating as an A-device.

Fixes: 2bff3916fda9 ("usb: musb: Fix PM for hub disconnect")
Cc: stable  # 4.9
Cc: Tony Lindgren 
Signed-off-by: Johan Hovold 
---
 drivers/usb/musb/musb_core.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index b67692857daf..38fa3603554f 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1861,22 +1861,22 @@ static void musb_pm_runtime_check_session(struct musb 
*musb)
MUSB_DEVCTL_HR;
switch (devctl & ~s) {
case MUSB_QUIRK_B_INVALID_VBUS_91:
-   if (musb->quirk_retries--) {
+   if (musb->quirk_retries) {
musb_dbg(musb,
 "Poll devctl on invalid vbus, assume no 
session");
schedule_delayed_work(>irq_work,
  msecs_to_jiffies(1000));
-
+   musb->quirk_retries--;
return;
}
/* fall through */
case MUSB_QUIRK_A_DISCONNECT_19:
-   if (musb->quirk_retries--) {
+   if (musb->quirk_retries) {
musb_dbg(musb,
 "Poll devctl on possible host mode 
disconnect");
schedule_delayed_work(>irq_work,
  msecs_to_jiffies(1000));
-
+   musb->quirk_retries--;
return;
}
if (!musb->session)
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Mathias Nyman]

On 05.09.2017 19:42, Hans de Goede wrote:

The Intel cherrytrail xhci controller has an extended cap mmio-range
which contains registers to control the muxing to the xhci (host mode)
or the dwc3 (device mode) and vbus-detection for the otg usb-phy.

Having a mux driver included in the xhci code (or under drivers/usb/host)
is not desirable. So this commit adds a simple handler for this extended
capability, which creates a platform device with the caps mmio region as
resource, this allows us to write a separate platform mux driver for the
mux.

Signed-off-by: Hans de Goede 
---
Changes in v2:
-Check xHCI controller PCI device-id instead of only checking for the
  Intel Extended capability ID, as the Extended capability ID is used on
  other model Intel xHCI controllers too
---
  drivers/usb/host/Makefile|  2 +-
  drivers/usb/host/xhci-intel-quirks.c | 85 
  drivers/usb/host/xhci-pci.c  |  4 ++
  drivers/usb/host/xhci.h  |  2 +
  4 files changed, 92 insertions(+), 1 deletion(-)
  create mode 100644 drivers/usb/host/xhci-intel-quirks.c

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index cf2691fffcc0..441edf82eb1c 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)+= ohci-da8xx.o
  obj-$(CONFIG_USB_UHCI_HCD)+= uhci-hcd.o
  obj-$(CONFIG_USB_FHCI_HCD)+= fhci.o
  obj-$(CONFIG_USB_XHCI_HCD)+= xhci-hcd.o
-obj-$(CONFIG_USB_XHCI_PCI) += xhci-pci.o
+obj-$(CONFIG_USB_XHCI_PCI) += xhci-pci.o xhci-intel-quirks.o
  obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
  obj-$(CONFIG_USB_XHCI_MTK)+= xhci-mtk.o
  obj-$(CONFIG_USB_XHCI_TEGRA)  += xhci-tegra.o
diff --git a/drivers/usb/host/xhci-intel-quirks.c 
b/drivers/usb/host/xhci-intel-quirks.c
new file mode 100644


I think it would be better to have one place where we add handlers for
vendor specific extended capabilities.

Something like xhci-vendor-ext-caps.c, or just xhci-ext-caps.c as
there's a xhci-ext-caps.h header already

We could walk through the capability list once and add the needed handlers.
Something like:

+int xhci_ext_cap_init(void __iomem *base)
+{
+u32 val;
+   u32 cap_offset;
+
+cap_offset = xhci_next_ext_cap(base, 0);
+
+while (cap_offset) {
+val = readl(base + cap_offset);
+
+switch (XHCI_EXT_CAPS_ID(val)) {
+case XHCI_EXT_CAPS_VENDOR_INTEL:
+/* check hw/id/something, and call what's needed */
+break;
+case XHCI_EXT_CAPS_VENDOR_XYZ:
+/* do something */
+break;
+default:
+break;
+}
+
+   printk(KERN_ERR "MATTU EXT_CAP id %d\n", XHCI_EXT_CAPS_ID(val));
+
+   cap_offset = xhci_next_ext_cap(base, cap_offset);
+}
+
+return 0;
+}

xhci_next_ext_cap() doesn't exist anywhere else than my local sandbox branch 
yet.


+
+/* Extended capability IDs for Intel Vendor Defined */
+#define XHCI_EXT_CAPS_INTEL_HOST_CAP   192


XHCI_EXT_CAPS_VENDOR_INTEL
and should be in xhci-ext-caps.h


+
+static void xhci_intel_unregister_pdev(void *arg)
+{
+   platform_device_unregister(arg);
+}
+
+int xhci_create_intel_cht_mux_pdev(struct xhci_hcd *xhci)
+{
+   struct usb_hcd *hcd = xhci_to_hcd(xhci);
+   struct device *dev = hcd->self.controller;
+   struct platform_device *pdev;
+   struct resource res = { 0, };
+   int ret, ext_offset;
+
+   ext_offset = xhci_find_next_ext_cap(>cap_regs->hc_capbase, 0,
+   XHCI_EXT_CAPS_INTEL_HOST_CAP);
+   if (!ext_offset) {
+   xhci_err(xhci, "couldn't find Intel ext caps\n");
+   return -ENODEV;
+   }
+
+   pdev = platform_device_alloc("intel_cht_usb_mux", PLATFORM_DEVID_NONE);
+   if (!pdev) {
+   xhci_err(xhci, "couldn't allocate intel_cht_usb_mux pdev\n");
+   return -ENOMEM;
+   }
+
+   res.start = hcd->rsrc_start + ext_offset;
+   res.end   = res.start + 0x3ff;
+   res.name  = "intel_cht_usb_mux";
+   res.flags = IORESOURCE_MEM;
+
+   ret = platform_device_add_resources(pdev, , 1);
+   if (ret) {
+   dev_err(dev, "couldn't add resources to intel_cht_usb_mux 
pdev\n");
+   platform_device_put(pdev);
+   return ret;
+   }
+
+   pdev->dev.parent = dev;
+
+   ret = platform_device_add(pdev);
+   if (ret) {
+   dev_err(dev, "couldn't register intel_cht_usb_mux pdev\n");
+   platform_device_put(pdev);
+   return ret;
+   }
+
+   ret = devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev);
+   if (ret) {
+   dev_err(dev, "couldn't add unregister action for intel_cht_usb_mux 

Re: [PATCH 0/2] USB: musb: PM fixes

[Johan Hovold]

On Tue, Sep 05, 2017 at 04:21:10PM +0200, Johan Hovold wrote:
> These patches fix a couple of bugs introduced by the recent runtime-PM
> work. 
> 
> Note that the external abort was due to the irq work never being flushed
> on suspend, and that we may need similar fixes for the delayed reset and
> resume work which are likewise never cancelled on suspend.

Looks like there are even more issues with musb suspend.

With this series, which allows the controller to runtime suspend upon
system resume, I can now trigger the following external abort at resume:

PM: Finishing wakeup.
OOM killer enabled.
Restarting tasks ... done.
hrtimer: interrupt took 191917 ns
Unhandled fault: external abort on non-linefetch (0x1008) at 0xc8249412
pgd = c0004000
[c8249412] *pgd=87350811, *pte=47401653, *ppte=47401453
Internal error: : 1008 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 572 Comm: kworker/0:2 Not tainted 4.12.0 #34
Hardware name: Generic AM33XX (Flattened Device Tree)
Workqueue: pm pm_runtime_work
task: c72057c0 task.stack: c722a000
PC is at musb_default_readw+0x4/0x10
LR is at musb_is_tx_fifo_empty+0x3c/0x48

[] (musb_default_readw) from [] 
(musb_is_tx_fifo_empty+0x3c/0x48)
[] (musb_is_tx_fifo_empty) from [] 
(cppi41_recheck_tx_req+0x5c/0x118)
[] (cppi41_recheck_tx_req) from [] 
(__hrtimer_run_queues.constprop.4+0x110/0x1bc)
[] (__hrtimer_run_queues.constprop.4) from [] 
(hrtimer_interrupt+0x98/0x230)
[] (hrtimer_interrupt) from [] 
(omap2_gp_timer_interrupt+0x28/0x30)
[] (omap2_gp_timer_interrupt) from [] 
(__handle_irq_event_percpu+0x88/0x124)
[] (__handle_irq_event_percpu) from [] 
(handle_irq_event_percpu+0x1c/0x58)
[] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x4c/0x84)
[] (handle_irq_event) from [] (handle_level_irq+0xb0/0x15c)
[] (handle_level_irq) from [] (generic_handle_irq+0x24/0x34)
[] (generic_handle_irq) from [] 
(__handle_domain_irq+0x70/0xdc)
[] (__handle_domain_irq) from [] (__irq_svc+0x6c/0xa8)
[] (__irq_svc) from [] (omap_hwmod_idle+0x30/0x74)
[] (omap_hwmod_idle) from [] (omap_device_idle+0x40/0x90)
[] (omap_device_idle) from [] (__rpm_callback+0x15c/0x258)
[] (__rpm_callback) from [] (rpm_callback+0x50/0x80)
[] (rpm_callback) from [] (rpm_suspend+0xe0/0x548)
[] (rpm_suspend) from [] (pm_runtime_work+0xac/0xbc)
[] (pm_runtime_work) from [] (process_one_work+0x11c/0x350)
[] (process_one_work) from [] (worker_thread+0x38/0x55c)
[] (worker_thread) from [] (kthread+0x100/0x130)
[] (kthread) from [] (ret_from_fork+0x14/0x3c)

after having suspended with an active ECM gadget.

Turns out system suspend breaks musb in gadget mode. It seems I need to
manually restart the gadget to get it to work again even it had just
been enumerated (and which does not trigger the above crash). (Bug 1)

But if an ECM gadget is also active (e.g. open SSH session) when
suspending, this in turn can trigger yet another bug in that the
early_tx dma-irq hrtimer is never cancelled when the tx-fifo does not
empty when the gadget driver initiates a transfer after resume. The
early_tx timer keeps rescheduling itself until the gadget it stopped
manually (keeping the BBB CPU busy at about 20-30%). (Bug 2)

If the controller is allowed to runtime suspend after system resume, as
with this series, this repeated scheduling triggers the above external
abort.

I've respun the series so that the session flag and runtime pm count is
left untouched unless we've already started the session-quirk timeout
handling.

This avoids the above crash, but does not address another problem with
the current code, namely that the controller is left active in case a
device is disconnected while suspended in host mode. (Bug 3)

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug 196559: xhci_hcd: kernel panic using thunderbolt dell dock TB16 on 4.12.4

[Mathias Nyman]

On 07.09.2017 12:23, Greg KH wrote:

On Wed, Sep 06, 2017 at 10:04:08AM -0600, Jose Marino wrote:

The bug is still present in kernel 4.13. The panic logs look pretty much the
same as with 4.12.4. I have attached the pstore and journald messages to the
bugzilla bug report just in case.

I reverted commit 5cc9b698a494827 on top of v4.13 and I can confirm that it
fixes the problem.


Mathias, any thoughts here?  Should we just revert this patch for now to
resolve the issue, or do you know of a fix for it?

thanks,

greg k-h



Adding patch authors to CC

5cc9b698a494827 in stable,  (9da5a1092b13 upstream) has some magic workaround 
for
ASMEDIA ASM1042A xHCI host.
It does some pci config space reading, polling (sleeping) and writing.

My first guess is that this ASmedia workaround somehow gets called in interrupt 
context
when host is pci hotplug removed.

Maybe worth trying to use udelay() instead of usleep_range(), or
make sure workaround is not called in interrupt context before reverting the 
patch

Does this help:

index 658d9d1..98a866f 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -447,7 +447,7 @@ static int usb_asmedia_wait_write(struct pci_dev *pdev)
if ((value & ASMT_CONTROL_WRITE_BIT) == 0)
return 0;
 
-   usleep_range(40, 60);

+   udelay(50);
}
 
dev_warn(>dev, "%s: check_write_ready timeout", __func__);


-Mathias



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot

[Felipe Balbi]

Hi,

gustavo panizzo  writes:
---
 drivers/usb/dwc3/core.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 326b302fc440..f92dfe213d89 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1259,6 +1259,38 @@ static int dwc3_probe(struct platform_device
*pdev)
return ret;
 }

+static void dwc3_shutdown(struct platform_device *pdev)
+{
+   struct dwc3 *dwc = platform_get_drvdata(pdev);
+   struct resource *res = platform_get_resource(pdev,
IORESOURCE_MEM, 0);
+
+   pm_runtime_get_sync(>dev);
+   /*
+* restore res->start back to its original value so that, in 
case
the
+* probe is deferred, we don't end up getting error in request
the
+* memory region the next time probe is called.
+*/
+   res->start -= DWC3_GLOBALS_REGS_START;
+
+   dwc3_debugfs_exit(dwc);
+   dwc3_core_exit_mode(dwc);
+   dwc3_event_buffers_cleanup(dwc);
>>
>>
>>What about dwc3_event_buffers_cleanup? should I remove it from
>>dwc3_shutdown()?
>>It is already in dwc3_core_exit()
>
>I think so. We should avoid duplicate code.
>
+   dwc3_free_event_buffers(dwc);
+
+   usb_phy_set_suspend(dwc->usb2_phy, 1);
+   usb_phy_set_suspend(dwc->usb3_phy, 1);
+
+   phy_power_off(dwc->usb2_generic_phy);
+   phy_power_off(dwc->usb3_generic_phy);
>>>
>>>
>>>We've done these in dwc3_core_exit().

This is the patch after testing on a Odroid XU4, on top of linux-next
964bcc1b4f57028d56dace7d9bc5924f2eb43f36 which translates to 
4.13.0-rc1-next-20170717+
I tested this patch for a week without problems with heavy USB and NIC 
usage.
Please consider merging it
>
> Author: Brian Kim 
> Date:   Wed Jul 12 11:26:55 2017 +0800
>
>usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot
>The dwc3 could not release resources when the module is built-in
>because this module does not have shutdown method. This causes the USB
>3.0 hub is not able to detect after warm boot.
>Original patch by Brian Kim, updated and submitted upstream by gustavo
>panizzo.
>Also see https://bugs.debian.org/843448
>Signed-off-by: Brian Kim 
>Signed-off-by: gustavo panizzo 
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 326b302fc440..09de37d47ee7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1259,6 +1259,32 @@ static int dwc3_probe(struct platform_device *pdev)
>   return ret;
> }
>
> +static void dwc3_shutdown(struct platform_device *pdev)
> +{
> +   struct dwc3 *dwc = platform_get_drvdata(pdev);
> +   struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +   pm_runtime_get_sync(>dev);
> +   /*
> +* restore res->start back to its original value so that, in case the
> +* probe is deferred, we don't end up getting error in request the
> +* memory region the next time probe is called.
> +*/
> +   res->start -= DWC3_GLOBALS_REGS_START;
> +
> +   dwc3_debugfs_exit(dwc);
> +   dwc3_core_exit_mode(dwc);
> +   dwc3_event_buffers_cleanup(dwc);
> +   dwc3_free_event_buffers(dwc);
> +
> +   dwc3_core_exit(dwc);
> +   dwc3_ulpi_exit(dwc);
> +
> +   pm_runtime_put_sync(>dev);
> +   pm_runtime_allow(>dev);
> +   pm_runtime_disable(>dev);
> +}
> +
> static int dwc3_remove(struct platform_device *pdev)
> {
>   struct dwc3 *dwc = platform_get_drvdata(pdev);
> @@ -1488,6 +1514,7 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
> static struct platform_driver dwc3_driver = {
> .probe  = dwc3_probe,
> .remove = dwc3_remove,
> + .shutdown   = dwc3_shutdown,
> .driver = {
> .name   = "dwc3",
> .of_match_table = of_match_ptr(of_dwc3_match),
>
> Patch applies cleanly on top of c6be5a0e3cebc145127d46a58350e05d2bcf6323 from 
> linux-next
> Can you _please_ merge it?

why are you upset? You didn't do the changes I requested until now. It's
too late for v4.14 merge window and you didn't even send this as a
proper patch. I also have no evidence that you've been testing mainline
kernel, the commits you pointed me to are against a v4.9 vendor kernel.

Test this against a vanilla tree (v4.13 was tagged days ago) and give me
logs showing the problem without your commit.

Also, the commit you pointed me to couldn't be the 

Re: Bug 196559: xhci_hcd: kernel panic using thunderbolt dell dock TB16 on 4.12.4

[Greg KH]

On Wed, Sep 06, 2017 at 10:04:08AM -0600, Jose Marino wrote:
> The bug is still present in kernel 4.13. The panic logs look pretty much the
> same as with 4.12.4. I have attached the pstore and journald messages to the
> bugzilla bug report just in case.
> 
> I reverted commit 5cc9b698a494827 on top of v4.13 and I can confirm that it
> fixes the problem.

Mathias, any thoughts here?  Should we just revert this patch for now to
resolve the issue, or do you know of a fix for it?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: gadget: only unmap requests from DMA if mapped

[Felipe Balbi]

Hi,

Jack Pham  writes:
>> On 6/29/2017 12:54 AM, Jack Pham wrote:
>> > A recent optimization was made so that a request put on the
>> > pending_list wouldn't get mapped for DMA until just before
>> > preparing a TRB for it. However, this poses a problem in case
>> > the request is dequeued or the endpoint is disabled before the
>> > mapping is done as that would lead to dwc3_gadget_giveback()
>> > unconditionally calling usb_gadget_unmap_request_for_dev() with
>> > an invalid request->dma handle. Depending on the platform's DMA
>> > implementation the unmap operation could result in a panic.
>> > 
>> > Since we know a successful mapping is a prerequisite for getting
>> > a TRB, the unmap can be conditionally called only when req->trb
>> > is non-NULL.
>> > 
>> > Fixes: cdb55b39fab8 ("usb: dwc3: gadget: lazily map requests for DMA")
>> > Signed-off-by: Jack Pham 
>> > ---
>> >   drivers/usb/dwc3/gadget.c | 8 +---
>> >   1 file changed, 5 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 9e41605a..6b299c7 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -191,14 +191,16 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, 
>> > struct dwc3_request *req,
>> >   
>> >req->started = false;
>> >list_del(>list);
>> > -  req->trb = NULL;
>> >req->remaining = 0;
>> >   
>> >if (req->request.status == -EINPROGRESS)
>> >req->request.status = status;
>> >   
>> > -  usb_gadget_unmap_request_by_dev(dwc->sysdev,
>> > -  >request, req->direction);
>> > +  if (req->trb)
>> This check does not account for control data transfer. TRBs for ep0 are 
>> not set to its req->trb. ep0out request needs to be unmapped, otherwise 
>> device will receive bogus data.
>> 
>> Our internal test showed that the device failed to interpret control 
>> data from host. I bisected to this patch.

what was the testing? How can I reproduce it?

> Hi Thinh,
>
> Thanks for catching this. I can think of two ways to address this:
>
> 1. Make sure req->trb is populated for ep0/1 as well. This should be
> easily done since the TRB corresponding to the mapped buffer is always
> dwc->ep0_trb. We can assign the pointer after each of the map_request()
> calls in __dwc3_ep0_do_control_data. And req->trb already gets zeroed
> in dwc3_giveback() already. Hopefully this can be taken for 4.13.y
> stable as well.
>
> 2. In 4.14-rc1 there is now commit 31fe084ffaaf ("usb: gadget: core:
> unmap request from DMA only if previously mapped") which handles
> $SUBJECT in a generic way so obviates the need for this patch, so
> maybe this patch can simply be reverted. However this might not backport
> so well for 4.13.y since reverting would bring us back to the behavior I
> originally intended to fix.
>
> Felipe, what do you think?

I think we need to make sure req->trb is set in control transfers,
too. But let's see, I want to be able to reproduce it first.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot

[gustavo panizzo]

Hello

On Sun, Jul 30, 2017 at 07:53:03AM +0800, gustavo panizzo wrote:

Hi

On Mon, Jul 24, 2017 at 12:53:27PM +0300, Felipe Balbi wrote:


Hi,

gustavo panizzo  writes:

On Wed, Jul 12, 2017 at 02:08:04PM +0800, Baolin Wang wrote:


Hi,

On 12 July 2017 at 11:52, gustavo panizzo  wrote:


The dwc3 could not release resources when the module is built-in
because this module does not have shutdown method. This causes the USB
3.0 hub is not able to detect after warm boot.

Original patch by Brian Kim, updated and submitted upstream by gustavo
panizzo.

Also see https://bugs.debian.org/843448

Signed-off-by: Brian Kim 
Signed-off-by: gustavo panizzo 
---
drivers/usb/dwc3/core.c | 33 +
1 file changed, 33 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 326b302fc440..f92dfe213d89 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1259,6 +1259,38 @@ static int dwc3_probe(struct platform_device
*pdev)
   return ret;
}

+static void dwc3_shutdown(struct platform_device *pdev)
+{
+   struct dwc3 *dwc = platform_get_drvdata(pdev);
+   struct resource *res = platform_get_resource(pdev,
IORESOURCE_MEM, 0);
+
+   pm_runtime_get_sync(>dev);
+   /*
+* restore res->start back to its original value so that, in case
the
+* probe is deferred, we don't end up getting error in request
the
+* memory region the next time probe is called.
+*/
+   res->start -= DWC3_GLOBALS_REGS_START;
+
+   dwc3_debugfs_exit(dwc);
+   dwc3_core_exit_mode(dwc);
+   dwc3_event_buffers_cleanup(dwc);



What about dwc3_event_buffers_cleanup? should I remove it from
dwc3_shutdown()?
It is already in dwc3_core_exit()


I think so. We should avoid duplicate code.


+   dwc3_free_event_buffers(dwc);
+
+   usb_phy_set_suspend(dwc->usb2_phy, 1);
+   usb_phy_set_suspend(dwc->usb3_phy, 1);
+
+   phy_power_off(dwc->usb2_generic_phy);
+   phy_power_off(dwc->usb3_generic_phy);



We've done these in dwc3_core_exit().


This is the patch after testing on a Odroid XU4, on top of linux-next
964bcc1b4f57028d56dace7d9bc5924f2eb43f36 which translates to 
4.13.0-rc1-next-20170717+
I tested this patch for a week without problems with heavy USB and NIC usage.
Please consider merging it


Author: Brian Kim 
Date:   Wed Jul 12 11:26:55 2017 +0800

  usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot
  The dwc3 could not release resources when the module is built-in
  because this module does not have shutdown method. This causes the USB
  3.0 hub is not able to detect after warm boot.
  Original patch by Brian Kim, updated and submitted upstream by gustavo
  panizzo.
  Also see https://bugs.debian.org/843448
  Signed-off-by: Brian Kim 
  Signed-off-by: gustavo panizzo 

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 326b302fc440..09de37d47ee7 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1259,6 +1259,32 @@ static int dwc3_probe(struct platform_device *pdev)
return ret;
}

+static void dwc3_shutdown(struct platform_device *pdev)
+{
+   struct dwc3 *dwc = platform_get_drvdata(pdev);
+   struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+   pm_runtime_get_sync(>dev);
+   /*
+* restore res->start back to its original value so that, in case the
+* probe is deferred, we don't end up getting error in request the
+* memory region the next time probe is called.
+*/
+   res->start -= DWC3_GLOBALS_REGS_START;
+
+   dwc3_debugfs_exit(dwc);
+   dwc3_core_exit_mode(dwc);
+   dwc3_event_buffers_cleanup(dwc);
+   dwc3_free_event_buffers(dwc);
+
+   dwc3_core_exit(dwc);
+   dwc3_ulpi_exit(dwc);
+
+   pm_runtime_put_sync(>dev);
+   pm_runtime_allow(>dev);
+   pm_runtime_disable(>dev);
+}
+
static int dwc3_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
@@ -1488,6 +1514,7 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
static struct platform_driver dwc3_driver = {
   .probe  = dwc3_probe,
   .remove = dwc3_remove,
+   .shutdown   = dwc3_shutdown,
   .driver = {
   .name   = "dwc3",
   .of_match_table = of_match_ptr(of_dwc3_match),

Patch applies cleanly on top of c6be5a0e3cebc145127d46a58350e05d2bcf6323 from 
linux-next
Can you _please_ merge it?




Author: gustavo panizzo 
Date:   Wed Jul 12 11:26:55 2017 +0800

   usb: dwc3: Fix the USB 3.0 hub detection bug after warm boot

   The dwc3 could not release resources when the module is built-in
   because this module does not have shutdown method. This causes the USB
   3.0 hub is not able to detect 

Re: [PATCH] usb: dwc3: gadget: only unmap requests from DMA if mapped

[Jack Pham]

On Thu, Sep 07, 2017 at 02:20:17AM +, Thinh Nguyen wrote:
> Hi,
> 
> On 6/29/2017 12:54 AM, Jack Pham wrote:
> > A recent optimization was made so that a request put on the
> > pending_list wouldn't get mapped for DMA until just before
> > preparing a TRB for it. However, this poses a problem in case
> > the request is dequeued or the endpoint is disabled before the
> > mapping is done as that would lead to dwc3_gadget_giveback()
> > unconditionally calling usb_gadget_unmap_request_for_dev() with
> > an invalid request->dma handle. Depending on the platform's DMA
> > implementation the unmap operation could result in a panic.
> > 
> > Since we know a successful mapping is a prerequisite for getting
> > a TRB, the unmap can be conditionally called only when req->trb
> > is non-NULL.
> > 
> > Fixes: cdb55b39fab8 ("usb: dwc3: gadget: lazily map requests for DMA")
> > Signed-off-by: Jack Pham 
> > ---
> >   drivers/usb/dwc3/gadget.c | 8 +---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 9e41605a..6b299c7 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -191,14 +191,16 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct 
> > dwc3_request *req,
> >   
> > req->started = false;
> > list_del(>list);
> > -   req->trb = NULL;
> > req->remaining = 0;
> >   
> > if (req->request.status == -EINPROGRESS)
> > req->request.status = status;
> >   
> > -   usb_gadget_unmap_request_by_dev(dwc->sysdev,
> > -   >request, req->direction);
> > +   if (req->trb)
> This check does not account for control data transfer. TRBs for ep0 are 
> not set to its req->trb. ep0out request needs to be unmapped, otherwise 
> device will receive bogus data.
> 
> Our internal test showed that the device failed to interpret control 
> data from host. I bisected to this patch.

Hi Thinh,

Thanks for catching this. I can think of two ways to address this:

1. Make sure req->trb is populated for ep0/1 as well. This should be
easily done since the TRB corresponding to the mapped buffer is always
dwc->ep0_trb. We can assign the pointer after each of the map_request()
calls in __dwc3_ep0_do_control_data. And req->trb already gets zeroed
in dwc3_giveback() already. Hopefully this can be taken for 4.13.y
stable as well.

2. In 4.14-rc1 there is now commit 31fe084ffaaf ("usb: gadget: core:
unmap request from DMA only if previously mapped") which handles
$SUBJECT in a generic way so obviates the need for this patch, so
maybe this patch can simply be reverted. However this might not backport
so well for 4.13.y since reverting would bring us back to the behavior I
originally intended to fix.

Felipe, what do you think?

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html