See my comments below. I haven't tested this yet; I'm just being
nit-picky about the coding style because the way it's written makes it
hard for me to reason about error paths.
Sarah Sharp
On Fri, Apr 30, 2010 at 02:24:52AM +0800, Andiry Xu wrote:
> >From 4643a08179b0c2aac8f3f4cdb09a6fedf5ddbf72 Mon Sep 17 00:00:00 2001
> From: Dong Nguyen <Dong.Nguyen@xxxxxxx>
> Date: Thu, 22 Apr 2010 16:58:23 -0700
> Subject: [PATCH] xHCI: supporting MSI/MSI-X
>
> Enable MSI/MSI-X supporting in xHCI driver.
>
> xHCI driver will try to use MSI-X as default.
> If MSI-X register fails, it will roll back using MSI and Legacy IRQs.
>
> Signed-off-by: Dong Nguyen <Dong.Nguyen@xxxxxxx>
> Signed-off-by: Andiry Xu <andiry.xu@xxxxxxx>
> ---
> drivers/usb/host/xhci-pci.c | 5 +
> drivers/usb/host/xhci.c | 214 +++++++++++++++++++++++++++++++++++-------
> drivers/usb/host/xhci.h | 7 ++
> 3 files changed, 190 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 417d37a..a351b64 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -113,7 +113,12 @@ static const struct hc_driver xhci_pci_hc_driver = {
> /*
> * generic hardware linkage
> */
> + /* do not register irq for xhci HCD
> + * xhci use MSIx in xhci-hcd
> + */
> +#if 0
> .irq = xhci_irq,
> +#endif
Please don't #if 0 out these lines, just remove them. That's what we
have git history for. If you want, you can leave the comment.
> .flags = HCD_MEMORY | HCD_USB3,
>
> /*
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 492a61c..cbf4e3e 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/module.h>
> #include <linux/moduleparam.h>
> @@ -31,6 +32,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);
> module_param(link_quirk, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(link_quirk, "Don't clear the chain bit on a link TRB");
>
> @@ -131,22 +133,137 @@ int xhci_reset(struct xhci_hcd *xhci)
> return handshake(xhci, &xhci->op_regs->command, CMD_RESET, 0, 250 * 1000);
> }
>
> +/*
> + * request IRQs from MSI-X.
> + *
> + * This requests for msix_count IRQs and set the xhci_flags properly.
> + */
> +static int xhci_request_irq(struct xhci_hcd *xhci)
> +{
> + int i, ret = 0;
> + irq_handler_t fn;
> + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> +
> + if (xhci->xhci_flags & XHCI_FLAG_SUPPORT_MSIX) {
> + for (i = 0; i < xhci->msix_count; i++) {
> + fn = (irq_handler_t)xhci_msi_irq;
> + ret = request_irq(xhci->msix_entries[i].vector,
> + fn, 0, "xhci_hcd", xhci_to_hcd(xhci));
> + if (ret)
> + goto disable_msix;
> + }
> + xhci->xhci_flags |= XHCI_FLAG_USING_MSIX;
> + } else if (xhci->xhci_flags & XHCI_FLAG_SUPPORT_MSI) {
> + ret = request_irq(pdev->irq, (irq_handler_t)xhci_msi_irq,
> + 0, "xhci", xhci_to_hcd(xhci));
Why do you need fn here? Just cast the function pointer in that first
call like you do in the line above.
> + if (ret)
> + goto disable_msi;
> +
> + xhci->xhci_flags |= XHCI_FLAG_USING_MSI;
> + } else {
> + xhci_err(xhci, "Unknown xhci_flags for MSI-X interrupt\n");
> + }
> + return 0;
> +
> +disable_msi:
> + xhci_err(xhci, "disable MSI interrupt\n");
> + pci_disable_msi(pdev);
> + xhci->xhci_flags &= ~XHCI_FLAG_SUPPORT_MSI;
> + return ret;
> +
> +disable_msix:
> + xhci_err(xhci, "disable MSI-X interrupt\n");
> + pci_disable_msix(pdev);
I have to check, but does pci_disable_msix() free all the vectors
associated with that device, or do you have to cancel each one?
> + kfree(xhci->msix_entries);
> + xhci->msix_entries = NULL;
> + xhci->xhci_flags &= ~XHCI_FLAG_SUPPORT_MSIX;
> + return ret;
> +}
Please don't use goto statements unless there's shared error handling
code like this:
xhci_free_command(xhci, stream_info->free_streams_command);
cleanup_ctx:
kfree(stream_info->stream_rings);
cleanup_info:
kfree(stream_info);
cleanup_trbs:
xhci->cmd_ring_reserved_trbs--;
return NULL;
The goto statements just make this function more confusing.
The code to clean up the irq requests if it fails should be placed back
into xhci_setup_msi() and xhci_setup_msix(), since xhci_request_irq()
doesn't allocate xhci->msix_entries. Variables should be freed in the
function that allocated them and actions should be undone in the same
function if error conditions happen. It's just easier to keep track of
whether variables got freed properly.
I think xhci_request_irq() should be two functions, one to setup msi and
one to set up MSI-X. There's basically no shared code, if you call it
with XHCI_FLAG_SUPPORT_MSI set vs. XHCI_FLAG_USING_MSI. "Functions
should be short and sweet, and do just one thing."
(Documentation/CodingStyle)
Also, I think you should try to remove as many xhci->xhci_flags as
possible to make this code simpler. The flags are just confusing and
add extra state to the xhci_hcd that I don't think you need.
Splitting up this function into two functions will help remove some uses
of XHCI_FLAG_SUPPORT_MSIX and XHCI_FLAG_SUPPORT_MSI. I think you can
get rid of the other usages, see my comments below.
> +
> +/*
> + * 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->xhci_flags & XHCI_FLAG_USING_MSIX) {
> + if (!xhci->msix_entries)
> + return;
> +
> + for (i = 0; i < xhci->msix_count; i++)
> + free_irq(xhci->msix_entries[i].vector,
> + xhci_to_hcd(xhci));
> +
> + xhci->xhci_flags &= ~XHCI_FLAG_USING_MSIX;
> + return;
> + }
> +
> + if (xhci->xhci_flags & XHCI_FLAG_USING_MSI) {
> + free_irq(pdev->irq, xhci_to_hcd(xhci));
> + xhci->xhci_flags &= ~XHCI_FLAG_USING_MSI;
> + return;
> + }
> +
> + if (xhci->xhci_flags & XHCI_FLAG_USING_LEGACY_IRQ) {
> + free_irq(pdev->irq, xhci_to_hcd(xhci));
> + xhci->xhci_flags &= ~XHCI_FLAG_USING_LEGACY_IRQ;
> + return;
> + }
The freeing code for these two cases are basically the same, so why do
you have two different flags?
Also, where do you set XHCI_FLAG_USING_LEGACY_IRQ? I only see it
checked in this patch, never set, which means that second if statement
will never run.
> +
> + 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;
> + }
> +
> + xhci->xhci_flags |= XHCI_FLAG_SUPPORT_MSI;
> + ret = xhci_request_irq(xhci);
> +
> + 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;
> + xhci->msix_entries = NULL;
> + max_vecs = HCS_MAX_INTRS(xhci->hcs_params1);
> + msix_vecs = min(cpus + 1, max_vecs);
> + xhci->msix_count = msix_vecs;
> +
> /* XXX: did I do this right? ixgbe does kcalloc for more than one */
You can remove my comment here.
> - xhci->msix_entries = kmalloc(sizeof(struct msix_entry), GFP_KERNEL);
> + 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) {
> @@ -154,21 +271,10 @@ static int xhci_setup_msix(struct xhci_hcd *xhci)
> goto free_entries;
> }
>
> - /*
> - * 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;
> - }
> - xhci_dbg(xhci, "Finished setting up MSI-X\n");
> - return 0;
> + xhci->xhci_flags |= XHCI_FLAG_SUPPORT_MSIX;
> + ret = xhci_request_irq(xhci);
> + return ret;
>
> -disable_msix:
> - pci_disable_msix(pdev);
> free_entries:
> kfree(xhci->msix_entries);
> xhci->msix_entries = NULL;
> @@ -180,16 +286,28 @@ free_entries:
> 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)
> +
> + xhci_free_irq(xhci);
> +
> + if (xhci->xhci_flags & XHCI_FLAG_SUPPORT_MSIX) {
> + if (!xhci->msix_entries)
> + return;
> +
> + pci_disable_msix(pdev);
> + kfree(xhci->msix_entries);
> + xhci->msix_entries = NULL;
> + xhci->xhci_flags &= ~XHCI_FLAG_SUPPORT_MSIX;
> return;
> + }
The first if statement should just read
if (xhci->msix_entries)
pci_disable_msix(pdev);
...
return;
Then you can stop using XHCI_FLAG_SUPPORT_MSIX once you split up
xhci_request_irq().
> - 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");
> + if (xhci->xhci_flags & XHCI_FLAG_SUPPORT_MSI) {
> + pci_disable_msi(pdev);
> + xhci->xhci_flags &= ~XHCI_FLAG_SUPPORT_MSI;
> + return;
> + }
You can call pci_disable_msi() unconditionally here, because it will
immediately return if it never enabled MSI for the device. So you don't
need XHCI_FLAG_SUPPORT_MSI either.
> +
> + return;
> }
> -#endif
>
> /*
> * Initialize memory for HCD and xHC (one-time init).
> @@ -312,6 +430,14 @@ hw_died:
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)
> +{
> + irqreturn_t ret;
> +
> + ret = xhci_irq(hcd);
> + return ret;
> +}
> +
> #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> void xhci_event_ring_work(unsigned long arg)
> {
> @@ -387,21 +513,36 @@ 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) {
> + /* use msi*/
> + ret = xhci_setup_msi(xhci);
> + if (ret) {
> + /* use legacy IRQ */
> + ret = request_irq(pdev->irq,
> + (irq_handler_t)xhci_irq,
> + IRQF_SHARED,
> + "xhci",
> + xhci_to_hcd(xhci));
> + if (ret) {
> + xhci_err(xhci,
> + "Failed to request irq interrupt\n");
> + return ret;
> + }
> + }
> + }
You can un-nest most of this code, like so:
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",
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;
> @@ -485,9 +626,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;
> @@ -523,9 +663,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
> xhci_halt(xhci);
> spin_unlock_irq(&xhci->lock);
>
> -#if 0
> xhci_cleanup_msix(xhci);
> -#endif
>
> xhci_dbg(xhci, "xhci_shutdown completed - status = %x\n",
> xhci_readl(xhci, &xhci->op_regs->status));
> @@ -792,6 +930,10 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>
> xhci = hcd_to_xhci(hcd);
> spin_lock_irqsave(&xhci->lock, flags);
> +
> + if (xhci->xhci_flags & (XHCI_FLAG_USING_MSI | XHCI_FLAG_USING_MSIX))
> + clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
> +
I'm confused as to what you're doing here. It looks like you're
clearing the interrupt when the URB is being canceled by the class
driver. Why? We haven't entered this function because the hardware
interrupted us.
Shouldn't this code be in xhci_msi_irq() instead? If so, you don't need
to check the flags, because xhci_msi_irq() should only be called if
you've successfully set up MSI or MSI-X. That would get rid of what I
think is your last usage of XHCI_FLAG_USING_MSI and
XHCI_FLAG_USING_MSIX.
> /* 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 e5eb09b..cc27b17 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1112,6 +1112,13 @@ struct xhci_hcd {
> unsigned int quirks;
> #define XHCI_LINK_TRB_QUIRK (1 << 0)
> #define XHCI_RESET_EP_QUIRK (1 << 1)
> +
> + u32 xhci_flags;
> +#define XHCI_FLAG_USING_LEGACY_IRQ 0x00000001
> +#define XHCI_FLAG_SUPPORT_MSI 0x00000002
> +#define XHCI_FLAG_SUPPORT_MSIX 0x00000004
> +#define XHCI_FLAG_USING_MSI 0x00000008
> +#define XHCI_FLAG_USING_MSIX 0x00000010
> };
>
> /* For testing purposes */
> --
> 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
--
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
