[PATCH]: be2iscsi: Fix MSIX interrupt names

[PATCH]: be2iscsi: Fix MSIX interrupt names

[Prarit Bhargava]

The be2iscsi driver uses a single static array in a function for the
irq action->name field.

This results in /proc/interrupts output like

156:  0          0          0          0          0          0          0 
0          0          0          0          0          0          0          0 
        0          0          0          0          0          0          0    
     0          0       PCI-MSI-X 
W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������

                                                           
�8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��

In the line above, everything up to PCI-MSI-X is correct.  The pointer for
action->name is garbage and scribbles the output on the screen.

This patch fixes the problem:

156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017

P.

---->8----

Fix be2iscsi driver to use a separate pointer for each irq action->name field
and avoid display corruption in /proc/interrupts.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

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;
+			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]);
+	}
 	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]);
+		}
 	pci_disable_msix(phba->pcidev);
 	destroy_workqueue(phba->wq);
 	if (blk_iopoll_enabled)
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 90eb74f..0c97e5a 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -163,6 +163,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,
@@ -288,6 +290,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;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jayamohan.Kallickal]

Thanks for the patch. Pl see my comments in line

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


The be2iscsi driver uses a single static array in a function for the
irq action->name field.

This results in /proc/interrupts output like

156:  0          0          0          0          0          0          0 
0          0          0          0          0          0          0          0 
        0          0          0          0          0          0          0    
     0          0       PCI-MSI-X 
W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������

                                                           
�8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��

In the line above, everything up to PCI-MSI-X is correct.  The pointer for
action->name is garbage and scribbles the output on the screen.

This patch fixes the problem:

156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017

P.

---->8----

Fix be2iscsi driver to use a separate pointer for each irq action->name field
and avoid display corruption in /proc/interrupts.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

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

+			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]);
+	}
 	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?
+		}
 	pci_disable_msix(phba->pcidev);
 	destroy_workqueue(phba->wq);
 	if (blk_iopoll_enabled)
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 90eb74f..0c97e5a 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -163,6 +163,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,
@@ -288,6 +290,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;
 

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

[Prarit Bhargava]

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.

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

[Rolf Eike Beer]

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

Prarit Bhargava wrote:
> 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.

This could be simpler if you would use devres and devm_kzalloc() and 
devm_request_irq(). You simply need to return with error then and the driver 
core would free everything you already allocated.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Prarit Bhargava]

On 05/20/2011 04:17 PM, Rolf Eike Beer wrote:
> 
> This could be simpler if you would use devres and devm_kzalloc() and 
> devm_request_irq(). You simply need to return with error then and the driver 
> core would free everything you already allocated.
> 
> Eike

Thanks for the suggestion Eike.

I've never used devres before.  This seems to work -- please review as [v3].

Thanks,

P.

---8<----

The be2iscsi driver uses a single static array in a function for the
irq action->name field.

This results in /proc/interrupts output like

156:  0          0          0          0          0          0          0 
0          0          0          0          0          0          0          0 
        0          0          0          0          0          0          0    
	     0          0       PCI-MSI-X 
	     W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������

			                                                                
			     �8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��

In the line above, everything up to PCI-MSI-X is correct.  The pointer for
action->name is garbage and scribbles the output on the screen.

This patch fixes the problem and eliminates the need for memory cleanup by
using devres.

156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017


Signed-off-by: Prarit Bhargava <prarit@redhat.com>

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 94b9a07..16a83ee 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -874,40 +874,44 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
 	struct pci_dev *pcidev = phba->pcidev;
 	struct hwi_controller *phwi_ctrlr;
 	struct hwi_context_memory *phwi_context;
-	int ret, msix_vec, i, j;
-	char desc[32];
+	int ret, msix_vec, i;
 
 	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] = devm_kzalloc(&pcidev->dev, 20,
+							 GFP_KERNEL);
+			if (!phba->msi_name[i])
+				return -ENOMEM;
+			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,
-					  &phwi_context->be_eq[i]);
+			ret = devm_request_irq(&pcidev->dev,
+					       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)
-					return ret;
-				goto free_msix_irqs;
+				return ret;
 			}
 		}
 		msix_vec = phba->msix_entries[i].vector;
