: be2iscsi: Fix MSIX interrupt names
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 : 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.
drivers/scsi/be2iscsi/be_main.c +14 -5
@@ -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)
drivers/scsi/be2iscsi/be_main.h +3 -0
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the patch. Pl see my comments in line
-----Original Message-----
From: Prarit Bhargava [mailto:[email protected]]
Sent: Friday, May 20, 2011 11:13 AM
To: [email protected]
Cc: Kallickal, Jayamohan; [email protected]
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.
drivers/scsi/be2iscsi/be_main.c +4 -2
@@ -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)
drivers/scsi/be2iscsi/be_main.h +3 -0
@@ -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;
Quoted 3 lines · On 05/20/2011 02:33 PM, [email protected] wrote:
Thanks for the patch. Pl see my comments in line
np.
Quoted 20 lines
drivers/scsi/be2iscsi/be_main.c +4
-2@@ -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;
Quoted 2 lines
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.
Quoted 9 lines
drivers/scsi/be2iscsi/be_main.c +3
-1+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,
drivers/scsi/be2iscsi/be_main.c static int beiscsi_init_irqs(struct beiscsi_hba *phba) · line 926
Quoted 9 lines
drivers/scsi/be2iscsi/be_main.c +3
-1@@ -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-- ...
Quoted 14 lines
drivers/scsi/be2iscsi/be_main.c +3
-1return 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]);
Quoted 2 lines
Do we need the free for non-msix case as we are not allocation?
Fixed in next version.
<snip>
P.
Quoted 25 lines · Prarit Bhargava
Quoted 1 line · On 05/20/2011 02:33 PM, [email protected] wrote:
Thanks for the patch. Pl see my comments in line
np.
Quoted 20 lines
(quoted diff) +0
-0
b/drivers/scsi/be2iscsi/be_main.cindex 24e20ba..8d71e47 100644drivers/scsi/be2iscsi/be_main.c +0
-1@@ -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++) {
Quoted 12 lines
Quoted 6 lines
drivers/scsi/be2iscsi/be_main.c +4
-1-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_irqsWill 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
Quoted 6 lines · Rolf Eike Beer
This could be simpler if you would use devres and
devm_kzalloc()anddevm_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
drivers/scsi/be2iscsi/be_main.c +21 -20
@@ -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)
drivers/scsi/be2iscsi/be_main.h +1 -0
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, 24. Mai 2011, 10:48:09 schrieb Prarit Bhargava:
Quoted 10 lines · Rolf Eike Beer
Quoted 5 lines
This could be simpler if you would use devres and
devm_kzalloc()anddevm_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
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 [[email protected]]
Sent: Friday, May 20, 2011 11:51 AM
To: Kallickal, Jayamohan
Cc: [email protected]; [email protected]
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names
Quoted 3 lines · On 05/20/2011 02:33 PM, [email protected] wrote:
Thanks for the patch. Pl see my comments in line
np.
Quoted 20 lines
drivers/scsi/be2iscsi/be_main.c +4
-2@@ -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;
Quoted 2 lines
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.
Quoted 9 lines
drivers/scsi/be2iscsi/be_main.c +3
-1+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,
drivers/scsi/be2iscsi/be_main.c static int beiscsi_init_irqs(struct beiscsi_hba *phba) · line 926
Quoted 9 lines
drivers/scsi/be2iscsi/be_main.c +3
-1@@ -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-- ...
Quoted 14 lines
drivers/scsi/be2iscsi/be_main.c +3
-1return 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]);
Quoted 2 lines
Do we need the free for non-msix case as we are not allocation?
Fixed in next version.
<snip>
P.
From da39ea3b666500896121de575c66ce9c236ff3a1 Mon Sep 17 00:00:00 2001
From: Jayamohan Kallickal <[email protected]>
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.
--- drivers/scsi/be2iscsi/be_main.c | 35 ++++++++++++++++++++++++++++------- drivers/scsi/be2iscsi/be_main.h | 3 +++ 2 files changed, 31 insertions(+), 7 deletions(-)
drivers/scsi/be2iscsi/be_main.c +28 -7
@@ -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)
drivers/scsi/be2iscsi/be_main.h +3 -0
@@ -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;
Signature · 1 line
1.7.1
Quoted 8 lines
I have taken the original patch from Prarit and made changes to move the allocation
inside the main for loop along withrequest_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
Removed the redundant check for (!i) and reworked the patch.
Thanks
Jay
________________________________________
From: Rolf Eike Beer [[email protected]]
Sent: Wednesday, June 01, 2011 12:41 PM
To: Kallickal, Jayamohan
Cc: [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH]: be2iscsi: Fix MSIX interrupt names
Quoted 8 lines
I have taken the original patch from Prarit and made changes to move the allocation
inside the main for loop along withrequest_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
From 38b3bc1cb3c7936f5a1c91835d5a70c7175ab051 Mon Sep 17 00:00:00 2001
From: Jayamohan Kallickal <[email protected]>
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.
--- drivers/scsi/be2iscsi/be_main.c | 33 +++++++++++++++++++++++++-------- drivers/scsi/be2iscsi/be_main.h | 3 +++ 2 files changed, 28 insertions(+), 8 deletions(-)
drivers/scsi/be2iscsi/be_main.c +25 -8
@@ -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)
drivers/scsi/be2iscsi/be_main.h +3 -0
@@ -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;
Signature · 1 line
1.7.1
Rolf Eike Beer
Reviewed-byQuoted 1 line · [email protected] wrote:
Removed the redundant check for (!i) and reworked the patch.
Looks sane.
[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.
drivers/scsi/be2iscsi/be_main.c +15 -4
@@ -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)
drivers/scsi/be2iscsi/be_main.h +3 -0
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Pl see inline
-----Original Message-----
From: Prarit Bhargava [mailto:[email protected]]
Sent: Friday, May 20, 2011 12:13 PM
To: [email protected]
Cc: Kallickal, Jayamohan; [email protected]
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.
drivers/scsi/be2iscsi/be_main.c +11 -4
@@ -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)
drivers/scsi/be2iscsi/be_main.h +3 -0
@@ -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;
Quoted 86 lines · On 05/20/2011 06:07 PM, [email protected] wrote:
Pl see inline
-----Original Message-----
From: Prarit Bhargava [mailto:[email protected]]
Sent: Friday, May 20, 2011 12:13 PM
To: [email protected]
Cc: Kallickal, Jayamohan; [email protected]
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names[V2] Jayamohan ... I reworked the code so that the allocations of the
msi_namedon't collide with theirq_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->namefield.This results in
/proc/interruptsoutput like156: 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->nameis 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->namefield. Also fix a calculation error in thefree_msix_irqsfor loop.Successfully tested by me on an HP BL460C G7.
drivers/scsi/be2iscsi/be_main.c +11
-4@@ -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
drivers/scsi/be2iscsi/be_main.c +2
-0free_irq (msix_vec, &phwi_context->be_eq[j]); +free_msix_names: +for (i =0 ; i < phba->num_cpus ; i++)
Quoted 5 lines
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.
Quoted 28 lines
drivers/scsi/be2iscsi/be_main.c +2
-0+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)drivers/scsi/be2iscsi/be_main.h +2
-0@@ -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 ,
drivers/scsi/be2iscsi/be_main.h struct beiscsi_hba { · line 296
Quoted 7 lines
drivers/scsi/be2iscsi/be_main.h +1
-0@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
-----Original Message-----
From: Prarit Bhargava [mailto:[email protected]]
Sent: Friday, May 20, 2011 4:46 PM
To: Kallickal, Jayamohan
Cc: [email protected]; [email protected]
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names
Quoted 88 lines · On 05/20/2011 06:07 PM, [email protected] wrote:
Pl see inline
-----Original Message-----
From: Prarit Bhargava [mailto:[email protected]]
Sent: Friday, May 20, 2011 12:13 PM
To: [email protected]
Cc: Kallickal, Jayamohan; [email protected]
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names[V2] Jayamohan ... I reworked the code so that the allocations of the
msi_namedon't collide with theirq_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->namefield.This results in
/proc/interruptsoutput like156: 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->nameis 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->namefield. Also fix a calculation error in thefree_msix_irqsfor loop.Successfully tested by me on an HP BL460C G7.
drivers/scsi/be2iscsi/be_main.c +11
-4@@ -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
drivers/scsi/be2iscsi/be_main.c +2
-0free_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--)
Quoted 8 lines
as otherwise if alloc fails before reaching
num_cpus- 1, we would free
without allocatingOops ... 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...
Quoted 39 lines
I'll take a look at Rolf's suggestions ...
P.
drivers/scsi/be2iscsi/be_main.c +2
-0+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)drivers/scsi/be2iscsi/be_main.h +3
-0@@ -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;