> 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. > [Yu:] The upstream dwc3 driver haven't enable power management for the host mode until now. Actually, this is another big changes. In Intel intern tree, I have to wrote another separate file to re-implemented the PM callback to support platform device design. Maybe we need consider to add one new file(hcd-plat.c) which is familiar with hcd-pci.c? > > 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. [Yu:] Ok. Thanks for your review. Sorry, I am newcomer of the community. I will setup environment followed the guide and re-submit one new 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-2043a643d048c341b2a52044fd > 72d852aac87fef > > 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. > [Yu:] Get it. I will provide more details in the comment. > 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. [Yu:] Sorry. I haven't got what is your mean. The legacy_irq label to which line? Actually, this issue is not only register ISR twice. This issue will cause host initialization failed. Because the structure device will be convert to pci_dev which is not pci_dev. Then pdev->irq will be one random value which is invalid IRQ number. Then the second request_irq will be failed, and host_run will return error cause host initialization failed. I think we need to think a better solution to distinguish xHCI platform device and xHCI PCI device. Only use CONFIG_PCI is not enough. > > 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. [Yu:] To my understanding, the platform design is make clear with OTG architecture. For example, in Intel mobile platforms, we have OTG/HOST/DEVICE driver matched one real PCI USB OTG controller. Then we design OTG driver register as PCI device driver, and HOST/DEVICE drivers registers as platform device drivers whose matched platform devices are the children of OTG PCI device. This design make architecture clear, and also make the implementation of runtime pm clear for these three virtual devices. > > 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
