Skip to contentSkip to conversation

arcmsr: Support MSI-X interrupt

PATCH In review ching 2 participants 3 messages Apr 30, 2014 – May 2, 2014 SCSI

lore
Share & tools
lore

ching

author
#0 lore raw

PATCH 2/16 arcmsr: Support MSI-X interrupt

From: Ching<[email protected]>

Adding code for supporting MSI-X interrupt

Signed-off-by Ching<[email protected]>

---

drivers/scsi/arcmsr/arcmsr.h +4 -0
diff -uprN a/drivers/scsi/arcmsr/arcmsr.h b/drivers/scsi/arcmsr/arcmsr.h
@@ -64,6 +64,7 @@ struct device_attribute;
 #define ARCMSR_MAX_HBB_POSTQUEUE						264
 #define ARCMSR_MAX_XFER_LEN							0x26000 /* 152K */
 #define ARCMSR_CDB_SG_PAGE_LENGTH						256 
+#define ARCMST_NUM_MSIX_VECTORS		4
 #ifndef PCI_DEVICE_ID_ARECA_1880
 #define PCI_DEVICE_ID_ARECA_1880 0x1880
  #endif

@@ -508,6 +509,7 @@ struct AdapterControlBlock
 	struct pci_dev *		pdev;
 	struct Scsi_Host *		host;
 	unsigned long			vir2phy_offset;
+	struct msix_entry	entries[ARCMST_NUM_MSIX_VECTORS];
 	/* Offset is used in making arc cdb physical to virtual calculations */
 	uint32_t			outbound_int_enable;
 	uint32_t			cdb_phyaddr_hi32;

@@ -544,6 +546,8 @@ struct AdapterControlBlock
 	/* iop init */
 	#define ACB_F_ABORT				0x0200
 	#define ACB_F_FIRMWARE_TRAP           		0x0400
+	#define ACB_F_MSI_ENABLED		0x1000
+	#define ACB_F_MSIX_ENABLED		0x2000
 	struct CommandControlBlock *			pccb_pool[ARCMSR_MAX_FREECCB_NUM];
 	/* used for memory free */
 	struct list_head		ccb_free_list;
drivers/scsi/arcmsr/arcmsr_hba.c +106 -12

177 lines not on this page · raw message

#1 lore raw
Quoted 6 lines · ching

From: Ching<[email protected]>

Adding code for supporting MSI-X interrupt

Signed-off-by Ching<[email protected]>

---

These patches seem to be broken up nicely into patches which do one thing per patch and they are much easier to review now. Thanks!

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 610

