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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




-----Original Message-----
From: Prarit Bhargava [mailto:prarit@xxxxxxxxxx] 
Sent: Friday, May 20, 2011 4:46 PM
To: Kallickal, Jayamohan
Cc: linux-scsi@xxxxxxxxxxxxxxx; mchristi@xxxxxxxxxx
Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names



On 05/20/2011 06:07 PM, Jayamohan.Kallickal@xxxxxxxxxx wrote:
> Pl see inline
> -----Original Message-----
> From: Prarit Bhargava [mailto:prarit@xxxxxxxxxx] 
> Sent: Friday, May 20, 2011 12:13 PM
> To: linux-scsi@xxxxxxxxxxxxxxx
> Cc: Kallickal, Jayamohan; mchristi@xxxxxxxxxx
> Subject: Re: [PATCH]: be2iscsi: Fix MSIX interrupt names
>
>
> [V2] Jayamohan ... I reworked the code so that the allocations of the msi_name
> don't collide with the irq_requests.  If I do it in the same for loop things
> get very messy very quickly.  This should work since the phba is zero'd out.
>
> The be2iscsi driver uses a single static array in a function for the
> irq action->name field.
>
> This results in /proc/interrupts output like
>
> 156:  0          0          0          0          0          0          0 
> 0          0          0          0          0          0          0          0 
>         0          0          0          0          0          0          0    
>      0          0       PCI-MSI-X 
> Wï9ï_ÞJïïïïïïtïïkfbYïï~}ïïïï(pïïï%ïïï'loCmïïïïïïïn`!ïvïï%ï4ïïïïb\"Pïïïïï/"ïtïïï(bïïIïïÔïï1/"ïïRmïu~ïïïïïïï
>
>                                                            
> ï8rï)ïNï>\aj*ïï+ïÐïïÛïwBäïBlïï
>
> In the line above, everything up to PCI-MSI-X is correct.  The pointer for
> action->name is garbage and scribbles the output on the screen.
>
> This patch fixes the problem:
>
> 156:          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0          0       PCI-MSI-X  beiscsi_msix_0017
>
>
> ---->8----
>
> Fix be2iscsi driver to use a separate pointer for each irq action->name field.
> Also fix a calculation error in the free_msix_irqs for loop.
>
> Successfully tested by me on an HP BL460C G7.
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 2e89f88..d44b59d 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -672,16 +672,23 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
>  	struct hwi_controller *phwi_ctrlr;
>  	struct hwi_context_memory *phwi_context;
>  	int ret, msix_vec, i, j;
> -	char desc[32];
>  
>  	phwi_ctrlr = phba->phwi_ctrlr;
>  	phwi_context = phwi_ctrlr->phwi_ctxt;
>  
> +	for (i = 0; i < phba->num_cpus; i++) {
> +		phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME, GFP_KERNEL);
> +		if (!phba->msi_name[i]) {
> +			ret = -ENOMEM;
> +			goto free_msix_names;
> +		}
> +	}
>  	if (phba->msix_enabled) {
>  		for (i = 0; i < phba->num_cpus; i++) {
> -			sprintf(desc, "beiscsi_msix_%04x", i);
> +			sprintf(phba->msi_name[i], "beiscsi_msix_%04x", i);
>  			msix_vec = phba->msix_entries[i].vector;
> -			ret = request_irq(msix_vec, be_isr_msix, 0, desc,
> +			ret = request_irq(msix_vec, be_isr_msix, 0,
> +					  phba->msi_name[i],
>  					  &phwi_context->be_eq[i]);
>  			if (ret) {
>  				shost_printk(KERN_ERR, phba->shost,
> @@ -713,8 +720,11 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
>  	}
>  	return 0;
>  free_msix_irqs:
> -	for (j = i - 1; j == 0; j++)
> +	for (j = i - 1; j == 0; j--)
>
> Agreed. Thanks
>  		free_irq(msix_vec, &phwi_context->be_eq[j]);
> +free_msix_names:
> +	for (i = 0; i < phba->num_cpus ; i++)
>
>
> I think we need to replace the above line with something like
> 	for (j = i - 1; j == 0; j--)
> as otherwise if alloc fails before reaching num_cpus - 1, we would free
>  without allocating
>   

>Oops ... cc'ing everyone.

>Actually, no -- this is okay because phba is zero'd out.  So at worst we
>end up kfree'ing a bunch of NULL pointers which is okay.

Ok. You are technically right and I have seen code that relies on free skipping NULL.
Just an uneasy feeling of knowingly doing it. Just my thoughts...

>I'll take a look at Rolf's suggestions ...

>P.

>
> +		kfree(phba->msi_name[i]);
>  	return ret;
>  }
>  
> @@ -3832,6 +3842,7 @@ static void beiscsi_remove(struct pci_dev *pcidev)
>  		for (i = 0; i <= phba->num_cpus; i++) {
>  			msix_vec = phba->msix_entries[i].vector;
>  			free_irq(msix_vec, &phwi_context->be_eq[i]);
> +			kfree(phba->msi_name[i]);
>  		}
>  	} else
>  		if (phba->pcidev->irq)
> diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
> index d1ee00b..08cc714 100644
> --- a/drivers/scsi/be2iscsi/be_main.h
> +++ b/drivers/scsi/be2iscsi/be_main.h
> @@ -163,6 +163,8 @@ do {							\
>  #define PAGES_REQUIRED(x) \
>  	((x < PAGE_SIZE) ? 1 :  ((x + PAGE_SIZE - 1) / PAGE_SIZE))
>  
> +#define BEISCSI_MSI_NAME 20 /* size of msi_name string */
> +
>  enum be_mem_enum {
>  	HWI_MEM_ADDN_CONTEXT,
>  	HWI_MEM_WRB,
> @@ -288,6 +290,7 @@ struct beiscsi_hba {
>  	unsigned int num_cpus;
>  	unsigned int nxt_cqid;
>  	struct msix_entry msix_entries[MAX_CPUS + 1];
> +	char *msi_name[MAX_CPUS + 1];
>  	bool msix_enabled;
>  	struct be_mem_descriptor *init_mem;
>  
>   
ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¥Š{±þÇ‹ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux