Re: [PATCH]: be2iscsi: Fix MSIX interrupt names

[Prarit Bhargava <prarit@redhat.com>]

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