[1/3] qla3xxx: ethernet: Fix bogus interrupt state flag.

From: Santosh Nayak <santoshprasadnayak@gmail.com>

In 'ql_adapter_initialize'
the first call for 'spin_unlock_irqrestore()' is with hw_flags = 0,
which is as good as 'spin_unlock_irq()' (unconditional interrupt
enabling). If this is intended, then for better performance
'spin_unlock_irqrestore()' can be replaced with 'spin_unlock_irq()'
and 'spin_lock_irqsave()' can be replaced by 'spin_lock_irq()

Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
 drivers/net/ethernet/qlogic/qla3xxx.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

On Fri, 2012-03-02 at 20:39 +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> In 'ql_adapter_initialize'
> the first call for 'spin_unlock_irqrestore()' is with hw_flags = 0,
> which is as good as 'spin_unlock_irq()' (unconditional interrupt
> enabling). If this is intended, then for better performance
> 'spin_unlock_irqrestore()' can be replaced with 'spin_unlock_irq()'
> and 'spin_lock_irqsave()' can be replaced by 'spin_lock_irq()

Maybe you should make the same change in ql_adapter_up(), which is the
caller that acquires this spinlock (and certainly shouldn't be called in
IRQ context).

Ben.

> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
> ---
>  drivers/net/ethernet/qlogic/qla3xxx.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qla3xxx.c b/drivers/net/ethernet/qlogic/qla3xxx.c
> index d49f6da..8da3e41 100644
> --- a/drivers/net/ethernet/qlogic/qla3xxx.c
> +++ b/drivers/net/ethernet/qlogic/qla3xxx.c
> @@ -3017,7 +3017,6 @@ static int ql_adapter_initialize(struct ql3_adapter *qdev)
>  		(void __iomem *)port_regs;
>  	u32 delay = 10;
>  	int status = 0;
> -	unsigned long hw_flags = 0;
>  
>  	if (ql_mii_setup(qdev))
>  		return -1;
> @@ -3228,9 +3227,9 @@ static int ql_adapter_initialize(struct ql3_adapter *qdev)
>  		value = ql_read_page0_reg(qdev, &port_regs->portStatus);
>  		if (value & PORT_STATUS_IC)
>  			break;
> -		spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
> +		spin_unlock_irq(&qdev->hw_lock);
>  		msleep(500);
> -		spin_lock_irqsave(&qdev->hw_lock, hw_flags);
> +		spin_lock_irq(&qdev->hw_lock);
>  	} while (--delay);
>  
>  	if (delay == 0) {

I am getting following error :

"qla3xxx.c:3231 ql_adapter_initialize() error: calling
'spin_unlock_irqrestore()' with bogus flags"


In "ql_adapter_up"  locking and unlocking  are happening in proper
sequence and the interrupt
state saved in "hw_flags" is restored.

In "ql_adapter_initialize", first unlock is done by
"spin_unlock_irqrestore(&qdev->hw_lock, hw_flags)"
with "hw_flags = 0"   ("hw_flags" is local variable and initialized to
zero.),   which is as good as
spin_unlock_irq.


To avoid the above error and  for better performance
'spin_unlock_irqrestore()' can be replaced
with 'spin_unlock_irq()' and 'spin_lock_irqsave()' can be replaced by
'spin_lock_irq()' in "ql_adapter_initialize()"


regards
santosh

On Fri, Mar 2, 2012 at 8:59 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Fri, 2012-03-02 at 20:39 +0530, santosh nayak wrote:
>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>
>> In 'ql_adapter_initialize'
>> the first call for 'spin_unlock_irqrestore()' is with hw_flags = 0,
>> which is as good as 'spin_unlock_irq()' (unconditional interrupt
>> enabling). If this is intended, then for better performance
>> 'spin_unlock_irqrestore()' can be replaced with 'spin_unlock_irq()'
>> and 'spin_lock_irqsave()' can be replaced by 'spin_lock_irq()
>
> Maybe you should make the same change in ql_adapter_up(), which is the
> caller that acquires this spinlock (and certainly shouldn't be called in
> IRQ context).
>
> Ben.
>
>> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
>> ---
>>  drivers/net/ethernet/qlogic/qla3xxx.c |    5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qla3xxx.c b/drivers/net/ethernet/qlogic/qla3xxx.c
>> index d49f6da..8da3e41 100644
>> --- a/drivers/net/ethernet/qlogic/qla3xxx.c
>> +++ b/drivers/net/ethernet/qlogic/qla3xxx.c
>> @@ -3017,7 +3017,6 @@ static int ql_adapter_initialize(struct ql3_adapter *qdev)
>>               (void __iomem *)port_regs;
>>       u32 delay = 10;
>>       int status = 0;
>> -     unsigned long hw_flags = 0;
>>
>>       if (ql_mii_setup(qdev))
>>               return -1;
>> @@ -3228,9 +3227,9 @@ static int ql_adapter_initialize(struct ql3_adapter *qdev)
>>               value = ql_read_page0_reg(qdev, &port_regs->portStatus);
>>               if (value & PORT_STATUS_IC)
>>                       break;
>> -             spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
>> +             spin_unlock_irq(&qdev->hw_lock);
>>               msleep(500);
>> -             spin_lock_irqsave(&qdev->hw_lock, hw_flags);
>> +             spin_lock_irq(&qdev->hw_lock);
>>       } while (--delay);
>>
>>       if (delay == 0) {
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: santosh prasad nayak <santoshprasadnayak@gmail.com>
Date: Fri, 2 Mar 2012 21:24:29 +0530

> In "ql_adapter_initialize", first unlock is done by
> "spin_unlock_irqrestore(&qdev->hw_lock, hw_flags)"
> with "hw_flags = 0"   ("hw_flags" is local variable and initialized to
> zero.),   which is as good as
> spin_unlock_irq.

You must never pass to irqrestore anything other than a hw_flags
value given by irqsave or similar.

You may not assume anything about what values hw_flags takes on nor
what those values might mean, they are architecture specific so
you may not just set it to zero and assume that does anything in
particular.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Mar 3, 2012 at 2:43 AM, David Miller <davem@davemloft.net> wrote:
> From: santosh prasad nayak <santoshprasadnayak@gmail.com>
> Date: Fri, 2 Mar 2012 21:24:29 +0530
>
>> In "ql_adapter_initialize", first unlock is done by
>> "spin_unlock_irqrestore(&qdev->hw_lock, hw_flags)"
>> with "hw_flags = 0"   ("hw_flags" is local variable and initialized to
>> zero.),   which is as good as
>> spin_unlock_irq.
>
> You must never pass to irqrestore anything other than a hw_flags
> value given by irqsave or similar.

David,

Thats what my point is.

The function call is as follow:

ql_adapter_up()
{
         .....
   spin_lock_irqsave(&qdev->hw_lock, hw_flags);
            .....
    err = ql_adapter_initialize(qdev);
             .....
   spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);
            ......
}