Quoted 5 lines
drivers/scsi/arcmsr/arcmsr_hba.c +5 -0
+static int arcmsr_request_irq(struct pci_dev *pdev, struct AdapterControlBlock *acb)
+{
+	int	i, j, r;
+	struct msix_entry entries[ARCMST_NUM_MSIX_VECTORS];
+

This function is more confusing than necessary and it goes over the 80 character limit. One hint for improving it is that you should always test for failure and handle the failure condition first before moving on. In other words reverse these tests.

Also let's rename LEG_INT to legacy_int: and MSI_INT to msi_int:.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 611

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+	if (pci_find_capability(pdev, PCI_CAP_ID_MSIX)) {

	if (!pci_find_capability(pdev, PCI_CAP_ID_MSIX))
		goto msi_int;

We can pull the following code in one indent level.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 614

Quoted 3 lines
drivers/scsi/arcmsr/arcmsr_hba.c +3 -0
+		for (i = 0; i < ARCMST_NUM_MSIX_VECTORS; i++) {
+			entries[i].entry = i;
+		}

Remove unneeded curly braces.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 615

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+		r = pci_enable_msix(pdev, entries, ARCMST_NUM_MSIX_VECTORS);

		if (r < 0)
			goto msi_int;
Quoted 5 lines
drivers/scsi/arcmsr/arcmsr_hba.c +5 -0
+		if (r == 0) {
+			for (i = 0; i < ARCMST_NUM_MSIX_VECTORS; i++) {
+				if (request_irq(entries[i].vector,
+					arcmsr_do_interrupt, 0, "arcmsr", acb)) {
+					pr_warn("arcmsr%d: request_irq =%d failed!\n",

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 630

Quoted 10 lines
drivers/scsi/arcmsr/arcmsr_hba.c +10 -0
+						acb->host->host_no, pdev->irq);
+					for (j = 0 ; j < i ; j++)
+						free_irq(entries[i].vector, acb);
+					pci_disable_msix(pdev);
+					goto MSI_INT;
+				}
+				acb->entries[i] = entries[i];
+			}
+			acb->acb_flags |= ACB_F_MSIX_ENABLED;
+			pr_warn("arcmsr%d: msi-x enabled\n", acb->host->host_no);

This should be pr_info(). pr_warn() will probably trigger a popup in gnome.

			return ARC_SUCCESS;

Returning right away is easier to understand than a return at the end of the function.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 631

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+		} else if (r > 0) {

This else statement is not needed because we already handled the other cases. Pull the code in one additional indent level.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 632

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+			if (!pci_enable_msix(pdev, entries, r)) {

			if (pci_enable_msix(pdev, entries, r))
				goto msi_int;

Pull the code in a third indent level.

Quoted 4 lines
drivers/scsi/arcmsr/arcmsr_hba.c +4 -0
+				for (i = 0; i < r; i++) {
+					if (request_irq(entries[i].vector,
+						arcmsr_do_interrupt, 0, "arcmsr", acb)) {
+						pr_warn("arcmsr%d: request_irq =%d failed!\n",

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 646

Quoted 10 lines
drivers/scsi/arcmsr/arcmsr_hba.c +10 -0
+							acb->host->host_no, pdev->irq);
+						for (j = 0 ; j < i ; j++)
+							free_irq(entries[i].vector, acb);
+						pci_disable_msix(pdev);
+						goto MSI_INT;
+					}
+					acb->entries[i] = entries[i];
+				}
+				acb->acb_flags |= ACB_F_MSIX_ENABLED;
+				pr_warn("arcmsr%d: msi-x enabled\n", acb->host->host_no);

Same thing. Change this to pr_info() and add a return ARC_SUCCESS;

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 650

Quoted 4 lines
drivers/scsi/arcmsr/arcmsr_hba.c +4 -0
+			} else {
+				goto MSI_INT;
+			}
+		} else {

Ok. At this point we have removed a lot of indenting so we are back to level 1 indenting.

msi_int:

	if (!pci_find_capability(pdev, PCI_CAP_ID_MSI))
		goto legacy_int;
	if (pci_enable_msi(pdev))
		goto legacy_int;
	if (request_irq(...)) {
		....
		goto legacy_int;
	}

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 663

Quoted 13 lines
drivers/scsi/arcmsr/arcmsr_hba.c +13 -0
+MSI_INT:
+			if (pci_find_capability(pdev, PCI_CAP_ID_MSI)) {
+				if (pci_enable_msi(pdev))
+					goto LEG_INT;
+				if (request_irq(pdev->irq, arcmsr_do_interrupt,
+					IRQF_SHARED, "arcmsr", acb)) {
+					pr_warn("arcmsr%d: request_irq =%d failed!\n",
+						acb->host->host_no, pdev->irq);
+					pci_disable_msi(pdev);
+					goto LEG_INT;
+				}
+				acb->acb_flags |= ACB_F_MSI_ENABLED;
+				pr_warn("arcmsr%d: msi enabled\n", acb->host->host_no);

return sucess.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 666

Quoted 4 lines
drivers/scsi/arcmsr/arcmsr_hba.c +4 -0
+			} else {
+				goto LEG_INT;
+			}
+		}

The following block is all duplicative code and we have already handled msi_int. Just delete it.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 680

Quoted 13 lines
drivers/scsi/arcmsr/arcmsr_hba.c +13 -0
+	} else if (pci_find_capability(pdev, PCI_CAP_ID_MSI)) {
+		if (pci_enable_msi(pdev))
+			goto LEG_INT;
+		if (request_irq(pdev->irq, arcmsr_do_interrupt,
+			IRQF_SHARED, "arcmsr", acb)) {
+			pr_warn("arcmsr%d: request_irq =%d failed!\n",
+				acb->host->host_no, pdev->irq);
+			pci_disable_msi(pdev);
+			goto LEG_INT;
+		}
+		acb->acb_flags |= ACB_F_MSI_ENABLED;
+		pr_warn("arcmsr%d: msi enabled\n", acb->host->host_no);
+	} else {

Ok. We're back to level one indenting now.

legacy_int:

	if (request_irq(...) {
		...
		return ARC_FAILURE;
	}
	return ARC_SUCCESS;

Flipping the conditions around and adding immediate returns makes the code a lot easier to read.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 691

Quoted 11 lines
drivers/scsi/arcmsr/arcmsr_hba.c +11 -0
+LEG_INT:
+		if (request_irq(pdev->irq, arcmsr_do_interrupt,
+			IRQF_SHARED, "arcmsr", acb)) {
+			pr_warn("arcmsr%d: request_irq = %d failed!\n",
+				acb->host->host_no, pdev->irq);
+			return ARC_FAILURE;
+		}
+	}
+	return ARC_SUCCESS;
+}
+

regards,
dan carpenter

author
#2 lore raw

Thanks for Dan's advice.
I will revise the patch code.
Next upload will be PATCH v1.1 16 patches.

Quoted 179 lines · Dan Carpenter
Quoted 6 lines · ching

From: Ching<[email protected]>

Adding code for supporting MSI-X interrupt

Signed-off-by Ching<[email protected]>

---

These patches seem to be broken up nicely into patches which do one thing per patch and they are much easier to review now. Thanks!

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 610

Quoted 5 lines
drivers/scsi/arcmsr/arcmsr_hba.c +5 -0
+static int arcmsr_request_irq(struct pci_dev *pdev, struct AdapterControlBlock *acb)
+{
+	int	i, j, r;
+	struct msix_entry entries[ARCMST_NUM_MSIX_VECTORS];
+

This function is more confusing than necessary and it goes over the 80 character limit. One hint for improving it is that you should always test for failure and handle the failure condition first before moving on. In other words reverse these tests.

Also let's rename LEG_INT to legacy_int: and MSI_INT to msi_int:.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 611

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+	if (pci_find_capability(pdev, PCI_CAP_ID_MSIX)) {

	if (!pci_find_capability(pdev, PCI_CAP_ID_MSIX))
		goto msi_int;

We can pull the following code in one indent level.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 614

Quoted 3 lines
drivers/scsi/arcmsr/arcmsr_hba.c +3 -0
+		for (i = 0; i < ARCMST_NUM_MSIX_VECTORS; i++) {
+			entries[i].entry = i;
+		}

Remove unneeded curly braces.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 615

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+		r = pci_enable_msix(pdev, entries, ARCMST_NUM_MSIX_VECTORS);

		if (r < 0)
			goto msi_int;
Quoted 5 lines
drivers/scsi/arcmsr/arcmsr_hba.c +5 -0
+		if (r == 0) {
+			for (i = 0; i < ARCMST_NUM_MSIX_VECTORS; i++) {
+				if (request_irq(entries[i].vector,
+					arcmsr_do_interrupt, 0, "arcmsr", acb)) {
+					pr_warn("arcmsr%d: request_irq =%d failed!\n",

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 630

Quoted 10 lines
drivers/scsi/arcmsr/arcmsr_hba.c +10 -0
+						acb->host->host_no, pdev->irq);
+					for (j = 0 ; j < i ; j++)
+						free_irq(entries[i].vector, acb);
+					pci_disable_msix(pdev);
+					goto MSI_INT;
+				}
+				acb->entries[i] = entries[i];
+			}
+			acb->acb_flags |= ACB_F_MSIX_ENABLED;
+			pr_warn("arcmsr%d: msi-x enabled\n", acb->host->host_no);

This should be pr_info(). pr_warn() will probably trigger a popup in gnome.

			return ARC_SUCCESS;

Returning right away is easier to understand than a return at the end of the function.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 631

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+		} else if (r > 0) {

This else statement is not needed because we already handled the other cases. Pull the code in one additional indent level.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 632

Quoted 1 line
drivers/scsi/arcmsr/arcmsr_hba.c +1 -0
+			if (!pci_enable_msix(pdev, entries, r)) {

			if (pci_enable_msix(pdev, entries, r))
				goto msi_int;

Pull the code in a third indent level.

Quoted 4 lines
drivers/scsi/arcmsr/arcmsr_hba.c +4 -0
+				for (i = 0; i < r; i++) {
+					if (request_irq(entries[i].vector,
+						arcmsr_do_interrupt, 0, "arcmsr", acb)) {
+						pr_warn("arcmsr%d: request_irq =%d failed!\n",

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 646

Quoted 10 lines
drivers/scsi/arcmsr/arcmsr_hba.c +10 -0
+							acb->host->host_no, pdev->irq);
+						for (j = 0 ; j < i ; j++)
+							free_irq(entries[i].vector, acb);
+						pci_disable_msix(pdev);
+						goto MSI_INT;
+					}
+					acb->entries[i] = entries[i];
+				}
+				acb->acb_flags |= ACB_F_MSIX_ENABLED;
+				pr_warn("arcmsr%d: msi-x enabled\n", acb->host->host_no);

Same thing. Change this to pr_info() and add a return ARC_SUCCESS;

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 650

Quoted 4 lines
drivers/scsi/arcmsr/arcmsr_hba.c +4 -0
+			} else {
+				goto MSI_INT;
+			}
+		} else {

Ok. At this point we have removed a lot of indenting so we are back to level 1 indenting.

msi_int:

	if (!pci_find_capability(pdev, PCI_CAP_ID_MSI))
		goto legacy_int;
	if (pci_enable_msi(pdev))
		goto legacy_int;
	if (request_irq(...)) {
		....
		goto legacy_int;
	}

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 663

Quoted 13 lines
drivers/scsi/arcmsr/arcmsr_hba.c +13 -0
+MSI_INT:
+			if (pci_find_capability(pdev, PCI_CAP_ID_MSI)) {
+				if (pci_enable_msi(pdev))
+					goto LEG_INT;
+				if (request_irq(pdev->irq, arcmsr_do_interrupt,
+					IRQF_SHARED, "arcmsr", acb)) {
+					pr_warn("arcmsr%d: request_irq =%d failed!\n",
+						acb->host->host_no, pdev->irq);
+					pci_disable_msi(pdev);
+					goto LEG_INT;
+				}
+				acb->acb_flags |= ACB_F_MSI_ENABLED;
+				pr_warn("arcmsr%d: msi enabled\n", acb->host->host_no);

return sucess.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 666

Quoted 4 lines
drivers/scsi/arcmsr/arcmsr_hba.c +4 -0
+			} else {
+				goto LEG_INT;
+			}
+		}

The following block is all duplicative code and we have already handled msi_int. Just delete it.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 680

Quoted 13 lines
drivers/scsi/arcmsr/arcmsr_hba.c +13 -0
+	} else if (pci_find_capability(pdev, PCI_CAP_ID_MSI)) {
+		if (pci_enable_msi(pdev))
+			goto LEG_INT;
+		if (request_irq(pdev->irq, arcmsr_do_interrupt,
+			IRQF_SHARED, "arcmsr", acb)) {
+			pr_warn("arcmsr%d: request_irq =%d failed!\n",
+				acb->host->host_no, pdev->irq);
+			pci_disable_msi(pdev);
+			goto LEG_INT;
+		}
+		acb->acb_flags |= ACB_F_MSI_ENABLED;
+		pr_warn("arcmsr%d: msi enabled\n", acb->host->host_no);
+	} else {

Ok. We're back to level one indenting now.

legacy_int:

	if (request_irq(...) {
		...
		return ARC_FAILURE;
	}
	return ARC_SUCCESS;

Flipping the conditions around and adding immediate returns makes the code a lot easier to read.

drivers/scsi/arcmsr/arcmsr_hba.c static void arcmsr_message_isr_bh_fn(str · line 691

Quoted 11 lines
drivers/scsi/arcmsr/arcmsr_hba.c +11 -0
+LEG_INT:
+		if (request_irq(pdev->irq, arcmsr_do_interrupt,
+			IRQF_SHARED, "arcmsr", acb)) {
+			pr_warn("arcmsr%d: request_irq = %d failed!\n",
+				acb->host->host_no, pdev->irq);
+			return ARC_FAILURE;
+		}
+	}
+	return ARC_SUCCESS;
+}
+

regards,
dan carpenter