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