ehci-usb regression on 32-bit laptops in v4.17-rc1

* ehci-usb regression on 32-bit laptops in v4.17-rc1
@ 2018-05-04 17:27 Greg Kroah-Hartman
  0 siblings, 0 replies; 3+ messages in thread
From: Greg Kroah-Hartman @ 2018-05-04 17:27 UTC (permalink / raw)
  To: Alan Stern; +Cc: Erick Cafferata, Souptick Joarder, USB list

On Fri, May 04, 2018 at 10:39:07AM -0400, Alan Stern wrote:
> You should have CC'ed the author of the offending commit.  Added.
> 
> On Thu, 3 May 2018, Erick Cafferata wrote:
> 
> > Starting from v4.17-rc1, I've been having problems with my laptop's
> > webcam. Immediately after attempting to open Guvcview(or any other camera
> > software), the system just stops working. No response from any input and
> > the Caps lock LED starts blinking.
> > I've been following the last three -rc and no changes, so I started
> > testing on my own. I bisected it back to
> > 
> > commit 22072e83ebd510fb6a090aef9d65ccfda9b1e7e4
> > Author: Souptick Joarder <jrdr.linux@gmail.com>
> > Date:   Wed Feb 14 23:18:48 2018 +0530
> > 
> >     usb: host: ehci: Use dma_pool_zalloc()
> > 
> >     Use dma_pool_zalloc() instead of dma_pool_alloc + memset
> > 
> > I tried reverting it on top of -rc3 and everything worked again.
> > I test this bug on four machines:
> > - 2 hp mini 210(atom n450 and n2800, respectively)
> > - 1 Sony Vaio (Core 2 Duo T7250)
> > - 1 Thinkpad T410 (some core i7..)
> > The three 32-bit laptops presented the exact same bug, however the
> > Thinkpad did not crash (the driver didn't create the /dev/videoN at
> > all, I think this might be a different issue, so disregard).
> > And reverting this commit solved the issue in all three laptops.
> > 
> > Let me know if you need any further information regarding this issue.
> > Sincerely,
> > Erick Cafferata
> 
> Right you are!  I should never have approved that patch in the first 
> place.  :-(
> 
> > commit d3c88476223b51d2a0a1bc2326760c0dcb6efd88
> > Author: Erick Cafferata <erick@cafferata.me>
> > Date:   Wed May 2 11:13:32 2018 -0500
> > 
> >     Revert "usb: host: ehci: Use dma_pool_zalloc()"
> >     
> >     This reverts commit 22072e83ebd510fb6a090aef9d65ccfda9b1e7e4.
> > 
> > diff --git a/drivers/usb/host/ehci-mem.c b/drivers/usb/host/ehci-mem.c
> > index 4c6c08b675b5..21307d862af6 100644
> > --- a/drivers/usb/host/ehci-mem.c
> > +++ b/drivers/usb/host/ehci-mem.c
> > @@ -73,9 +73,10 @@ static struct ehci_qh *ehci_qh_alloc (struct ehci_hcd *ehci, gfp_t flags)
> >  	if (!qh)
> >  		goto done;
> >  	qh->hw = (struct ehci_qh_hw *)
> > -		dma_pool_zalloc(ehci->qh_pool, flags, &dma);
> > +		dma_pool_alloc(ehci->qh_pool, flags, &dma);
> >  	if (!qh->hw)
> >  		goto fail;
> > +	memset(qh->hw, 0, sizeof *qh->hw);
> 
> In fact, this part was okay.  It does not need to be reverted.
> 
> >  	qh->qh_dma = dma;
> >  	// INIT_LIST_HEAD (&qh->qh_list);
> >  	INIT_LIST_HEAD (&qh->qtd_list);
> > diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> > index 28e2a338b481..e56db44708bc 100644
> > --- a/drivers/usb/host/ehci-sched.c
> > +++ b/drivers/usb/host/ehci-sched.c
> > @@ -1287,7 +1287,7 @@ itd_urb_transaction(
> >  		} else {
> >   alloc_itd:
> >  			spin_unlock_irqrestore(&ehci->lock, flags);
> > -			itd = dma_pool_zalloc(ehci->itd_pool, mem_flags,
> > +			itd = dma_pool_alloc(ehci->itd_pool, mem_flags,
> >  					&itd_dma);
> >  			spin_lock_irqsave(&ehci->lock, flags);
> >  			if (!itd) {
> > @@ -1297,6 +1297,7 @@ itd_urb_transaction(
> >  			}
> >  		}
> >  
> > +		memset(itd, 0, sizeof(*itd));
> >  		itd->itd_dma = itd_dma;
> >  		itd->frame = NO_FRAME;
> >  		list_add(&itd->itd_list, &sched->td_list);
> > @@ -2080,7 +2081,7 @@ sitd_urb_transaction(
> >  		} else {
> >   alloc_sitd:
> >  			spin_unlock_irqrestore(&ehci->lock, flags);
> > -			sitd = dma_pool_zalloc(ehci->sitd_pool, mem_flags,
> > +			sitd = dma_pool_alloc(ehci->sitd_pool, mem_flags,
> >  					&sitd_dma);
> >  			spin_lock_irqsave(&ehci->lock, flags);
> >  			if (!sitd) {
> > @@ -2090,6 +2091,7 @@ sitd_urb_transaction(
> >  			}
> >  		}
> >  
> > +		memset(sitd, 0, sizeof(*sitd));
> >  		sitd->sitd_dma = sitd_dma;
> >  		sitd->frame = NO_FRAME;
> >  		list_add(&sitd->sitd_list, &iso_sched->td_list);
> 
> But these parts were definitely wrong.
> 
> What you can't see just from reading the patch is that in both cases
> (ehci->itd_pool and ehci->sitd_pool) there are two allocation paths --
> the two branches of an "if" statement -- and only one of the paths
> calls dma_pool_[z]alloc.  However, the memset is needed for both paths, 
> and so it can't be eliminated.  Given that it must be present, there's 
> no advantage to calling dma_pool_zalloc rather than dma_pool_alloc.
> 
> Can you try reverting just these portions while leaving the first part 
> unchanged?

Ugh, I missed that.  I'll revert this patch for now.

thanks,

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

^ permalink raw reply	[flat|nested] 3+ messages in thread