Pl see inline -----Original Message----- From: Prarit Bhargava [mailto:prarit@xxxxxxxxxx] Sent: Friday, May 20, 2011 12:13 PM To: linux-scsi@xxxxxxxxxxxxxxx Cc: Kallickal, Jayamohan; mchristi@xxxxxxxxxx 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; ÿô.nlj·Ÿ®‰†+%ŠË±é¥Šwÿº{.nlj·¥Š{±þÇ‹ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf
