Hi Felipe, This patch brings up an interesting point: will a dwc3 PCI host use the xhci platform driver instead of the xhci PCI driver? If so, that seems less than ideal. Won't it not use the standard xHCI PCI suspend and resume functions if it's registered as a platform device? Also, it seems registering it that way means XHCI_BROKEN_MSI is set unconditionally. That leads to the xhci driver not enabling MSI or MSI-X for the PCI host, which will impact performance. Hi Yu, Thanks for taking this bug down. I have some process-oriented issues that need to be addressed, and some comments that need to be addressed in the next version of your patch. First, please use this email address to send me patches: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> I use the linux.intel.com email address to handle all patches and traffic to the Linux mailing lists. My intel.com email address is for Intel internal communications only. In order for us to apply patches, you need to send them inline in the email, without your introduction. The subject line must be the subject line of the patch. Basically, you need to use either `git send-email` or `git format-patch` and mutt to send the patch. Please see this tutorial for more information on sending proper patches: http://kernelnewbies.org/OPWfirstpatch#head-2043a643d048c341b2a52044fd72d852aac87fef If you still need to introduce a patch, you can put this "scissors" line between your introduction and the patch description: >8-------------------------------------------------------------8< However, in general, your patch description should contain all the information necessary for us to decide whether we need to apply the patch. If the bug was only hit with a specific configuration, that needs to be included in the patch description, so that distros know whether they need to apply the patch. Comments on the patch below. On Tue, Aug 06, 2013 at 11:08:04PM -0700, Wang, Yu Y wrote: > Hi Balbi, Sarah, > > > > I found that when CONFIG_PCI is set, and xHCI driver register as platform > device driver. > > Then xhci ISR will be register twice. > > The first time is in usb_add_hcd, the second time is in xhci_run > (xhci_try_enable_msi). > > > > This issue should be reproduce when dwc3 driver registered as PCI device > driver. > > And CONFIG_USB_DWC3_DUAL_ROLE is set. This information should all be in your patch description. It's important to document how to reproduce the bug you're fixing in the patch that fixes it. > Fixed patch please help review: Hopefully when you use `git send-email` or mutt you won't get these extra newlines. Don't send patches through outlook, it completely mangles them. You'll need to set up esmtp on a Linux system in order to be able to send mail to the Intel exchange servers with `git send-email` or mutt. > From 8b437ac59be296cd7c0fa189344f3b30281fb3f1 Mon Sep 17 00:00:00 2001 > > From: Wang, Yu <yu.y.wang@xxxxxxxxx> > > Date: Wed, 7 Aug 2013 13:26:30 +0800 > > Subject: [PATCH 5/6] xhci: xHCI ISR be registers twice. > > > > This is one xHCI driver BUG. If CONFIG_PCI be set and xHCI driver > > registers as platform driver. Then xHCI ISR will be registers twice, the > > first time is during usb_add_hcd. The second time is in xhci_run. > You'll need a newline between your description and the Signed-off-by line. I can't tell if you currently have one because of the mangled patch. > Signed-off-by: Wang, Yu <yu.y.wang@xxxxxxxxx> > > > > Change-Id: Ibf86bca8f099586a0f379ee843b95c4d93b88d67 > > --- > > drivers/usb/host/xhci.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index d8f640b..b43f722 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -372,6 +372,10 @@ static int xhci_try_enable_msi(struct usb_hcd *hcd) > > } > > > > legacy_irq: > > + /* The leacy irq was already registered in USB core */ > > + if (hcd->irq) > > + return 0; > > + Let's take a look at the function you're patching: static int xhci_try_enable_msi(struct usb_hcd *hcd) { struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); int ret; /* * Some Fresco Logic host controllers advertise MSI, but fail to * generate interrupts. Don't even try to enable MSI. */ if (xhci->quirks & XHCI_BROKEN_MSI) goto legacy_irq; /* unregister the legacy interrupt */ if (hcd->irq) free_irq(hcd->irq, hcd); hcd->irq = 0; ret = xhci_setup_msix(xhci); if (ret) /* fall back to msi*/ ret = xhci_setup_msi(xhci); if (!ret) /* hcd->irq is 0, we have MSI */ return 0; if (!pdev->irq) { xhci_err(xhci, "No msi-x/msi found and no IRQ in BIOS\n"); return -EINVAL; } legacy_irq: /* fall back to legacy interrupt*/ ret = request_irq(pdev->irq, &usb_hcd_irq, IRQF_SHARED, hcd->irq_descr, hcd); if (ret) { xhci_err(xhci, "request interrupt %d failed\n", pdev->irq); return ret; } hcd->irq = pdev->irq; return 0; } The function frees the legacy PCI interrupt unless the XHCI_BROKEN_MSI quirk is set. In that case, yes, it looks like the code will request the PCI interrupt twice. However, the fix should be to move the legacy_irq label down to the last return statement, not to return early within that label if hcd->irq is set. That's a cleaner solution, since the XHCI_BROKEN_MSI case is the only code that uses that label. I see that xhci-plat sets XHCI_BROKEN_MSI unconditionally. Is your dwc3 host actually a PCI device, but uses the xHCI platform driver? That seems odd to me, and probably should be fixed. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
