RE: [PATCH]: be2iscsi: Fix MSIX interrupt names

[<Jayamohan.Kallickal@Emulex.Com>]

[-- Attachment #1: Type: text/plain, Size: 3754 bytes --]

I have taken the original patch from Prarit and made changes to move the allocation 
inside the main for loop along with request_irq.

I feel doing devres would be good but going to take some time. However, we need to fix this now and hence
am submitting this patch. When we have a final devres solution working and tested ,we would move over.

Thanks
Jay
________________________________________
From: Prarit Bhargava [prarit@redhat.com]
Sent: Friday, May 20, 2011 11:51 AM
To: Kallickal, Jayamohan
Cc: linux-scsi@vger.kernel.org; mchristi@redhat.com
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names

On 05/20/2011 02:33 PM, Jayamohan.Kallickal@Emulex.Com wrote:
> Thanks for the patch. Pl see my comments in line
>
>

np.

> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 24e20ba..8d71e47 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -874,16 +874,20 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
>       struct hwi_controller *phwi_ctrlr;
>       struct hwi_context_memory *phwi_context;
>       int ret, msix_vec, i, j;
> -     char desc[32];
>
>       phwi_ctrlr = phba->phwi_ctrlr;
>       phwi_context = phwi_ctrlr->phwi_ctxt;
>
>       if (phba->msix_enabled) {
>               for (i = 0; i < phba->num_cpus; i++) {
> -                     sprintf(desc, "beiscsi_msix_%04x", i);
> +                     phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME,
> +                                                 GFP_KERNEL);
> +                     if (!phba->msi_name[i])
> +                             goto free_msix_irqs;
> We need to ensure i != 0 before jumping to free_msix_irqs
>

Will fix in next version ... I think I may have to do a bit more work
here because I just realized that if we free_irq() on a non-allocated
irq we'll get an angry message from the kernel ;)  Unfortunately this
may complicate the code.... I'll rework this and see if I can come up
with something better.

> +                     sprintf(phba->msi_name[i], "beiscsi_msix_%04x", i);
>                       msix_vec = phba->msix_entries[i].vector;
> -                     ret = request_irq(msix_vec, be_isr_msix, 0, desc,
> +                     ret = request_irq(msix_vec, be_isr_msix, 0,
> +                                       phba->msi_name[i],
>                                         &phwi_context->be_eq[i]);
>                       if (ret) {
>                               shost_printk(KERN_ERR, phba->shost,
> @@ -915,8 +919,10 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
>       }
>       return 0;
>  free_msix_irqs:
> -     for (j = i - 1; j == 0; j++)
> +     for (j = i - 1; j == 0; j++) {
>               free_irq(msix_vec, &phwi_context->be_eq[j]);
> +             kfree(phba->msi_name[i]);
> +     }
>

... I was looking at this, and I'm confused by it.

Should this really be 'j++'?  Or should it be j-- ?

It seems to make sense that this code is j-- ...


>       return ret;
>  }
>
> @@ -4117,10 +4123,13 @@ static void beiscsi_remove(struct pci_dev *pcidev)
>               for (i = 0; i <= phba->num_cpus; i++) {
>                       msix_vec = phba->msix_entries[i].vector;
>                       free_irq(msix_vec, &phwi_context->be_eq[i]);
> +                     kfree(phba->msi_name[i]);
>               }
>       } else
> -             if (phba->pcidev->irq)
> +             if (phba->pcidev->irq) {
>                       free_irq(phba->pcidev->irq, phba);
> +                     kfree(phba->msi_name[i]);
> Do we need the free for non-msix case as we are not allocation?
>

Fixed in next version.

<snip>

P.

[-- Attachment #2: 0001-be2iscsi-Fixing-the-proc-interrupts-problem.patch --]
[-- Type: application/octet-stream, Size: 3765 bytes --]

From da39ea3b666500896121de575c66ce9c236ff3a1 Mon Sep 17 00:00:00 2001
From: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
Date: Wed, 1 Jun 2011 10:53:31 -0700
Subject: [PATCH 1/1] be2iscsi: Fixing the /proc/interrupts problem

   This patch is based on one by Prarit Bhargava. I have made minor
changes.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Jayamohan Kallickal <jayamohan,kallickal@emulex.com>
---
 drivers/scsi/be2iscsi/be_main.c |   35 ++++++++++++++++++++++++++++-------
 drivers/scsi/be2iscsi/be_main.h |    3 +++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 94b9a07..8618925 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -875,33 +875,51 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
 	struct hwi_controller *phwi_ctrlr;
 	struct hwi_context_memory *phwi_context;
 	int ret, msix_vec, i, j;
-	char desc[32];
 
 	phwi_ctrlr = phba->phwi_ctrlr;
 	phwi_context = phwi_ctrlr->phwi_ctxt;
 
 	if (phba->msix_enabled) {
 		for (i = 0; i < phba->num_cpus; i++) {
-			sprintf(desc, "beiscsi_msix_%04x", i);
+			phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
+			if (!phba->msi_name[i]) {
+				ret = -ENOMEM;
+				if (!i)
+					return ret;
+				goto free_msix_irqs;
+			}
+
+			sprintf(phba->msi_name[i], "beiscsi_%02x_%02x",
+				phba->shost->host_no, i);
 			msix_vec = phba->msix_entries[i].vector;
-			ret = request_irq(msix_vec, be_isr_msix, 0, desc,
+			ret = request_irq(msix_vec, be_isr_msix, 0,
+					  phba->msi_name[i],
 					  &phwi_context->be_eq[i]);
 			if (ret) {
 				shost_printk(KERN_ERR, phba->shost,
 					     "beiscsi_init_irqs-Failed to"
 					     "register msix for i = %d\n", i);
-				if (!i)
+				kfree(phba->msi_name[i]);
+				if (!i) {
 					return ret;
+				}
 				goto free_msix_irqs;
 			}
 		}
 		msix_vec = phba->msix_entries[i].vector;
-		ret = request_irq(msix_vec, be_isr_mcc, 0, "beiscsi_msix_mcc",
+		phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
+		if (!phba->msi_name[i]) {
+			ret = -ENOMEM;
+			goto free_msix_irqs;
+		}
+		sprintf(phba->msi_name[i], "beiscsi_mcc_%02x",
+			phba->shost->host_no);
+		ret = request_irq(msix_vec, be_isr_mcc, 0, phba->msi_name[i],
 				  &phwi_context->be_eq[i]);
 		if (ret) {
 			shost_printk(KERN_ERR, phba->shost, "beiscsi_init_irqs-"
 				     "Failed to register beiscsi_msix_mcc\n");
-			i++;
+			kfree(phba->msi_name[i]);
 			goto free_msix_irqs;
 		}
 
@@ -916,8 +934,10 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
 	}
 	return 0;
 free_msix_irqs:
-	for (j = i - 1; j == 0; j++)
+	for (j = i - 1; j >= 0; j--) {
+		kfree(phba->msi_name[j]);
 		free_irq(msix_vec, &phwi_context->be_eq[j]);
+	}
 	return ret;
 }
 
@@ -4123,6 +4143,7 @@ static void beiscsi_remove(struct pci_dev *pcidev)
 		for (i = 0; i <= phba->num_cpus; i++) {
 			msix_vec = phba->msix_entries[i].vector;
 			free_irq(msix_vec, &phwi_context->be_eq[i]);
+			kfree(phba->msi_name[i]);
 		}
 	} else
 		if (phba->pcidev->irq)
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 081c171..1a55aee 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -162,6 +162,8 @@ do {							\
 #define PAGES_REQUIRED(x) \
 	((x < PAGE_SIZE) ? 1 :  ((x + PAGE_SIZE - 1) / PAGE_SIZE))
 
+#define BEISCSI_MSI_NAME 20 /* size of msi_name string */
+
 enum be_mem_enum {
 	HWI_MEM_ADDN_CONTEXT,
 	HWI_MEM_WRB,
@@ -287,6 +289,7 @@ struct beiscsi_hba {
 	unsigned int num_cpus;
 	unsigned int nxt_cqid;
 	struct msix_entry msix_entries[MAX_CPUS + 1];
+	char *msi_name[MAX_CPUS + 1];
 	bool msix_enabled;
 	struct be_mem_descriptor *init_mem;
 
-- 
1.7.1