-		ret = request_irq(msix_vec, be_isr_mcc, 0, "beiscsi_msix_mcc",
-				  &phwi_context->be_eq[i]);
+		ret = devm_request_irq(&pcidev->dev,
+				       msix_vec, be_isr_mcc, 0,
+				       "beiscsi_msix_mcc",
+				       &phwi_context->be_eq[i]);
 		if (ret) {
 			shost_printk(KERN_ERR, phba->shost, "beiscsi_init_irqs-"
 				     "Failed to register beiscsi_msix_mcc\n");
-			i++;
-			goto free_msix_irqs;
+			return ret;
 		}
 
 	} else {
-		ret = request_irq(pcidev->irq, be_isr, IRQF_SHARED,
-				  "beiscsi", phba);
+		ret = devm_request_irq(&pcidev->dev, pcidev->irq, be_isr,
+				       IRQF_SHARED, "beiscsi", phba);
 		if (ret) {
 			shost_printk(KERN_ERR, phba->shost, "beiscsi_init_irqs-"
 				     "Failed to register irq\\n");
@@ -915,10 +919,6 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
 		}
 	}
 	return 0;
-free_msix_irqs:
-	for (j = i - 1; j == 0; j++)
-		free_irq(msix_vec, &phwi_context->be_eq[j]);
-	return ret;
 }
 
 static void hwi_ring_cq_db(struct beiscsi_hba *phba,
@@ -4122,11 +4122,12 @@ static void beiscsi_remove(struct pci_dev *pcidev)
 	if (phba->msix_enabled) {
 		for (i = 0; i <= phba->num_cpus; i++) {
 			msix_vec = phba->msix_entries[i].vector;
-			free_irq(msix_vec, &phwi_context->be_eq[i]);
+			devm_free_irq(&pcidev->dev, msix_vec,
+				      &phwi_context->be_eq[i]);
 		}
 	} else
 		if (phba->pcidev->irq)
-			free_irq(phba->pcidev->irq, phba);
+			devm_free_irq(&pcidev->dev, phba->pcidev->irq, phba);
 	pci_disable_msix(phba->pcidev);
 	destroy_workqueue(phba->wq);
 	if (blk_iopoll_enabled)
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 081c171..1c03174 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -287,6 +287,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;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rolf Eike Beer]

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

Am Dienstag, 24. Mai 2011, 10:48:09 schrieb Prarit Bhargava:
> On 05/20/2011 04:17 PM, Rolf Eike Beer wrote:
> > This could be simpler if you would use devres and devm_kzalloc() and
> > devm_request_irq(). You simply need to return with error then and the
> > driver core would free everything you already allocated.
> > 
> > Eike
> 
> Thanks for the suggestion Eike.
> 
> I've never used devres before.  This seems to work -- please review as [v3].

No, sorry, this wont work. You need to change your call of pci_enable_device() 
to pcim_enable_device(). Afterwards you should check what else in your probe 
routine can be converted to devres. This is optional, but why duplicate work?

What you need to take care of: resources that you do not allocate by devres 
(e.g. the scsi_host) and which are explicitely freed by you must not be needed 
e.g. in the IRQ handler if that would be freed by devres, i.e.:

int ret;
pcim_enable_device()
beiscsi_hba_alloc()
devm_request_irq()

ret = something();
if (ret != 0) {
 beiscsi_free_hba();
 return ret;
}
// devres would now free IRQs etc.

If an IRQ happens right before it is freed by devres (which could e.g. happen 
if you enable IRQ debugging) you could hit a NULL or stale host pointer. If 
your IRQ handler requires some resources to always be present (e.g. the 
scsi_host) then you must explicitely deregister it before releasing those 
resources. At the end this makes the init savings void. So you better add a 
single check

if (unlikely(my_hba == NULL))
  return IRQ_NONE;

to your IRQ handler to be safe.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Jayamohan.Kallickal]

[-- 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

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

[Rolf Eike Beer]

> 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.

I think this is a good way to go.

+			if (!phba->msi_name[i]) {
+				ret = -ENOMEM;
+				if (!i)
+					return ret;
+				goto free_msix_irqs;
+			}

That "if (!i)" stuff is useless. If you enter the loop with i==0 j will
become -1 which causes the loop never to execute. The same is true for the
request_irq() error handling below.

Eike

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

[Jayamohan.Kallickal]

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

Removed the redundant check for (!i) and reworked the patch.

Thanks
Jay
________________________________________
From: Rolf Eike Beer [eike-kernel@sf-tec.de]
Sent: Wednesday, June 01, 2011 12:41 PM
To: Kallickal, Jayamohan
Cc: prarit@redhat.com; linux-scsi@vger.kernel.org; mchristi@redhat.com
Subject: RE: [PATCH]: be2iscsi: Fix MSIX interrupt names

> 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.

I think this is a good way to go.

+                       if (!phba->msi_name[i]) {
+                               ret = -ENOMEM;
+                               if (!i)
+                                       return ret;
+                               goto free_msix_irqs;
+                       }

That "if (!i)" stuff is useless. If you enter the loop with i==0 j will
become -1 which causes the loop never to execute. The same is true for the
request_irq() error handling below.

Eike

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

From 38b3bc1cb3c7936f5a1c91835d5a70c7175ab051 Mon Sep 17 00:00:00 2001
From: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
Date: Wed, 1 Jun 2011 16:22:27 -0700
Subject: [PATCH 1/1] be2iscsi:Fixing the /proc/interrupts problem V2

   This patch is based on one by Prarit Bhargava. I have made minor
changes. Also, removed the redundant check for i == 0 as suggested
by Rolf.

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

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 94b9a07..0b3e522 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -875,33 +875,46 @@ 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;
+				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)
-					return ret;
+				kfree(phba->msi_name[i]);
 				goto free_msix_irqs;
 			}
 		}
+		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);
 		msix_vec = phba->msix_entries[i].vector;
-		ret = request_irq(msix_vec, be_isr_mcc, 0, "beiscsi_msix_mcc",
+		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 +929,11 @@ 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]);
+		msix_vec = phba->msix_entries[j].vector;
 		free_irq(msix_vec, &phwi_context->be_eq[j]);
+	}
 	return ret;
 }
 
@@ -4123,6 +4139,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

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

[Rolf Eike Beer]

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

Jayamohan.Kallickal@Emulex.Com wrote:
> Removed the redundant check for (!i) and reworked the patch.

Looks sane.

Reviewed-By: Rolf Eike Beer <eike-kernel@sf-tec.de>

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Prarit Bhargava]

[V2] Jayamohan ... I reworked the code so that the allocations of the msi_name
don't collide with the irq_requests.  If I do it in the same for loop things
get very messy very quickly.  This should work since the phba is zero'd out.

The be2iscsi driver uses a single static array in a function for the
irq action->name field.

This results in /proc/interrupts output like

156:  0          0          0          0          0          0          0 
0          0          0          0          0          0          0          0 
        0          0          0          0          0          0          0    
     0          0       PCI-MSI-X 
W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������

                                                           
�8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��

In the line above, everything up to PCI-MSI-X is correct.  The pointer for
action->name is garbage and scribbles the output on the screen.

This patch fixes the problem:

156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017


---->8----

Fix be2iscsi driver to use a separate pointer for each irq action->name field.
Also fix a calculation error in the free_msix_irqs for loop.

Successfully tested by me on an HP BL460C G7.

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 2e89f88..d44b59d 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -672,16 +672,23 @@ 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;
 
+	for (i = 0; i < phba->num_cpus; i++) {
+		phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
+		if (!phba->msi_name[i]) {
+			ret = -ENOMEM;
+			goto free_msix_names;
+		}
+	}
 	if (phba->msix_enabled) {
 		for (i = 0; i < phba->num_cpus; i++) {
-			sprintf(desc, "beiscsi_msix_%04x", i);
+			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,
@@ -713,8 +720,11 @@ 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]);
+free_msix_names:
+	for (i = 0; i < phba->num_cpus ; i++)
+		kfree(phba->msi_name[i]);
 	return ret;
 }
 
@@ -3832,6 +3842,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 d1ee00b..08cc714 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -163,6 +163,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,
@@ -288,6 +290,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;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jayamohan.Kallickal]

Pl see inline
-----Original Message-----
From: Prarit Bhargava [mailto:prarit@redhat.com] 
Sent: Friday, May 20, 2011 12:13 PM
To: linux-scsi@vger.kernel.org
Cc: Kallickal, Jayamohan; mchristi@redhat.com
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names


