Comments below.
On Mon, May 10, 2010 at 07:03:42PM +0800, Andiry Xu wrote:
> >From f00f3c5df2393291a20f8b515d79d2d5638a85d1 Mon Sep 17 00:00:00 2001
> From: Dong Nguyen <Dong.Nguyen@xxxxxxx>
> Date: Mon, 10 May 2010 18:35:39 +0800
> Subject: [PATCH] xHCI: supporting MSI/MSI-X
>
> Enable MSI/MSI-X supporting in xhci driver.
>
> It also provides the mechanism to fall back using MSI and Legacy IRQs
> if MSI-X IRQs register failed.
>
> Signed-off-by: Dong Nguyen <Dong.Nguyen@xxxxxxx>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-pci.c | 1 -
> drivers/usb/host/xhci.c | 152 ++++++++++++++++++++++++++++++++----------
> drivers/usb/host/xhci.h | 2 +-
> 3 files changed, 117 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 9714db9..f319112 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -113,7 +113,6 @@ static const struct hc_driver xhci_pci_hc_driver = {
> /*
> * generic hardware linkage
> */
> - .irq = xhci_irq,
> .flags = HCD_MEMORY | HCD_USB3,
>
> /*
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a22a1e6..43d7fc4 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -20,6 +20,7 @@
> * Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#include <linux/pci.h>
> #include <linux/irq.h>
> #include <linux/log2.h>
> #include <linux/module.h>
> @@ -33,6 +34,7 @@
>
> /* Some 0.95 hardware can't handle the chain bit on a Link TRB being cleared */
> static int link_quirk;
> +static irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd);
Can you move xhci_msi_irq() before xhci_free_irq() so that you don't
need this line?
> module_param(link_quirk, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
>
> @@ -133,22 +135,78 @@ int xhci_reset(struct xhci_hcd *xhci)
> return handshake(xhci, &xhci->op_regs->command, CMD_RESET, 0, 250 * 1000);
> }
>
> +/*
> + * Free IRQs
> + * free all IRQs request
> + */
> +static void xhci_free_irq(struct xhci_hcd *xhci)
> +{
> + int i;
> + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> +
> + if (xhci->msix_entries) {
> + for (i = 0; i < xhci->msix_count; i++)
> + if (xhci->msix_entries[i].vector)
> + free_irq(xhci->msix_entries[i].vector,
> + xhci_to_hcd(xhci));
> + } else {
> + free_irq(pdev->irq, xhci_to_hcd(xhci));
> + }
> +
> + return;
> +}
> +
> +/*
> + * Set up MSI
> + */
> +static int xhci_setup_msi(struct xhci_hcd *xhci)
> +{
> + int ret;
> + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> +
> + ret = pci_enable_msi(pdev);
> + if (ret) {
> + xhci_err(xhci, "failed to allocate MSI entry\n");
> + return ret;
> + }
> +
> + ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
> + 0, "xhci_hcd", xhci_to_hcd(xhci));
> + if (ret) {
> + xhci_err(xhci, "disable MSI interrupt\n");
> + pci_disable_msi(pdev);
> + }
> +
> + return ret;
> +}
>
> -#if 0
> -/* Set up MSI-X table for entry 0 (may claim other entries later) */
> +/*
> + * Set up MSI-X
> + */
> static int xhci_setup_msix(struct xhci_hcd *xhci)
> {
> - int ret;
> + int i, ret = 0;
> + int cpus = num_online_cpus();
> + int msix_vecs, max_vecs;
> struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
>
> - xhci->msix_count = 0;
> - /* XXX: did I do this right? ixgbe does kcalloc for more than one */
> - xhci->msix_entries = kmalloc(sizeof(struct msix_entry), GFP_KERNEL);
> + xhci->msix_entries = NULL;
No need to set this to NULL here, since usb_create_hcd() in
drivers/usb/core/hcd.c uses kzalloc() to create xhci_hcd.
> + max_vecs = HCS_MAX_INTRS(xhci->hcs_params1);
> + msix_vecs = min(cpus + 1, max_vecs);
> + xhci->msix_count = msix_vecs;
Why cpus + 1? Please add a comment. Also add a comment about what
HCS_MAX_INTRS means (the maximum number of interrupters the host can
handle).
Why not directly assign to xhci->msix_count instead of using the extra
variables cpus, max_vecs and msix_vecs? Like so:
xhci->msix_count = min(num_online_cpus() + 1,
HCS_MAX_INTRS(xhci->hcs_params1);
> +
> + xhci->msix_entries =
> + kmalloc((sizeof(struct msix_entry))*xhci->msix_count,
> + GFP_KERNEL);
> if (!xhci->msix_entries) {
> xhci_err(xhci, "Failed to allocate MSI-X entries\n");
> return -ENOMEM;
> }
> - xhci->msix_entries[0].entry = 0;
> +
> + for (i = 0; i < xhci->msix_count; i++) {
> + xhci->msix_entries[i].entry = i;
> + xhci->msix_entries[i].vector = 0;
> + }
>
> ret = pci_enable_msix(pdev, xhci->msix_entries, xhci->msix_count);
> if (ret) {
> @@ -156,20 +214,18 @@ static int xhci_setup_msix(struct xhci_hcd *xhci)
> goto free_entries;
> }
I see from Documention/PCI/MSI-HOWTO.txt that pci_enable_msix() can
return less than xhci->msix_count, if the PCI core could only enable a
certain number of MSIs. Why not set xhci->msix_count to that returned
number and continue on with a few less MSIs than the driver requested?
>
> - /*
> - * Pass the xhci pointer value as the request_irq "cookie".
> - * If more irqs are added, this will need to be unique for each one.
> - */
> - ret = request_irq(xhci->msix_entries[0].vector, &xhci_irq, 0,
> - "xHCI", xhci_to_hcd(xhci));
> - if (ret) {
> - xhci_err(xhci, "Failed to allocate MSI-X interrupt\n");
> - goto disable_msix;
> + for (i = 0; i < xhci->msix_count; i++) {
> + ret = request_irq(xhci->msix_entries[i].vector,
> + (irq_handler_t)xhci_msi_irq,
> + 0, "xhci_hcd", xhci_to_hcd(xhci));
> + if (ret)
> + goto disable_msix;
> }
> - xhci_dbg(xhci, "Finished setting up MSI-X\n");
> - return 0;
> + return ret;
>
> disable_msix:
> + xhci_err(xhci, "disable MSI-X interrupt\n");
> + xhci_free_irq(xhci);
You have a bug if request_irq() fails for an MSI where. xhci_free_irq()
will call free_irq() for a vector that was never allocated. Set
xhci->msix_count to (i - 1) before jumping to disable_msix.
> pci_disable_msix(pdev);
> free_entries:
> kfree(xhci->msix_entries);
> @@ -177,21 +233,23 @@ free_entries:
> return ret;
> }
>
> -/* XXX: code duplication; can xhci_setup_msix call this? */
> /* Free any IRQs and disable MSI-X */
> static void xhci_cleanup_msix(struct xhci_hcd *xhci)
> {
> struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> - if (!xhci->msix_entries)
> - return;
>
> - free_irq(xhci->msix_entries[0].vector, xhci);
> - pci_disable_msix(pdev);
> - kfree(xhci->msix_entries);
> - xhci->msix_entries = NULL;
> - xhci_dbg(xhci, "Finished cleaning up MSI-X\n");
> + xhci_free_irq(xhci);
> +
> + if (xhci->msix_entries) {
> + pci_disable_msix(pdev);
> + kfree(xhci->msix_entries);
> + xhci->msix_entries = NULL;
> + } else {
> + pci_disable_msi(pdev);
> + }
> +
> + return;
> }
> -#endif
>
> /*
> * Initialize memory for HCD and xHC (one-time init).
> @@ -314,6 +372,16 @@ hw_died:
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)
> +{
> + irqreturn_t ret;
> +
> + clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
> +
> + ret = xhci_irq(hcd);
> + return ret;
> +}
> +
> #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> void xhci_event_ring_work(unsigned long arg)
> {
> @@ -385,21 +453,35 @@ int xhci_run(struct usb_hcd *hcd)
> {
> u32 temp;
> u64 temp_64;
> + u32 ret;
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
> void (*doorbell)(struct xhci_hcd *) = NULL;
>
> hcd->uses_new_polling = 1;
> hcd->poll_rh = 0;
>
> xhci_dbg(xhci, "xhci_run\n");
> -#if 0 /* FIXME: MSI not setup yet */
> +
> /* Do this at the very last minute */
> ret = xhci_setup_msix(xhci);
> - if (!ret)
> - return ret;
> + if (ret)
> + /* fall back to msi*/
> + ret = xhci_setup_msi(xhci);
> + if (ret) {
> + /* fall back to legacy IRQ */
> + ret = request_irq(pdev->irq,
> + (irq_handler_t)xhci_irq,
> + IRQF_SHARED,
> + "xhci_hcd",
> + xhci_to_hcd(xhci));
> + if (ret) {
> + xhci_err(xhci,
> + "Failed to request irq interrupt\n");
> + return ret;
> + }
> + }
>
> - return -ENOSYS;
> -#endif
> #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> init_timer(&xhci->event_ring_timer);
> xhci->event_ring_timer.data = (unsigned long) xhci;
> @@ -483,9 +565,8 @@ void xhci_stop(struct usb_hcd *hcd)
> xhci_reset(xhci);
> spin_unlock_irq(&xhci->lock);
>
> -#if 0 /* No MSI yet */
> xhci_cleanup_msix(xhci);
> -#endif
> +
> #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> /* Tell the event ring poll function not to reschedule */
> xhci->zombie = 1;
> @@ -521,9 +602,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
> xhci_halt(xhci);
> spin_unlock_irq(&xhci->lock);
>
> -#if 0
> xhci_cleanup_msix(xhci);
> -#endif
xhci_cleanup_msix() should be called with the spinlock held.
> xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
> xhci_readl(xhci, &xhci->op_regs->status));
> @@ -803,6 +882,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>
> xhci = hcd_to_xhci(hcd);
> spin_lock_irqsave(&xhci->lock, flags);
> +
Don't add unnecessary white space.
> /* Make sure the URB hasn't completed or been unlinked already */
> ret = usb_hcd_check_unlink_urb(hcd, urb, status);
> if (ret || !urb->hcpriv)
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index fb3440b..a32f644 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1121,7 +1121,7 @@ struct xhci_hcd {
> int page_size;
> /* Valid values are 12 to 20, inclusive */
> int page_shift;
> - /* only one MSI vector for now, but might need more later */
> + /* msi-x vectors */
> int msix_count;
> struct msix_entry *msix_entries;
> /* data structures */
> --
> 1.6.3.3
>
>
>
>
--
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
