Re: [PATCH 2/3] usb: enable pci MSI/MSIX in usb core

[Alex,Shi]

On Fri, 2012-02-24 at 10:59 -0500, Alan Stern wrote:
> On Fri, 24 Feb 2012, Felipe Balbi wrote:
> 
> > > > > > +			retval = request_irq(hcd->msix_entries[i].vector,
> > > > > > +					(irq_handler_t)hcd->driver->msix_irq,
> > > > 
> > > > do you really need this cast here ?
> > > 
> > > Yes, otherwise the complain like here:
> > > drivers/usb/core/hcd-pci.c:330: warning: passing argument 2 of ârequest_irqâ from incompatible pointer type
> > > include/linux/interrupt.h:134: note: expected âirq_handler_tâ but argument is of type âenum irqreturn_t (* const)(int,  struct usb_hcd *)â
> > 
> > Alan, Sarah, is the definition of the IRQ handler wrong on the hc_driver
> > structure ?
> 
> No.  It is never passed to request_irq().
> 
> > Alex, I think you should fix your definition for the msix_irq handler.
> 
> The second parameter in the prototype is supposed to be void *, not 
> struct usb_hcd *.

Yes, Thanks you all for reviewing! 

Is the following change OK?
=======
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 3606afe..a198f4d 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -265,7 +265,8 @@ free_entries:
 	return ret;
 }
 
-/* Check for buggy HCD devices, and driver's expectation for MSI.
+/*
+ * Check for buggy HCD devices, and driver's expectation for MSI.
  * Now, only xhci driver want it by HCD_MSI_FIRST setting. Other driver
  * like ehci/uhci can follow this setting, if they want.
  */
@@ -326,8 +327,8 @@ int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 		/* register hc_driver's msix_irq handler */
 		for (i = 0; i < hcd->msix_count; i++) {
 			retval = request_irq(hcd->msix_entries[i].vector,
-					(irq_handler_t)hcd->driver->msix_irq,
-					0, hcd->driver->description, hcd);
+					hcd->driver->msix_irq, 0,
+					hcd->driver->description, hcd);
 			if (retval) {
 				hcd_free_msix(hcd);
 				break;
@@ -337,8 +338,7 @@ int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 			hcd->msix_enabled = 1;
 	} else if (hcd->irq == -1 && hcd->driver->msi_irq) {
 		/* register hc_driver's msi_irq handler */
-		retval = request_irq(irqnum,
-					(irq_handler_t)hcd->driver->msi_irq,
+		retval = request_irq(irqnum, hcd->driver->msi_irq,
 					0, hcd->driver->description, hcd);
 		if (retval)
 			hcd_free_msi(hcd);
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 579cbd3..9bc6568 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2365,12 +2365,9 @@ static int usb_hcd_request_default_irqs(struct usb_hcd *hcd,
 int usb_hcd_request_irqs(struct usb_hcd *hcd, unsigned int irqnum,
 		unsigned long irqflags)
 {
-	int retval = 1;
+	int retval = 0;
 
-#ifdef CONFIG_PCI
-	retval = usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags);
-#endif
-	if (retval)
+	if (usb_hcd_request_msi_msix_irqs(hcd, irqnum, irqflags))
 		retval = usb_hcd_request_default_irqs(hcd, irqnum, irqflags);
 
 	return retval;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b62037b..c223158 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2396,9 +2396,10 @@ hw_died:
 	return IRQ_HANDLED;
 }
 
-irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)
+irqreturn_t xhci_msi_irq(int irq, void *hcd)
 {
-	return xhci_irq(hcd);
+	struct usb_hcd *xhci_hcd = hcd;
+	return xhci_irq(xhci_hcd);
 }
 
 /****		Endpoint Ring Operations	****/
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8d511dd..6186d12 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1668,7 +1668,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
 
 int xhci_get_frame(struct usb_hcd *hcd);
 irqreturn_t xhci_irq(struct usb_hcd *hcd);
-irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd);
+irqreturn_t xhci_msi_irq(int irq, void *hcd);
 int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev);
 void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev);
 int xhci_alloc_tt_info(struct xhci_hcd *xhci,
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 5253c02..2f94c02 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -212,8 +212,8 @@ struct hc_driver {
 
 	/* irq handler */
 	irqreturn_t	(*irq) (struct usb_hcd *hcd);
-	irqreturn_t	(*msi_irq) (int irq, struct usb_hcd *hcd);
-	irqreturn_t	(*msix_irq) (int irq, struct usb_hcd *hcd);
+	irqreturn_t	(*msi_irq) (int irq, void *hcd);
+	irqreturn_t	(*msix_irq) (int irq, void *hcd);
 
 	int	flags;
 #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
@@ -413,6 +413,13 @@ extern int usb_hcd_register_msi_msix_irqs(struct usb_hcd *hcd);
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops usb_hcd_pci_pm_ops;
 #endif
+
+#else
+static inline int usb_hcd_request_msi_msix_irqs(struct usb_hcd *hcd,
+		unsigned int irqnum, unsigned long irqflags)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_PCI */
 
 /* pci-ish (pdev null is ok) buffer alloc/mapping support */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/