[V2] Jayamohan ... I reworked the code so that the allocations of the msi_name
don't collide with the irq_requests.  If I do it in the same for loop things
get very messy very quickly.  This should work since the phba is zero'd out.

The be2iscsi driver uses a single static array in a function for the
irq action->name field.

This results in /proc/interrupts output like

156:  0          0          0          0          0          0          0 
0          0          0          0          0          0          0          0 
        0          0          0          0          0          0          0    
     0          0       PCI-MSI-X 
W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������

                                                           
�8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��

In the line above, everything up to PCI-MSI-X is correct.  The pointer for
action->name is garbage and scribbles the output on the screen.

This patch fixes the problem:

156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017


---->8----

Fix be2iscsi driver to use a separate pointer for each irq action->name field.
Also fix a calculation error in the free_msix_irqs for loop.

Successfully tested by me on an HP BL460C G7.

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 2e89f88..d44b59d 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -672,16 +672,23 @@ 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;
 
+	for (i = 0; i < phba->num_cpus; i++) {
+		phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
+		if (!phba->msi_name[i]) {
+			ret = -ENOMEM;
+			goto free_msix_names;
+		}
+	}
 	if (phba->msix_enabled) {
 		for (i = 0; i < phba->num_cpus; i++) {
-			sprintf(desc, "beiscsi_msix_%04x", i);
+			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,
@@ -713,8 +720,11 @@ 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--)

Agreed. Thanks
 		free_irq(msix_vec, &phwi_context->be_eq[j]);
+free_msix_names:
+	for (i = 0; i < phba->num_cpus ; i++)


I think we need to replace the above line with something like
	for (j = i - 1; j == 0; j--)
as otherwise if alloc fails before reaching num_cpus - 1, we would free
 without allocating


+		kfree(phba->msi_name[i]);
 	return ret;
 }
 
@@ -3832,6 +3842,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 d1ee00b..08cc714 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -163,6 +163,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,
@@ -288,6 +290,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;
 

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

[Prarit Bhargava]

On 05/20/2011 06:07 PM, Jayamohan.Kallickal@Emulex.Com wrote:
> Pl see inline
> -----Original Message-----
> From: Prarit Bhargava [mailto:prarit@redhat.com] 
> Sent: Friday, May 20, 2011 12:13 PM
> To: linux-scsi@vger.kernel.org
> Cc: Kallickal, Jayamohan; mchristi@redhat.com
> Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names
>
>
> [V2] Jayamohan ... I reworked the code so that the allocations of the msi_name
> don't collide with the irq_requests.  If I do it in the same for loop things
> get very messy very quickly.  This should work since the phba is zero'd out.
>
> The be2iscsi driver uses a single static array in a function for the
> irq action->name field.
>
> This results in /proc/interrupts output like
>
> 156:  0          0          0          0          0          0          0 
> 0          0          0          0          0          0          0          0 
>         0          0          0          0          0          0          0    
>      0          0       PCI-MSI-X 
> W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������
>
>                                                            
> �8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��
>
> In the line above, everything up to PCI-MSI-X is correct.  The pointer for
> action->name is garbage and scribbles the output on the screen.
>
> This patch fixes the problem:
>
> 156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017
>
>
> ---->8----
>
> Fix be2iscsi driver to use a separate pointer for each irq action->name field.
> Also fix a calculation error in the free_msix_irqs for loop.
>
> Successfully tested by me on an HP BL460C G7.
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 2e89f88..d44b59d 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -672,16 +672,23 @@ 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;
>  
> +	for (i = 0; i < phba->num_cpus; i++) {
> +		phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
> +		if (!phba->msi_name[i]) {
> +			ret = -ENOMEM;
> +			goto free_msix_names;
> +		}
> +	}
>  	if (phba->msix_enabled) {
>  		for (i = 0; i < phba->num_cpus; i++) {
> -			sprintf(desc, "beiscsi_msix_%04x", i);
> +			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,
> @@ -713,8 +720,11 @@ 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--)
>
> Agreed. Thanks
>  		free_irq(msix_vec, &phwi_context->be_eq[j]);
> +free_msix_names:
> +	for (i = 0; i < phba->num_cpus ; i++)
>
>
> I think we need to replace the above line with something like
> 	for (j = i - 1; j == 0; j--)
> as otherwise if alloc fails before reaching num_cpus - 1, we would free
>  without allocating
>   

Oops ... cc'ing everyone.

Actually, no -- this is okay because phba is zero'd out.  So at worst we
end up kfree'ing a bunch of NULL pointers which is okay.

I'll take a look at Rolf's suggestions ...

P.

>
> +		kfree(phba->msi_name[i]);
>  	return ret;
>  }
>  
> @@ -3832,6 +3842,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 d1ee00b..08cc714 100644
> --- a/drivers/scsi/be2iscsi/be_main.h
> +++ b/drivers/scsi/be2iscsi/be_main.h
> @@ -163,6 +163,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,
> @@ -288,6 +290,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;
>  
>   
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jayamohan.Kallickal]

