* [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: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 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 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
* 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
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.