| Message ID | 1389089261-25714-1-git-send-email-yuvalmin@broadcom.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Yuval Mintz <yuvalmin@broadcom.com> Date: Tue, 7 Jan 2014 12:07:41 +0200 > + fp->state &= BNX2X_FP_STATE_DISABLED; ... > + fp->state &= BNX2X_FP_STATE_DISABLED; Surely you mean "&= ~BNX2X_FP_STATE_DISABLED" here? -- 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
> > > + fp->state &= BNX2X_FP_STATE_DISABLED; > ... > > + fp->state &= BNX2X_FP_STATE_DISABLED; > > Surely you mean "&= ~BNX2X_FP_STATE_DISABLED" here? Thought the comment above the code was sufficient to show this is intentional. The BNX2X_FP_STATE_DISABLED will cause BNX@X_FP_LOCKED to Be true. The bnx2x_fp_ll_disable() works by setting this bit even when BNX2X_FP_OWNED is set; while other flows release the lock the will clear the bit indications except for the disabled indication, so that no other flow could take the lock after it was disabled, and the loop of bnx2x_fp_ll_disable() calls will eventually return true. Thanks, Yuval -- 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: Yuval Mintz <yuvalmin@broadcom.com> Date: Wed, 8 Jan 2014 05:41:48 +0000 >> >> > + fp->state &= BNX2X_FP_STATE_DISABLED; >> ... >> > + fp->state &= BNX2X_FP_STATE_DISABLED; >> >> Surely you mean "&= ~BNX2X_FP_STATE_DISABLED" here? > > Thought the comment above the code was sufficient to show > this is intentional. > > The BNX2X_FP_STATE_DISABLED will cause BNX@X_FP_LOCKED to > Be true. The bnx2x_fp_ll_disable() works by setting this bit even > when BNX2X_FP_OWNED is set; while other flows release the lock > the will clear the bit indications except for the disabled indication, > so that no other flow could take the lock after it was disabled, > and the loop of bnx2x_fp_ll_disable() calls will eventually return true. This bit handling is completely unintuitive. The way a boolean state works is you manage it by setting it when a condition arises, and clear it when a condition goes away. -- 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
> >> > >> > + fp->state &= BNX2X_FP_STATE_DISABLED; > >> ... > >> > + fp->state &= BNX2X_FP_STATE_DISABLED; > >> > >> Surely you mean "&= ~BNX2X_FP_STATE_DISABLED" here? > > > > Thought the comment above the code was sufficient to show > > this is intentional. > > > > The BNX2X_FP_STATE_DISABLED will cause BNX@X_FP_LOCKED to > > Be true. The bnx2x_fp_ll_disable() works by setting this bit even > > when BNX2X_FP_OWNED is set; while other flows release the lock > > the will clear the bit indications except for the disabled indication, > > so that no other flow could take the lock after it was disabled, > > and the loop of bnx2x_fp_ll_disable() calls will eventually return true. > > This bit handling is completely unintuitive. > > The way a boolean state works is you manage it by setting it when > a condition arises, and clear it when a condition goes away. True; but we didn't reach this bit of intuitive code on our own - this is practically a porting of 27d9ce3f "ixgbe: fix qv_lock_napi call in ixgbe_napi_disable_all" into the bnx2x driver. -- 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: Yuval Mintz <yuvalmin@broadcom.com> Date: Wed, 8 Jan 2014 07:05:36 +0000 > True; but we didn't reach this bit of intuitive code on our own - > this is practically a porting of 27d9ce3f "ixgbe: fix qv_lock_napi > call in ixgbe_napi_disable_all" into the bnx2x driver. Fair enough, patch 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/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h index a1f66e2..46eeace 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h @@ -520,10 +520,12 @@ struct bnx2x_fastpath { #define BNX2X_FP_STATE_IDLE 0 #define BNX2X_FP_STATE_NAPI (1 << 0) /* NAPI owns this FP */ #define BNX2X_FP_STATE_POLL (1 << 1) /* poll owns this FP */ -#define BNX2X_FP_STATE_NAPI_YIELD (1 << 2) /* NAPI yielded this FP */ -#define BNX2X_FP_STATE_POLL_YIELD (1 << 3) /* poll yielded this FP */ +#define BNX2X_FP_STATE_DISABLED (1 << 2) +#define BNX2X_FP_STATE_NAPI_YIELD (1 << 3) /* NAPI yielded this FP */ +#define BNX2X_FP_STATE_POLL_YIELD (1 << 4) /* poll yielded this FP */ +#define BNX2X_FP_OWNED (BNX2X_FP_STATE_NAPI | BNX2X_FP_STATE_POLL) #define BNX2X_FP_YIELD (BNX2X_FP_STATE_NAPI_YIELD | BNX2X_FP_STATE_POLL_YIELD) -#define BNX2X_FP_LOCKED (BNX2X_FP_STATE_NAPI | BNX2X_FP_STATE_POLL) +#define BNX2X_FP_LOCKED (BNX2X_FP_OWNED | BNX2X_FP_STATE_DISABLED) #define BNX2X_FP_USER_PEND (BNX2X_FP_STATE_POLL | BNX2X_FP_STATE_POLL_YIELD) /* protect state */ spinlock_t lock; @@ -613,7 +615,7 @@ static inline bool bnx2x_fp_lock_napi(struct bnx2x_fastpath *fp) { bool rc = true; - spin_lock(&fp->lock); + spin_lock_bh(&fp->lock); if (fp->state & BNX2X_FP_LOCKED) { WARN_ON(fp->state & BNX2X_FP_STATE_NAPI); fp->state |= BNX2X_FP_STATE_NAPI_YIELD; @@ -622,7 +624,7 @@ static inline bool bnx2x_fp_lock_napi(struct bnx2x_fastpath *fp) /* we don't care if someone yielded */ fp->state = BNX2X_FP_STATE_NAPI; } - spin_unlock(&fp->lock); + spin_unlock_bh(&fp->lock); return rc; } @@ -631,14 +633,16 @@ static inline bool bnx2x_fp_unlock_napi(struct bnx2x_fastpath *fp) { bool rc = false; - spin_lock(&fp->lock); + spin_lock_bh(&fp->lock); WARN_ON(fp->state & (BNX2X_FP_STATE_POLL | BNX2X_FP_STATE_NAPI_YIELD)); if (fp->state & BNX2X_FP_STATE_POLL_YIELD) rc = true; - fp->state = BNX2X_FP_STATE_IDLE; - spin_unlock(&fp->lock); + + /* state ==> idle, unless currently disabled */ + fp->state &= BNX2X_FP_STATE_DISABLED; + spin_unlock_bh(&fp->lock); return rc; } @@ -669,7 +673,9 @@ static inline bool bnx2x_fp_unlock_poll(struct bnx2x_fastpath *fp) if (fp->state & BNX2X_FP_STATE_POLL_YIELD) rc = true; - fp->state = BNX2X_FP_STATE_IDLE; + + /* state ==> idle, unless currently disabled */ + fp->state &= BNX2X_FP_STATE_DISABLED; spin_unlock_bh(&fp->lock); return rc; } @@ -677,9 +683,23 @@ static inline bool bnx2x_fp_unlock_poll(struct bnx2x_fastpath *fp) /* true if a socket is polling, even if it did not get the lock */ static inline bool bnx2x_fp_ll_polling(struct bnx2x_fastpath *fp) { - WARN_ON(!(fp->state & BNX2X_FP_LOCKED)); + WARN_ON(!(fp->state & BNX2X_FP_OWNED)); return fp->state & BNX2X_FP_USER_PEND; } + +/* false if fp is currently owned */ +static inline bool bnx2x_fp_ll_disable(struct bnx2x_fastpath *fp) +{ + int rc = true; + + spin_lock_bh(&fp->lock); + if (fp->state & BNX2X_FP_OWNED) + rc = false; + fp->state |= BNX2X_FP_STATE_DISABLED; + spin_unlock_bh(&fp->lock); + + return rc; +} #else static inline void bnx2x_fp_init_lock(struct bnx2x_fastpath *fp) { @@ -709,6 +729,10 @@ static inline bool bnx2x_fp_ll_polling(struct bnx2x_fastpath *fp) { return false; } +static inline bool bnx2x_fp_ll_disable(struct bnx2x_fastpath *fp) +{ + return true; +} #endif /* CONFIG_NET_RX_BUSY_POLL */ /* Use 2500 as a mini-jumbo MTU for FCoE */ diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index ec96130..c6745d7 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c @@ -1790,26 +1790,22 @@ static void bnx2x_napi_disable_cnic(struct bnx2x *bp) { int i; - local_bh_disable(); for_each_rx_queue_cnic(bp, i) { napi_disable(&bnx2x_fp(bp, i, napi)); - while (!bnx2x_fp_lock_napi(&bp->fp[i])) - mdelay(1); + while (!bnx2x_fp_ll_disable(&bp->fp[i])) + usleep_range(1000, 2000); } - local_bh_enable(); } static void bnx2x_napi_disable(struct bnx2x *bp) { int i; - local_bh_disable(); for_each_eth_queue(bp, i) { napi_disable(&bnx2x_fp(bp, i, napi)); - while (!bnx2x_fp_lock_napi(&bp->fp[i])) - mdelay(1); + while (!bnx2x_fp_ll_disable(&bp->fp[i])) + usleep_range(1000, 2000); } - local_bh_enable(); } void bnx2x_netif_start(struct bnx2x *bp)