-----Original Message-----
From: Prarit Bhargava [mailto:prarit@redhat.com] 
Sent: Friday, May 20, 2011 4:46 PM
To: Kallickal, Jayamohan
Cc: linux-scsi@vger.kernel.org; mchristi@redhat.com
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names



On 05/20/2011 06:07 PM, Jayamohan.Kallickal@Emulex.Com wrote:
> Pl see inline
> -----Original Message-----
> From: Prarit Bhargava [mailto:prarit@redhat.com] 
> Sent: Friday, May 20, 2011 12:13 PM
> To: linux-scsi@vger.kernel.org
> Cc: Kallickal, Jayamohan; mchristi@redhat.com
> Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names
>
>
> [V2] Jayamohan ... I reworked the code so that the allocations of the msi_name
> don't collide with the irq_requests.  If I do it in the same for loop things
> get very messy very quickly.  This should work since the phba is zero'd out.
>
> The be2iscsi driver uses a single static array in a function for the
> irq action->name field.
>
> This results in /proc/interrupts output like
>
> 156:  0          0          0          0          0          0          0 
> 0          0          0          0          0          0          0          0 
>         0          0          0          0          0          0          0    
>      0          0       PCI-MSI-X 
> W�9�_޲J������t��kfbY��~}����(p���%���'loCm�������n`!�v��%�4����b\"P�����/"�t���(b��I��ԫ��1/"��Rm�u~�������
>
>                                                            
> �8r�)�N�>\aj*��+�е��ۻ�wB䝟�Bl��
>
> In the line above, everything up to PCI-MSI-X is correct.  The pointer for
> action->name is garbage and scribbles the output on the screen.
>
> This patch fixes the problem:
>
> 156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017
>
>
> ---->8----
>
> Fix be2iscsi driver to use a separate pointer for each irq action->name field.
> Also fix a calculation error in the free_msix_irqs for loop.
>
> Successfully tested by me on an HP BL460C G7.
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 2e89f88..d44b59d 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -672,16 +672,23 @@ 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;
>  
> +	for (i = 0; i < phba->num_cpus; i++) {
> +		phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
> +		if (!phba->msi_name[i]) {
> +			ret = -ENOMEM;
> +			goto free_msix_names;
> +		}
> +	}
>  	if (phba->msix_enabled) {
>  		for (i = 0; i < phba->num_cpus; i++) {
> -			sprintf(desc, "beiscsi_msix_%04x", i);
> +			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,
> @@ -713,8 +720,11 @@ 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--)
>
> Agreed. Thanks
>  		free_irq(msix_vec, &phwi_context->be_eq[j]);
> +free_msix_names:
> +	for (i = 0; i < phba->num_cpus ; i++)
>
>
> I think we need to replace the above line with something like
> 	for (j = i - 1; j == 0; j--)
> as otherwise if alloc fails before reaching num_cpus - 1, we would free
>  without allocating
>   

>Oops ... cc'ing everyone.

>Actually, no -- this is okay because phba is zero'd out.  So at worst we
>end up kfree'ing a bunch of NULL pointers which is okay.

Ok. You are technically right and I have seen code that relies on free skipping NULL.
Just an uneasy feeling of knowingly doing it. Just my thoughts...

>I'll take a look at Rolf's suggestions ...

>P.

>
> +		kfree(phba->msi_name[i]);
>  	return ret;
>  }
>  
> @@ -3832,6 +3842,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 d1ee00b..08cc714 100644
> --- a/drivers/scsi/be2iscsi/be_main.h
> +++ b/drivers/scsi/be2iscsi/be_main.h
> @@ -163,6 +163,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,
> @@ -288,6 +290,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;
>  
>