[PATCH 2/8] xhci: apply PME_STUCK_QUIRK and MISSING_CAS quirk for Denverton

[PATCH 2/8] xhci: apply PME_STUCK_QUIRK and MISSING_CAS quirk for Denverton

[Mathias Nyman]

Intel Denverton microserver is Atom based and need the PME and CAS quirks
as well.

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 7b86508..fcf1f3f 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -52,6 +52,7 @@
 #define PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI		0x0aa8
 #define PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI		0x1aa8
 #define PCI_DEVICE_ID_INTEL_APL_XHCI			0x5aa8
+#define PCI_DEVICE_ID_INTEL_DNV_XHCI			0x19d0
 
 static const char hcd_name[] = "xhci_hcd";
 
@@ -166,7 +167,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		 pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
 		 pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI ||
 		 pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI ||
-		 pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) {
+		 pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI ||
+		 pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI)) {
 		xhci->quirks |= XHCI_PME_STUCK_QUIRK;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
@@ -175,7 +177,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
 	    (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
-	     pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI))
+	     pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI ||
+	     pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI))
 		xhci->quirks |= XHCI_MISSING_CAS;
 
 	if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
-- 
1.9.1

[PATCH 3/8] usb: host: xhci-mem: allocate zeroed Scratchpad Buffer

[Mathias Nyman]

From: Peter Chen <peter.chen@nxp.com>

