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

[santosh prasad nayak]

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@xxxxxxxxxxxxxx> wrote:
> On Fri, 2012-03-02 at 20:39 +0530, santosh nayak wrote:
>> From: Santosh Nayak <santoshprasadnayak@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>>  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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/