New sparse warning in lpfc_sli.c

New sparse warning in lpfc_sli.c

[Harvey Harrison]

James, somewhere between 2.6.25-rc1 and -rc2 this new warning appeared, care
to give the locking a quick once-over here?

drivers/scsi/lpfc/lpfc_sli.c:645:1: warning: context imbalance in 'lpfc_sli_hbqbuf_fill_hbqs' - wrong count at exit

Full log available if you want it.

Harvey

Re: New sparse warning in lpfc_sli.c

[Matthew Wilcox]

On Wed, Feb 20, 2008 at 11:30:19PM -0800, Harvey Harrison wrote:
> James, somewhere between 2.6.25-rc1 and -rc2 this new warning appeared, care
> to give the locking a quick once-over here?
> 
> drivers/scsi/lpfc/lpfc_sli.c:645:1: warning: context imbalance in 'lpfc_sli_hbqbuf_fill_hbqs' - wrong count at exit

Easy enough to spot by hand:

        /* Check whether HBQ is still in use */
        spin_lock_irqsave(&phba->hbalock, flags);
        if (!phba->hbq_in_use) {
                spin_unlock_irqrestore(&phba->hbalock, flags);
                return 0;
        }

        /* Populate HBQ entries */
        for (i = start; i < end; i++) {
                hbq_buffer = (phba->hbqs[hbqno].hbq_alloc_buffer)(phba);
                if (!hbq_buffer)
--->                    return 1;

Here's a patch:

lpfc: Balance locking

Commit 3163f725a5d071eea1830bbbfab78cfe3fc9baaf introduced locking in
lpfc_sli_hbqbuf_fill_hbqs, but missed unlocking on one exit.

Reported-by: Harvey Harrison <harvey.harrison@gmail.com>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
Cc: James Smart <james.smart@emulex.com>

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index f532064..fc0d950 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -648,28 +648,24 @@ lpfc_sli_hbqbuf_fill_hbqs(struct lpfc_hba *phba, uint32_t hbqno, uint32_t count)
 	unsigned long flags;
 	struct hbq_dmabuf *hbq_buffer;
 
-	if (!phba->hbqs[hbqno].hbq_alloc_buffer) {
+	if (!phba->hbqs[hbqno].hbq_alloc_buffer)
 		return 0;
-	}
 
 	start = phba->hbqs[hbqno].buffer_count;
 	end = count + start;
-	if (end > lpfc_hbq_defs[hbqno]->entry_count) {
+	if (end > lpfc_hbq_defs[hbqno]->entry_count)
 		end = lpfc_hbq_defs[hbqno]->entry_count;
-	}
 
 	/* Check whether HBQ is still in use */
 	spin_lock_irqsave(&phba->hbalock, flags);
-	if (!phba->hbq_in_use) {
-		spin_unlock_irqrestore(&phba->hbalock, flags);
-		return 0;
-	}
+	if (!phba->hbq_in_use)
+		goto out;
 
 	/* Populate HBQ entries */
 	for (i = start; i < end; i++) {
 		hbq_buffer = (phba->hbqs[hbqno].hbq_alloc_buffer)(phba);
 		if (!hbq_buffer)
-			return 1;
+			goto err;
 		hbq_buffer->tag = (i | (hbqno << 16));
 		if (lpfc_sli_hbq_to_firmware(phba, hbqno, hbq_buffer))
 			phba->hbqs[hbqno].buffer_count++;
@@ -677,8 +673,12 @@ lpfc_sli_hbqbuf_fill_hbqs(struct lpfc_hba *phba, uint32_t hbqno, uint32_t count)
 			(phba->hbqs[hbqno].hbq_free_buffer)(phba, hbq_buffer);
 	}
 
+ out:
 	spin_unlock_irqrestore(&phba->hbalock, flags);
 	return 0;
+ err:
+	spin_unlock_irqrestore(&phba->hbalock, flags);
+	return 1;
 }
 
 int

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: New sparse warning in lpfc_sli.c

[James Smart]

Thanks Matt.

ACK

-- james s


Matthew Wilcox wrote:
> On Wed, Feb 20, 2008 at 11:30:19PM -0800, Harvey Harrison wrote:
>> James, somewhere between 2.6.25-rc1 and -rc2 this new warning appeared, care
>> to give the locking a quick once-over here?
>>
>> drivers/scsi/lpfc/lpfc_sli.c:645:1: warning: context imbalance in 'lpfc_sli_hbqbuf_fill_hbqs' - wrong count at exit
> 
> Easy enough to spot by hand:
> 
>         /* Check whether HBQ is still in use */
>         spin_lock_irqsave(&phba->hbalock, flags);
>         if (!phba->hbq_in_use) {
>                 spin_unlock_irqrestore(&phba->hbalock, flags);
>                 return 0;
>         }
> 
>         /* Populate HBQ entries */
>         for (i = start; i < end; i++) {
>                 hbq_buffer = (phba->hbqs[hbqno].hbq_alloc_buffer)(phba);
>                 if (!hbq_buffer)
> --->                    return 1;
> 
> Here's a patch:
> 
> lpfc: Balance locking
> 
> Commit 3163f725a5d071eea1830bbbfab78cfe3fc9baaf introduced locking in
> lpfc_sli_hbqbuf_fill_hbqs, but missed unlocking on one exit.
> 
> Reported-by: Harvey Harrison <harvey.harrison@gmail.com>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> Cc: James Smart <james.smart@emulex.com>
> 
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index f532064..fc0d950 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -648,28 +648,24 @@ lpfc_sli_hbqbuf_fill_hbqs(struct lpfc_hba *phba, uint32_t hbqno, uint32_t count)
>  	unsigned long flags;
>  	struct hbq_dmabuf *hbq_buffer;
>  
> -	if (!phba->hbqs[hbqno].hbq_alloc_buffer) {
> +	if (!phba->hbqs[hbqno].hbq_alloc_buffer)
>  		return 0;
> -	}
>  
>  	start = phba->hbqs[hbqno].buffer_count;
>  	end = count + start;
> -	if (end > lpfc_hbq_defs[hbqno]->entry_count) {
> +	if (end > lpfc_hbq_defs[hbqno]->entry_count)
>  		end = lpfc_hbq_defs[hbqno]->entry_count;
> -	}
>  
>  	/* Check whether HBQ is still in use */
>  	spin_lock_irqsave(&phba->hbalock, flags);
> -	if (!phba->hbq_in_use) {
> -		spin_unlock_irqrestore(&phba->hbalock, flags);
> -		return 0;
> -	}
> +	if (!phba->hbq_in_use)
> +		goto out;
>  
>  	/* Populate HBQ entries */
>  	for (i = start; i < end; i++) {
>  		hbq_buffer = (phba->hbqs[hbqno].hbq_alloc_buffer)(phba);
>  		if (!hbq_buffer)
> -			return 1;
> +			goto err;
>  		hbq_buffer->tag = (i | (hbqno << 16));
>  		if (lpfc_sli_hbq_to_firmware(phba, hbqno, hbq_buffer))
>  			phba->hbqs[hbqno].buffer_count++;
> @@ -677,8 +673,12 @@ lpfc_sli_hbqbuf_fill_hbqs(struct lpfc_hba *phba, uint32_t hbqno, uint32_t count)
>  			(phba->hbqs[hbqno].hbq_free_buffer)(phba, hbq_buffer);
>  	}
>  
> + out:
>  	spin_unlock_irqrestore(&phba->hbalock, flags);
>  	return 0;
> + err:
> +	spin_unlock_irqrestore(&phba->hbalock, flags);
> +	return 1;
>  }
>  
>  int
>