* [PATCH]: be2iscsi: Fix MSIX interrupt names [not found] <4DD6AEB7.2090900@redhat.com> @ 2011-05-20 18:12 ` Prarit Bhargava 2011-05-20 18:33 ` Jayamohan.Kallickal 0 siblings, 1 reply; 14+ messages in thread From: Prarit Bhargava @ 2011-05-20 18:12 UTC (permalink / raw) To: linux-scsi; +Cc: jayamohank, mchristi 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 18:12 ` [PATCH]: be2iscsi: Fix MSIX interrupt names Prarit Bhargava @ 2011-05-20 18:33 ` Jayamohan.Kallickal 2011-05-20 18:51 ` Prarit Bhargava 2011-05-20 19:13 ` Prarit Bhargava 0 siblings, 2 replies; 14+ messages in thread From: Jayamohan.Kallickal @ 2011-05-20 18:33 UTC (permalink / raw) To: prarit, linux-scsi; +Cc: mchristi 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 18:33 ` Jayamohan.Kallickal @ 2011-05-20 18:51 ` Prarit Bhargava 2011-05-20 20:17 ` Rolf Eike Beer 2011-06-01 18:55 ` Jayamohan.Kallickal 2011-05-20 19:13 ` Prarit Bhargava 1 sibling, 2 replies; 14+ messages in thread From: Prarit Bhargava @ 2011-05-20 18:51 UTC (permalink / raw) To: Jayamohan.Kallickal; +Cc: linux-scsi, mchristi 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 18:51 ` Prarit Bhargava @ 2011-05-20 20:17 ` Rolf Eike Beer 2011-05-24 14:48 ` Prarit Bhargava 2011-06-01 18:55 ` Jayamohan.Kallickal 1 sibling, 1 reply; 14+ messages in thread From: Rolf Eike Beer @ 2011-05-20 20:17 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Jayamohan.Kallickal, linux-scsi, mchristi [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 20:17 ` Rolf Eike Beer @ 2011-05-24 14:48 ` Prarit Bhargava 2011-05-24 15:09 ` Rolf Eike Beer 0 siblings, 1 reply; 14+ messages in thread From: Prarit Bhargava @ 2011-05-24 14:48 UTC (permalink / raw) To: linux-scsi; +Cc: Rolf Eike Beer, Jayamohan.Kallickal, mchristi 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-24 14:48 ` Prarit Bhargava @ 2011-05-24 15:09 ` Rolf Eike Beer 0 siblings, 0 replies; 14+ messages in thread From: Rolf Eike Beer @ 2011-05-24 15:09 UTC (permalink / raw) To: Prarit Bhargava; +Cc: linux-scsi, Jayamohan.Kallickal, mchristi [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 18:51 ` Prarit Bhargava 2011-05-20 20:17 ` Rolf Eike Beer @ 2011-06-01 18:55 ` Jayamohan.Kallickal 2011-06-01 19:41 ` Rolf Eike Beer 1 sibling, 1 reply; 14+ messages in thread From: Jayamohan.Kallickal @ 2011-06-01 18:55 UTC (permalink / raw) To: prarit; +Cc: linux-scsi, mchristi [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-06-01 18:55 ` Jayamohan.Kallickal @ 2011-06-01 19:41 ` Rolf Eike Beer 2011-06-01 23:50 ` Jayamohan.Kallickal 0 siblings, 1 reply; 14+ messages in thread From: Rolf Eike Beer @ 2011-06-01 19:41 UTC (permalink / raw) To: Jayamohan.Kallickal; +Cc: prarit, linux-scsi, mchristi > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-06-01 19:41 ` Rolf Eike Beer @ 2011-06-01 23:50 ` Jayamohan.Kallickal 2011-06-02 9:35 ` Rolf Eike Beer 0 siblings, 1 reply; 14+ messages in thread From: Jayamohan.Kallickal @ 2011-06-01 23:50 UTC (permalink / raw) To: eike-kernel; +Cc: prarit, linux-scsi, mchristi [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-06-01 23:50 ` Jayamohan.Kallickal @ 2011-06-02 9:35 ` Rolf Eike Beer 0 siblings, 0 replies; 14+ messages in thread From: Rolf Eike Beer @ 2011-06-02 9:35 UTC (permalink / raw) To: Jayamohan.Kallickal; +Cc: prarit, linux-scsi, mchristi [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 18:33 ` Jayamohan.Kallickal 2011-05-20 18:51 ` Prarit Bhargava @ 2011-05-20 19:13 ` Prarit Bhargava 2011-05-20 22:07 ` Jayamohan.Kallickal 1 sibling, 1 reply; 14+ messages in thread From: Prarit Bhargava @ 2011-05-20 19:13 UTC (permalink / raw) To: linux-scsi; +Cc: Jayamohan.Kallickal, mchristi [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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 19:13 ` Prarit Bhargava @ 2011-05-20 22:07 ` Jayamohan.Kallickal 2011-05-20 23:45 ` Prarit Bhargava 0 siblings, 1 reply; 14+ messages in thread From: Jayamohan.Kallickal @ 2011-05-20 22:07 UTC (permalink / raw) To: prarit, linux-scsi; +Cc: mchristi 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 22:07 ` Jayamohan.Kallickal @ 2011-05-20 23:45 ` Prarit Bhargava 2011-05-23 17:22 ` Jayamohan.Kallickal 0 siblings, 1 reply; 14+ messages in thread From: Prarit Bhargava @ 2011-05-20 23:45 UTC (permalink / raw) To: Jayamohan.Kallickal; +Cc: linux-scsi, mchristi 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH]: be2iscsi: Fix MSIX interrupt names 2011-05-20 23:45 ` Prarit Bhargava @ 2011-05-23 17:22 ` Jayamohan.Kallickal 0 siblings, 0 replies; 14+ messages in thread From: Jayamohan.Kallickal @ 2011-05-23 17:22 UTC (permalink / raw) To: prarit; +Cc: linux-scsi, mchristi -----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; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-06-02 9:35 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <4DD6AEB7.2090900@redhat.com> 2011-05-20 18:12 ` [PATCH]: be2iscsi: Fix MSIX interrupt names Prarit Bhargava 2011-05-20 18:33 ` Jayamohan.Kallickal 2011-05-20 18:51 ` Prarit Bhargava 2011-05-20 20:17 ` Rolf Eike Beer 2011-05-24 14:48 ` Prarit Bhargava 2011-05-24 15:09 ` Rolf Eike Beer 2011-06-01 18:55 ` Jayamohan.Kallickal 2011-06-01 19:41 ` Rolf Eike Beer 2011-06-01 23:50 ` Jayamohan.Kallickal 2011-06-02 9:35 ` Rolf Eike Beer 2011-05-20 19:13 ` Prarit Bhargava 2011-05-20 22:07 ` Jayamohan.Kallickal 2011-05-20 23:45 ` Prarit Bhargava 2011-05-23 17:22 ` Jayamohan.Kallickal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.