Message ID | 1330700945-18195-1-git-send-email-santoshprasadnayak@gmail.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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
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) {