| Message ID | 20180504013800.mtidmwujbub5b744@YUKI.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Alan Stern -- 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
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
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); 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);
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 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. -- 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