ql_adapter_initialize()
{
    unsigned long hw_flags = 0;   // D
        .......
   spin_unlock_irqrestore(&qdev->hw_lock, hw_flags);  // A
   msleep(500);                                                         // B
   spin_lock_irqsave(&qdev->hw_lock, hw_flags);        // C
        .....
}

In ql_adapter_initialize, at A,
'spin_unlock_irqrestore'  is called with "hw_flags = 0",
which is as good as spin_unlock_irq().

Static analyzer is showing it as "Error : bogus hw_flags",
which is true. Because "hw_flags" is initialized to zero at D
and the same  "hw_flags" is used to restore IRQ at A.

If intention of the developer is to unlock and enable IRQ
at A then we can use "spin_unlock_irq()"  which will
remove the static analyzer error and also give better
performance.


Regards
Santosh



> You may not assume anything about what values hw_flags takes on nor
> what those values might mean, they are architecture specific so
> you may not just set it to zero and assume that does anything in
> particular.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: santosh nayak <santoshprasadnayak@gmail.com>
Date: Fri,  2 Mar 2012 20:39:05 +0530

> From: Santosh Nayak <santoshprasadnayak@gmail.com>
> 
> In 'ql_adapter_initialize'
> the first call for 'spin_unlock_irqrestore()' is with hw_flags = 0,
> which is as good as 'spin_unlock_irq()' (unconditional interrupt
> enabling). If this is intended, then for better performance
> 'spin_unlock_irqrestore()' can be replaced with 'spin_unlock_irq()'
> and 'spin_lock_irqsave()' can be replaced by 'spin_lock_irq()
> 
> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>

This change is definitely correct, applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html