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.li...@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 <er...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html