[PATCH] xhci: use dma_alloc_coherent with fully cleared pages

[PATCH] xhci: use dma_alloc_coherent with fully cleared pages

[Tim Chen]

Commit d92ef66c4f8f ("x86: make dma_alloc_coherent() return zeroed memory
if CMA is enabled") changed the dma_alloc_coherent page clearance from
using an __GFP_ZERO in page allocation to not setting the flag but doing
an explicit memory clear at the end.

However the memory clear only covered the memory size that
was requested, but may not be up to the full extent of the
last page, if the total pages returned exceed the
memory size requested.  This behavior has caused problem with XHCI
and caused it to hang:

kernel: xhci_hcd 0000:00:14.0: Stopped the command ring failed, maybe the host is dead
kernel: xhci_hcd 0000:00:14.0: Abort command ring failed
kernel: xhci_hcd 0000:00:14.0: HC died; cleaning up
kernel: xhci_hcd 0000:00:14.0: Error while assigning device slot ID
kernel: xhci_hcd 0000:00:14.0: Max number of devices this xHCI host supports is 64.

This patch reverts xhci the use the old behavior to avoid the hang.  XHCI should
do a proper fix to avoid this un-allocated memory.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
Cc: <stable@vger.kernel.org> # 3.16+
---
 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 5cb3d7a..39e7196 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1658,7 +1658,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
 		goto fail_sp;
 
 	xhci->scratchpad->sp_array = dma_alloc_coherent(dev,
-				     num_sp * sizeof(u64),
+				     PAGE_ALIGN(num_sp * sizeof(u64)),
 				     &xhci->scratchpad->sp_dma, flags);
 	if (!xhci->scratchpad->sp_array)
 		goto fail_sp2;
-- 
1.9.3

Re: [PATCH] xhci: use dma_alloc_coherent with fully cleared pages

[Mathias Nyman]

On 19.02.2015 02:34, Tim Chen wrote:
> 
> Commit d92ef66c4f8f ("x86: make dma_alloc_coherent() return zeroed memory
> if CMA is enabled") changed the dma_alloc_coherent page clearance from
> using an __GFP_ZERO in page allocation to not setting the flag but doing
> an explicit memory clear at the end.
> 
> However the memory clear only covered the memory size that
> was requested, but may not be up to the full extent of the
> last page, if the total pages returned exceed the
> memory size requested.  This behavior has caused problem with XHCI
> and caused it to hang:
> 
> kernel: xhci_hcd 0000:00:14.0: Stopped the command ring failed, maybe the host is dead
> kernel: xhci_hcd 0000:00:14.0: Abort command ring failed
> kernel: xhci_hcd 0000:00:14.0: HC died; cleaning up
> kernel: xhci_hcd 0000:00:14.0: Error while assigning device slot ID
> kernel: xhci_hcd 0000:00:14.0: Max number of devices this xHCI host supports is 64.
> 
> This patch reverts xhci the use the old behavior to avoid the hang.  XHCI should
> do a proper fix to avoid this un-allocated memory.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # 3.16+
> ---
>  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 5cb3d7a..39e7196 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1658,7 +1658,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
>  		goto fail_sp;
>  
>  	xhci->scratchpad->sp_array = dma_alloc_coherent(dev,
> -				     num_sp * sizeof(u64),
> +				     PAGE_ALIGN(num_sp * sizeof(u64)),
>  				     &xhci->scratchpad->sp_dma, flags);
>  	if (!xhci->scratchpad->sp_array)
>  		goto fail_sp2;
> 

Thanks for figuring out that the issue was in allocation of scratchpad memory.

Turns out we don't calculate the num_sp correctly in the first place, we didn't include
the additional high order field of the max scratchpad buffers xhci tells we should reserve.
I got a patch for this and will send it to forward to Greg once 3.20-rc1 is tagged

-Mathias

Re: [PATCH] xhci: use dma_alloc_coherent with fully cleared pages

[Tim Chen]

On Thu, 2015-02-19 at 17:05 +0200, Mathias Nyman wrote:
> On 19.02.2015 02:34, Tim Chen wrote:
> > 
> > Commit d92ef66c4f8f ("x86: make dma_alloc_coherent() return zeroed memory
> > if CMA is enabled") changed the dma_alloc_coherent page clearance from
> > using an __GFP_ZERO in page allocation to not setting the flag but doing
> > an explicit memory clear at the end.
> > 
> > However the memory clear only covered the memory size that
> > was requested, but may not be up to the full extent of the
> > last page, if the total pages returned exceed the
> > memory size requested.  This behavior has caused problem with XHCI
> > and caused it to hang:
> > 
> > kernel: xhci_hcd 0000:00:14.0: Stopped the command ring failed, maybe the host is dead
> > kernel: xhci_hcd 0000:00:14.0: Abort command ring failed
> > kernel: xhci_hcd 0000:00:14.0: HC died; cleaning up
> > kernel: xhci_hcd 0000:00:14.0: Error while assigning device slot ID
> > kernel: xhci_hcd 0000:00:14.0: Max number of devices this xHCI host supports is 64.
> > 
> > This patch reverts xhci the use the old behavior to avoid the hang.  XHCI should
> > do a proper fix to avoid this un-allocated memory.
> > 
> > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # 3.16+
> > ---
> >  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 5cb3d7a..39e7196 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -1658,7 +1658,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
> >  		goto fail_sp;
> >  
> >  	xhci->scratchpad->sp_array = dma_alloc_coherent(dev,
> > -				     num_sp * sizeof(u64),
> > +				     PAGE_ALIGN(num_sp * sizeof(u64)),
> >  				     &xhci->scratchpad->sp_dma, flags);
> >  	if (!xhci->scratchpad->sp_array)
> >  		goto fail_sp2;
> > 
> 
> Thanks for figuring out that the issue was in allocation of scratchpad memory.
> 
> Turns out we don't calculate the num_sp correctly in the first place, we didn't include
> the additional high order field of the max scratchpad buffers xhci tells we should reserve.
> I got a patch for this and will send it to forward to Greg once 3.20-rc1 is tagged
> 
> -Mathias

Great. Thanks for fixing this.

Tim

Re: [PATCH] xhci: use dma_alloc_coherent with fully cleared pages

[Andi Kleen]

> Great. Thanks for fixing this.

Not great.
This still leaves all the other drivers which may be affected by similar problems.
IMHO Tim's original patch is still needed.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH] xhci: use dma_alloc_coherent with fully cleared pages

[Tim Chen]

On Thu, 2015-02-19 at 17:05 +0200, Mathias Nyman wrote:
> On 19.02.2015 02:34, Tim Chen wrote:
> > 
> > Commit d92ef66c4f8f ("x86: make dma_alloc_coherent() return zeroed memory
> > if CMA is enabled") changed the dma_alloc_coherent page clearance from
> > using an __GFP_ZERO in page allocation to not setting the flag but doing
> > an explicit memory clear at the end.
> > 
> > However the memory clear only covered the memory size that
> > was requested, but may not be up to the full extent of the
> > last page, if the total pages returned exceed the
> > memory size requested.  This behavior has caused problem with XHCI
> > and caused it to hang:
> > 
> > kernel: xhci_hcd 0000:00:14.0: Stopped the command ring failed, maybe the host is dead
> > kernel: xhci_hcd 0000:00:14.0: Abort command ring failed
> > kernel: xhci_hcd 0000:00:14.0: HC died; cleaning up
> > kernel: xhci_hcd 0000:00:14.0: Error while assigning device slot ID
> > kernel: xhci_hcd 0000:00:14.0: Max number of devices this xHCI host supports is 64.
> > 
> > This patch reverts xhci the use the old behavior to avoid the hang.  XHCI should
> > do a proper fix to avoid this un-allocated memory.
> > 
> > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # 3.16+
> > ---
> >  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 5cb3d7a..39e7196 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -1658,7 +1658,7 @@ static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
> >  		goto fail_sp;
> >  
> >  	xhci->scratchpad->sp_array = dma_alloc_coherent(dev,
> > -				     num_sp * sizeof(u64),
> > +				     PAGE_ALIGN(num_sp * sizeof(u64)),
> >  				     &xhci->scratchpad->sp_dma, flags);
> >  	if (!xhci->scratchpad->sp_array)
> >  		goto fail_sp2;
> > 
> 
> Thanks for figuring out that the issue was in allocation of scratchpad memory.
> 
> Turns out we don't calculate the num_sp correctly in the first place, we didn't include
> the additional high order field of the max scratchpad buffers xhci tells we should reserve.
> I got a patch for this and will send it to forward to Greg once 3.20-rc1 is tagged
> 
> -Mathias

Confirmed that the patch:  xhci: Allocate correct amount of scratchpad buffers did resolve
the problem I've seen for XHCI.

Tim