According to xHCI ch4.20 Scratchpad Buffers, the Scratchpad
Buffer needs to be zeroed.

	...
	The following operations take place to allocate
       	Scratchpad Buffers to the xHC:
	...
		b. Software clears the Scratchpad Buffer to '0'

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 69428e9..12b573c 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1724,7 +1724,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
 	xhci->dcbaa->dev_context_ptrs[0] = cpu_to_le64(xhci->scratchpad->sp_dma);
 	for (i = 0; i < num_sp; i++) {
 		dma_addr_t dma;
-		void *buf = dma_alloc_coherent(dev, xhci->page_size, &dma,
+		void *buf = dma_zalloc_coherent(dev, xhci->page_size, &dma,
 				flags);
 		if (!buf)
 			goto fail_sp4;
-- 
1.9.1

[PATCH 5/8] USB: xhci: fix lock-inversion problem

[Mathias Nyman]

From: Alan Stern <stern@rowland.harvard.edu>

With threaded interrupts, bottom-half handlers are called with
interrupts enabled.  Therefore they can't safely use spin_lock(); they
have to use spin_lock_irqsave().  Lockdep warns about a violation
occurring in xhci_irq():

=========================================================
[ INFO: possible irq lock inversion dependency detected ]
4.11.0-rc8-dbg+ #1 Not tainted
---------------------------------------------------------
swapper/7/0 just changed the state of lock:
 (&(&ehci->lock)->rlock){-.-...}, at: [<ffffffffa0130a69>]
ehci_hrtimer_func+0x29/0xc0 [ehci_hcd]
but this lock took another, HARDIRQ-unsafe lock in the past:
 (hcd_urb_list_lock){+.....}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
 Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(hcd_urb_list_lock);
                               local_irq_disable();
                               lock(&(&ehci->lock)->rlock);
                               lock(hcd_urb_list_lock);
  <Interrupt>
    lock(&(&ehci->lock)->rlock);
 *** DEADLOCK ***

no locks held by swapper/7/0.
the shortest dependencies between 2nd lock and 1st lock:
 -> (hcd_urb_list_lock){+.....} ops: 252 {
    HARDIRQ-ON-W at:
                      __lock_acquire+0x602/0x1280
                      lock_acquire+0xd5/0x1c0
                      _raw_spin_lock+0x2f/0x40
                      usb_hcd_unlink_urb_from_ep+0x1b/0x60 [usbcore]
                      xhci_giveback_urb_in_irq.isra.45+0x70/0x1b0 [xhci_hcd]
                      finish_td.constprop.60+0x1d8/0x2e0 [xhci_hcd]
                      xhci_irq+0xdd6/0x1fa0 [xhci_hcd]
                      usb_hcd_irq+0x26/0x40 [usbcore]
                      irq_forced_thread_fn+0x2f/0x70
                      irq_thread+0x149/0x1d0
                      kthread+0x113/0x150
                      ret_from_fork+0x2e/0x40

This patch fixes the problem.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-and-tested-by: Bart Van Assche <bart.vanassche@sandisk.com>
CC: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0830b25..6d2492c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2677,11 +2677,12 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	union xhci_trb *event_ring_deq;
 	irqreturn_t ret = IRQ_NONE;
+	unsigned long flags;
 	dma_addr_t deq;
 	u64 temp_64;
 	u32 status;
 
-	spin_lock(&xhci->lock);
+	spin_lock_irqsave(&xhci->lock, flags);
 	/* Check if the xHC generated the interrupt, or the irq is shared */
 	status = readl(&xhci->op_regs->status);
 	if (status == ~(u32)0) {
@@ -2754,7 +2755,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 	ret = IRQ_HANDLED;
 
 out:
-	spin_unlock(&xhci->lock);
+	spin_unlock_irqrestore(&xhci->lock, flags);
 
 	return ret;
 }
-- 
1.9.1

[PATCH 6/8] xhci: remove GFP_DMA flag from allocation

[Mathias Nyman]

From: Matthias Lange <matthias.lange@kernkonzept.com>

There is no reason to restrict allocations to the first 16MB ISA DMA
addresses.

It is causing problems in a virtualization setup with enabled IOMMU
(x86_64). The result is that USB is not working in the VM.

CC: <stable@vger.kernel.org>
Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 12b573c..1f1687e 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -56,7 +56,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
 	}
 
 	if (max_packet) {
-		seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
+		seg->bounce_buf = kzalloc(max_packet, flags);
 		if (!seg->bounce_buf) {
 			dma_pool_free(xhci->segment_pool, seg->trbs, dma);
 			kfree(seg);
-- 
1.9.1

[PATCH 7/8] xhci: Fix command ring stop regression in 4.11

[Mathias Nyman]

In 4.11 TRB completion codes were renamed to match spec.

Completion codes for command ring stopped and endpoint stopped
were mixed, leading to failures while handling a stopped command ring.

Use the correct completion code for command ring stopped events.

Fixes: 0b7c105a04ca ("usb: host: xhci: rename completion codes to match spec")
Cc: <stable@vger.kernel.org> # 4.11
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c  | 2 +-
 drivers/usb/host/xhci-ring.c | 8 ++++----
 drivers/usb/host/xhci.c      | 8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 5e3e9d4..0dde49c 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -419,7 +419,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 	wait_for_completion(cmd->completion);
 
 	if (cmd->status == COMP_COMMAND_ABORTED ||
-			cmd->status == COMP_STOPPED) {
+	    cmd->status == COMP_COMMAND_RING_STOPPED) {
 		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
 		ret = -ETIME;
 	}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6d2492c..03f63f5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -323,7 +323,7 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 		if (i_cmd->status != COMP_COMMAND_ABORTED)
 			continue;
 
-		i_cmd->status = COMP_STOPPED;
+		i_cmd->status = COMP_COMMAND_RING_STOPPED;
 
 		xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
 			 i_cmd->command_trb);
@@ -1380,7 +1380,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 	cmd_comp_code = GET_COMP_CODE(le32_to_cpu(event->status));
 
 	/* If CMD ring stopped we own the trbs between enqueue and dequeue */
-	if (cmd_comp_code == COMP_STOPPED) {
+	if (cmd_comp_code == COMP_COMMAND_RING_STOPPED) {
 		complete_all(&xhci->cmd_ring_stop_completion);
 		return;
 	}
@@ -1436,8 +1436,8 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 		break;
 	case TRB_CMD_NOOP:
 		/* Is this an aborted command turned to NO-OP? */
-		if (cmd->status == COMP_STOPPED)
-			cmd_comp_code = COMP_STOPPED;
+		if (cmd->status == COMP_COMMAND_RING_STOPPED)
+			cmd_comp_code = COMP_COMMAND_RING_STOPPED;
 		break;
 	case TRB_RESET_EP:
 		WARN_ON(slot_id != TRB_TO_SLOT_ID(
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 71eb299..30f47d9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1764,7 +1764,7 @@ static int xhci_configure_endpoint_result(struct xhci_hcd *xhci,
 
 	switch (*cmd_status) {
 	case COMP_COMMAND_ABORTED:
-	case COMP_STOPPED:
+	case COMP_COMMAND_RING_STOPPED:
 		xhci_warn(xhci, "Timeout while waiting for configure endpoint command\n");
 		ret = -ETIME;
 		break;
@@ -1814,7 +1814,7 @@ static int xhci_evaluate_context_result(struct xhci_hcd *xhci,
 
 	switch (*cmd_status) {
 	case COMP_COMMAND_ABORTED:
-	case COMP_STOPPED:
+	case COMP_COMMAND_RING_STOPPED:
 		xhci_warn(xhci, "Timeout while waiting for evaluate context command\n");
 		ret = -ETIME;
 		break;
@@ -3433,7 +3433,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
 	ret = reset_device_cmd->status;
 	switch (ret) {
 	case COMP_COMMAND_ABORTED:
-	case COMP_STOPPED:
+	case COMP_COMMAND_RING_STOPPED:
 		xhci_warn(xhci, "Timeout waiting for reset device command\n");
 		ret = -ETIME;
 		goto command_cleanup;
@@ -3818,7 +3818,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 	 */
 	switch (command->status) {
 	case COMP_COMMAND_ABORTED:
-	case COMP_STOPPED:
+	case COMP_COMMAND_RING_STOPPED:
 		xhci_warn(xhci, "Timeout while waiting for setup device command\n");
 		ret = -ETIME;
 		break;
-- 
1.9.1

[PATCH 8/8] usb: host: xhci-plat: propagate return value of platform_get_irq()

[Mathias Nyman]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

platform_get_irq() returns an error code, but the xhci-plat driver
ignores it and always returns -ENODEV. This is not correct, and
prevents -EPROBE_DEFER from being propagated properly.

CC: <stable@vger.kernel.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-plat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 7c2a9e7..c04144b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -177,7 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
-		return -ENODEV;
+		return irq;
 
 	/*
 	 * sysdev must point to a device that is known to the system firmware
-- 
1.9.1

RE: [PATCH 6/8] xhci: remove GFP_DMA flag from allocation

[David Laight]

From: Mathias Nyman
> Sent: 17 May 2017 16:32
> There is no reason to restrict allocations to the first 16MB ISA DMA
> addresses.
> 
> It is causing problems in a virtualization setup with enabled IOMMU
> (x86_64). The result is that USB is not working in the VM.
...
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 12b573c..1f1687e 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -56,7 +56,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
>  	}
> 
>  	if (max_packet) {
> -		seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
> +		seg->bounce_buf = kzalloc(max_packet, flags);

This might allocate memory that the device cannot access.
So can only work if dma_map_single() itself allocates a bounce buffer.
There must be a sane way to do this that doesn't ever require
double copies.

	David

Re: [PATCH 6/8] xhci: remove GFP_DMA flag from allocation

[Mathias Nyman]

On 19.05.2017 12:10, David Laight wrote:
> From: Mathias Nyman
>> Sent: 17 May 2017 16:32
>> There is no reason to restrict allocations to the first 16MB ISA DMA
>> addresses.
>>
>> It is causing problems in a virtualization setup with enabled IOMMU
>> (x86_64). The result is that USB is not working in the VM.
> ...
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 12b573c..1f1687e 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -56,7 +56,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
>>   	}
>>
>>   	if (max_packet) {
>> -		seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
>> +		seg->bounce_buf = kzalloc(max_packet, flags);
>
> This might allocate memory that the device cannot access.
> So can only work if dma_map_single() itself allocates a bounce buffer.
> There must be a sane way to do this that doesn't ever require
> double copies.
>

We are using dma_map_single()
This allocated memory is used as the processor virtual memory required by dma_map_single()
i.e. the "void *cpu_addr" part of dma_map_single, see DMA-API.txt:


dma_map_single(struct device *dev, void *cpu_addr, size_t size,
                       enum dma_data_direction direction)

Maps a piece of processor virtual memory so it can be accessed by the
device and returns the DMA address of the memory.

...

Notes:  Not all memory regions in a machine can be mapped by this API.
Further, contiguous kernel virtual space may not be contiguous as
physical memory.  Since this API does not provide any scatter/gather
capability, it will fail if the user tries to map a non-physically
contiguous piece of memory.  For this reason, memory to be mapped by
this API should be obtained from sources which guarantee it to be
physically contiguous (like kmalloc).

I'm not fully sure I understand yout concern, are you thinking the driver should
doublecheck the dma address returned by dma_map_single() and make sure it's within the
dma mask set for the device?

-Mathias

RE: [PATCH 6/8] xhci: remove GFP_DMA flag from allocation

[David Laight]

From: Mathias Nyman
> Sent: 19 May 2017 10:49
> To: David Laight; gregkh@linuxfoundation.org
> Cc: linux-usb@vger.kernel.org; Matthias Lange; stable@vger.kernel.org
> Subject: Re: [PATCH 6/8] xhci: remove GFP_DMA flag from allocation
> 
> On 19.05.2017 12:10, David Laight wrote:
> > From: Mathias Nyman
> >> Sent: 17 May 2017 16:32
> >> There is no reason to restrict allocations to the first 16MB ISA DMA
> >> addresses.
> >>
> >> It is causing problems in a virtualization setup with enabled IOMMU
> >> (x86_64). The result is that USB is not working in the VM.
> > ...
> >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> >> index 12b573c..1f1687e 100644
> >> --- a/drivers/usb/host/xhci-mem.c
> >> +++ b/drivers/usb/host/xhci-mem.c
> >> @@ -56,7 +56,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
> >>   	}
> >>
> >>   	if (max_packet) {
> >> -		seg->bounce_buf = kzalloc(max_packet, flags | GFP_DMA);
> >> +		seg->bounce_buf = kzalloc(max_packet, flags);
> >
> > This might allocate memory that the device cannot access.
> > So can only work if dma_map_single() itself allocates a bounce buffer.
> > There must be a sane way to do this that doesn't ever require
> > double copies.
> >
> 
> We are using dma_map_single()
> This allocated memory is used as the processor virtual memory required by dma_map_single()
> i.e. the "void *cpu_addr" part of dma_map_single, see DMA-API.txt:
> 
> 
> dma_map_single(struct device *dev, void *cpu_addr, size_t size,
>                        enum dma_data_direction direction)
> 
> Maps a piece of processor virtual memory so it can be accessed by the
> device and returns the DMA address of the memory.
> 
> ...
> 
> Notes:  Not all memory regions in a machine can be mapped by this API.
> Further, contiguous kernel virtual space may not be contiguous as
> physical memory.  Since this API does not provide any scatter/gather
> capability, it will fail if the user tries to map a non-physically
> contiguous piece of memory.  For this reason, memory to be mapped by
> this API should be obtained from sources which guarantee it to be
> physically contiguous (like kmalloc).
> 
> I'm not fully sure I understand yout concern, are you thinking the driver should
> doublecheck the dma address returned by dma_map_single() and make sure it's within the
> dma mask set for the device?

The physical memory returned by kzalloc() (without GFP_DMA) can definitely be
outside the dma mask for the device.
If that happens something must allocate a dma bounce buffer, I'm guessing that
dma_map_single() does so - it certainly can since it can copy the data during
the unmap (for a read transfer).
So you really want to allocate a buffer that is within the devices dma window.

The buffer you are allocating isn't really a 'bounce' buffer at all - is it?
It is more of a de_fragmentation buffer.
It is there so that the LINK TRB will always be at a USB segment boundary.
(I'm guessing the bug I found several years ago still isn't fixed??)

	David