[PATCH 0/7 net-next] bnx2x: cleanups and VLAN stripping toggle

[PATCH 0/7 net-next] bnx2x: cleanups and VLAN stripping toggle

[Michal Schmidt]

Hello,
patches 1-6 are cleanups.
Patch 7 adds HW VLAN stripping control via ethtool.

Michal Schmidt (7):
  bnx2x: remove unused fields in struct bnx2x_func_init_params
  bnx2x: remove the 'leading' arguments
  bnx2x: decrease indentation in bnx2x_rx_int()
  bnx2x: simplify TPA sanity check
  bnx2x: do not set TPA flags and features in bnx2x_init_bp
  bnx2x: move fp->disable_tpa to ->flags
  bnx2x: expose HW RX VLAN stripping toggle

 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |   21 +--
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |  242 +++++++++++-----------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h  |    6 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   62 ++----
 4 files changed, 148 insertions(+), 183 deletions(-)

-- 
1.7.6

[PATCH 1/7] bnx2x: remove unused fields in struct bnx2x_func_init_params

[Michal Schmidt]

func_flgs is not used for anything. The only flag that's ever checked
(FUNC_FLG_SPQ) is always set. The other flags are never read.

fw_stat_map is not used at all.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |   15 ++-------------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   18 +++---------------
 2 files changed, 5 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index f127768..735e491 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1490,24 +1490,13 @@ extern int num_queues;
 #define RSS_IPV6_TCP_CAP_MASK						\
 	TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_TCP_CAPABILITY
 
-/* func init flags */
-#define FUNC_FLG_RSS		0x0001
-#define FUNC_FLG_STATS		0x0002
-/* removed  FUNC_FLG_UNMATCHED	0x0004 */
-#define FUNC_FLG_TPA		0x0008
-#define FUNC_FLG_SPQ		0x0010
-#define FUNC_FLG_LEADING	0x0020	/* PF only */
-
-
 struct bnx2x_func_init_params {
 	/* dma */
-	dma_addr_t	fw_stat_map;	/* valid iff FUNC_FLG_STATS */
-	dma_addr_t	spq_map;	/* valid iff FUNC_FLG_SPQ */
+	dma_addr_t	spq_map;
 
-	u16		func_flgs;
 	u16		func_id;	/* abs fid */
 	u16		pf_id;
-	u16		spq_prod;	/* valid iff FUNC_FLG_SPQ */
+	u16		spq_prod;
 };
 
 #define for_each_eth_queue(bp, var) \
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 85dd294..e7b584b 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -2661,11 +2661,9 @@ void bnx2x_func_init(struct bnx2x *bp, struct bnx2x_func_init_params *p)
 	storm_memset_func_en(bp, p->func_id, 1);
 
 	/* spq */
-	if (p->func_flgs & FUNC_FLG_SPQ) {
-		storm_memset_spq_addr(bp, p->spq_map, p->func_id);
-		REG_WR(bp, XSEM_REG_FAST_MEMORY +
-		       XSTORM_SPQ_PROD_OFFSET(p->func_id), p->spq_prod);
-	}
+	storm_memset_spq_addr(bp, p->spq_map, p->func_id);
+	REG_WR(bp, XSEM_REG_FAST_MEMORY +
+	       XSTORM_SPQ_PROD_OFFSET(p->func_id), p->spq_prod);
 }
 
 /**
@@ -2838,7 +2836,6 @@ static void bnx2x_pf_init(struct bnx2x *bp)
 {
 	struct bnx2x_func_init_params func_init = {0};
 	struct event_ring_data eq_data = { {0} };
-	u16 flags;
 
 	if (!CHIP_IS_E1x(bp)) {
 		/* reset IGU PF statistics: MSIX + ATTN */
@@ -2855,15 +2852,6 @@ static void bnx2x_pf_init(struct bnx2x *bp)
 				BP_FUNC(bp) : BP_VN(bp))*4, 0);
 	}
 
-	/* function setup flags */
-	flags = (FUNC_FLG_STATS | FUNC_FLG_LEADING | FUNC_FLG_SPQ);
-
-	/* This flag is relevant for E1x only.
-	 * E2 doesn't have a TPA configuration in a function level.
-	 */
-	flags |= (bp->flags & TPA_ENABLE_FLAG) ? FUNC_FLG_TPA : 0;
-
-	func_init.func_flgs = flags;
 	func_init.pf_id = BP_FUNC(bp);
 	func_init.func_id = BP_FUNC(bp);
 	func_init.spq_map = bp->spq_mapping;
-- 
1.7.6

Re: [PATCH 1/7] bnx2x: remove unused fields in struct bnx2x_func_init_params

[Vlad Zolotarov]

On Tuesday 30 August 2011 17:30:40 Michal Schmidt wrote:
> func_flgs is not used for anything. The only flag that's ever checked
> (FUNC_FLG_SPQ) is always set. The other flags are never read.
> 
> fw_stat_map is not used at all.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |   15 ++-------------
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   18 +++---------------
>  2 files changed, 5 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h index f127768..735e491
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -1490,24 +1490,13 @@ extern int num_queues;
>  #define RSS_IPV6_TCP_CAP_MASK						
\
>  	TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV6_TCP_CAPABILITY
> 
> -/* func init flags */
> -#define FUNC_FLG_RSS		0x0001
> -#define FUNC_FLG_STATS		0x0002
> -/* removed  FUNC_FLG_UNMATCHED	0x0004 */
> -#define FUNC_FLG_TPA		0x0008
> -#define FUNC_FLG_SPQ		0x0010
> -#define FUNC_FLG_LEADING	0x0020	/* PF only */
> -
> -
>  struct bnx2x_func_init_params {
>  	/* dma */
> -	dma_addr_t	fw_stat_map;	/* valid iff FUNC_FLG_STATS */
> -	dma_addr_t	spq_map;	/* valid iff FUNC_FLG_SPQ */
> +	dma_addr_t	spq_map;
> 
> -	u16		func_flgs;
>  	u16		func_id;	/* abs fid */
>  	u16		pf_id;
> -	u16		spq_prod;	/* valid iff FUNC_FLG_SPQ */
> +	u16		spq_prod;
>  };
> 
>  #define for_each_eth_queue(bp, var) \
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index 85dd294..e7b584b
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -2661,11 +2661,9 @@ void bnx2x_func_init(struct bnx2x *bp, struct
> bnx2x_func_init_params *p) storm_memset_func_en(bp, p->func_id, 1);
> 
>  	/* spq */
> -	if (p->func_flgs & FUNC_FLG_SPQ) {
> -		storm_memset_spq_addr(bp, p->spq_map, p->func_id);
> -		REG_WR(bp, XSEM_REG_FAST_MEMORY +
> -		       XSTORM_SPQ_PROD_OFFSET(p->func_id), p->spq_prod);
> -	}
> +	storm_memset_spq_addr(bp, p->spq_map, p->func_id);
> +	REG_WR(bp, XSEM_REG_FAST_MEMORY +
> +	       XSTORM_SPQ_PROD_OFFSET(p->func_id), p->spq_prod);
>  }
> 
>  /**
> @@ -2838,7 +2836,6 @@ static void bnx2x_pf_init(struct bnx2x *bp)
>  {
>  	struct bnx2x_func_init_params func_init = {0};
>  	struct event_ring_data eq_data = { {0} };
> -	u16 flags;
> 
>  	if (!CHIP_IS_E1x(bp)) {
>  		/* reset IGU PF statistics: MSIX + ATTN */
> @@ -2855,15 +2852,6 @@ static void bnx2x_pf_init(struct bnx2x *bp)
>  				BP_FUNC(bp) : BP_VN(bp))*4, 0);
>  	}
> 
> -	/* function setup flags */
> -	flags = (FUNC_FLG_STATS | FUNC_FLG_LEADING | FUNC_FLG_SPQ);
> -
> -	/* This flag is relevant for E1x only.
> -	 * E2 doesn't have a TPA configuration in a function level.
> -	 */
> -	flags |= (bp->flags & TPA_ENABLE_FLAG) ? FUNC_FLG_TPA : 0;
> -
> -	func_init.func_flgs = flags;
>  	func_init.pf_id = BP_FUNC(bp);
>  	func_init.func_id = BP_FUNC(bp);
>  	func_init.spq_map = bp->spq_mapping;

Acked-by: Vladislav Zolotarov <vladz@broadcom.com>

[PATCH 2/7] bnx2x: remove the 'leading' arguments

[Michal Schmidt]

Whether a queue is leading can be deduced from its index.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |    2 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h  |    4 +---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   21 +++++++++------------
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 735e491..c0d2d9c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -531,6 +531,7 @@ struct bnx2x_fastpath {
 
 #define IS_ETH_FP(fp)			(fp->index < \
 					 BNX2X_NUM_ETH_QUEUES(fp->bp))
+#define IS_LEADING_FP(fp)		((fp)->index == 0)
 #ifdef BCM_CNIC
 #define IS_FCOE_FP(fp)			(fp->index == FCOE_IDX)
 #define IS_FCOE_IDX(idx)		((idx) == FCOE_IDX)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 5c3eb17..448e301 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -1881,7 +1881,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
 #endif
 
 	for_each_nondefault_queue(bp, i) {
-		rc = bnx2x_setup_queue(bp, &bp->fp[i], 0);
+		rc = bnx2x_setup_queue(bp, &bp->fp[i]);
 		if (rc)
 			LOAD_ERROR_EXIT(bp, load_error4);
 	}
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
index 5b1f9b5..54d50b7 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
@@ -109,11 +109,9 @@ void bnx2x__init_func_obj(struct bnx2x *bp);
  *
  * @bp:		driver handle
  * @fp:		pointer to the fastpath structure
- * @leading:	boolean
  *
  */
-int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
-		       bool leading);
+int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp);
 
 /**
  * bnx2x_setup_leading - bring up a leading eth queue.
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index e7b584b..64314f7 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -2697,9 +2697,8 @@ static inline unsigned long bnx2x_get_common_flags(struct bnx2x *bp,
 	return flags;
 }
 
-static inline unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
-					      struct bnx2x_fastpath *fp,
-					      bool leading)
+static unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
+				       struct bnx2x_fastpath *fp)
 {
 	unsigned long flags = 0;
 
@@ -2715,7 +2714,7 @@ static inline unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
 		__set_bit(BNX2X_Q_FLG_TPA_IPV6, &flags);
 	}
 
-	if (leading) {
+	if (IS_LEADING_FP(fp)) {
 		__set_bit(BNX2X_Q_FLG_LEADING_RSS, &flags);
 		__set_bit(BNX2X_Q_FLG_MCAST, &flags);
 	}
@@ -6966,7 +6965,7 @@ int bnx2x_set_eth_mac(struct bnx2x *bp, bool set)
 
 int bnx2x_setup_leading(struct bnx2x *bp)
 {
-	return bnx2x_setup_queue(bp, &bp->fp[0], 1);
+	return bnx2x_setup_queue(bp, &bp->fp[0]);
 }
 
 /**
@@ -7177,10 +7176,10 @@ static inline void bnx2x_pf_q_prep_init(struct bnx2x *bp,
 			&bp->context.vcxt[fp->txdata[cos].cid].eth;
 }
 
-int bnx2x_setup_tx_only(struct bnx2x *bp, struct bnx2x_fastpath *fp,
+static int bnx2x_setup_tx_only(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 			struct bnx2x_queue_state_params *q_params,
 			struct bnx2x_queue_setup_tx_only_params *tx_only_params,
-			int tx_index, bool leading)
+			int tx_index)
 {
 	memset(tx_only_params, 0, sizeof(*tx_only_params));
 
@@ -7216,14 +7215,12 @@ int bnx2x_setup_tx_only(struct bnx2x *bp, struct bnx2x_fastpath *fp,
  *
  * @bp:		driver handle
  * @fp:		pointer to fastpath
- * @leading:	is leading
  *
  * This function performs 2 steps in a Queue state machine
  *      actually: 1) RESET->INIT 2) INIT->SETUP
  */
 
-int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
-		       bool leading)
+int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp)
 {
 	struct bnx2x_queue_state_params q_params = {0};
 	struct bnx2x_queue_setup_params *setup_params =
@@ -7264,7 +7261,7 @@ int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	memset(setup_params, 0, sizeof(*setup_params));
 
 	/* Set QUEUE flags */
-	setup_params->flags = bnx2x_get_q_flags(bp, fp, leading);
+	setup_params->flags = bnx2x_get_q_flags(bp, fp);
 
 	/* Set general SETUP parameters */
 	bnx2x_pf_q_prep_general(bp, fp, &setup_params->gen_params,
@@ -7293,7 +7290,7 @@ int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 
 		/* prepare and send tx-only ramrod*/
 		rc = bnx2x_setup_tx_only(bp, fp, &q_params,
-					  tx_only_params, tx_index, leading);
+					  tx_only_params, tx_index);
 		if (rc) {
 			BNX2X_ERR("Queue(%d.%d) TX_ONLY_SETUP failed\n",
 				  fp->index, tx_index);
-- 
1.7.6

Re: [PATCH 2/7] bnx2x: remove the 'leading' arguments

[Vlad Zolotarov]

On Tuesday 30 August 2011 17:30:41 Michal Schmidt wrote:
> Whether a queue is leading can be deduced from its index.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    1 +
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |    2 +-
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h  |    4 +---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   21
> +++++++++------------ 4 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h index 735e491..c0d2d9c
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -531,6 +531,7 @@ struct bnx2x_fastpath {
> 
>  #define IS_ETH_FP(fp)			(fp->index < \
>  					 BNX2X_NUM_ETH_QUEUES(fp->bp))
> +#define IS_LEADING_FP(fp)		((fp)->index == 0)
>  #ifdef BCM_CNIC
>  #define IS_FCOE_FP(fp)			(fp->index == FCOE_IDX)
>  #define IS_FCOE_IDX(idx)		((idx) == FCOE_IDX)
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index 5c3eb17..448e301
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -1881,7 +1881,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
>  #endif
> 
>  	for_each_nondefault_queue(bp, i) {
> -		rc = bnx2x_setup_queue(bp, &bp->fp[i], 0);
> +		rc = bnx2x_setup_queue(bp, &bp->fp[i]);
>  		if (rc)
>  			LOAD_ERROR_EXIT(bp, load_error4);
>  	}
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h index 5b1f9b5..54d50b7
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
> @@ -109,11 +109,9 @@ void bnx2x__init_func_obj(struct bnx2x *bp);
>   *
>   * @bp:		driver handle
>   * @fp:		pointer to the fastpath structure
> - * @leading:	boolean
>   *
>   */
> -int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
> -		       bool leading);
> +int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp);
> 
>  /**
>   * bnx2x_setup_leading - bring up a leading eth queue.
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index e7b584b..64314f7
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -2697,9 +2697,8 @@ static inline unsigned long
> bnx2x_get_common_flags(struct bnx2x *bp, return flags;
>  }
> 
> -static inline unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
> -					      struct bnx2x_fastpath *fp,
> -					      bool leading)
> +static unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
> +				       struct bnx2x_fastpath *fp)
>  {
>  	unsigned long flags = 0;
> 
> @@ -2715,7 +2714,7 @@ static inline unsigned long bnx2x_get_q_flags(struct
> bnx2x *bp, __set_bit(BNX2X_Q_FLG_TPA_IPV6, &flags);
>  	}
> 
> -	if (leading) {
> +	if (IS_LEADING_FP(fp)) {
>  		__set_bit(BNX2X_Q_FLG_LEADING_RSS, &flags);
>  		__set_bit(BNX2X_Q_FLG_MCAST, &flags);
>  	}
> @@ -6966,7 +6965,7 @@ int bnx2x_set_eth_mac(struct bnx2x *bp, bool set)
> 
>  int bnx2x_setup_leading(struct bnx2x *bp)
>  {
> -	return bnx2x_setup_queue(bp, &bp->fp[0], 1);
> +	return bnx2x_setup_queue(bp, &bp->fp[0]);
>  }
> 
>  /**
> @@ -7177,10 +7176,10 @@ static inline void bnx2x_pf_q_prep_init(struct
> bnx2x *bp, &bp->context.vcxt[fp->txdata[cos].cid].eth;
>  }
> 
> -int bnx2x_setup_tx_only(struct bnx2x *bp, struct bnx2x_fastpath *fp,
> +static int bnx2x_setup_tx_only(struct bnx2x *bp, struct bnx2x_fastpath
> *fp, struct bnx2x_queue_state_params *q_params,
>  			struct bnx2x_queue_setup_tx_only_params 
*tx_only_params,
> -			int tx_index, bool leading)
> +			int tx_index)
>  {
>  	memset(tx_only_params, 0, sizeof(*tx_only_params));
> 
> @@ -7216,14 +7215,12 @@ int bnx2x_setup_tx_only(struct bnx2x *bp, struct
> bnx2x_fastpath *fp, *
>   * @bp:		driver handle
>   * @fp:		pointer to fastpath
> - * @leading:	is leading
>   *
>   * This function performs 2 steps in a Queue state machine
>   *      actually: 1) RESET->INIT 2) INIT->SETUP
>   */
> 
> -int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp,
> -		       bool leading)
> +int bnx2x_setup_queue(struct bnx2x *bp, struct bnx2x_fastpath *fp)
>  {
>  	struct bnx2x_queue_state_params q_params = {0};
>  	struct bnx2x_queue_setup_params *setup_params =
> @@ -7264,7 +7261,7 @@ int bnx2x_setup_queue(struct bnx2x *bp, struct
> bnx2x_fastpath *fp, memset(setup_params, 0, sizeof(*setup_params));
> 
>  	/* Set QUEUE flags */
> -	setup_params->flags = bnx2x_get_q_flags(bp, fp, leading);
> +	setup_params->flags = bnx2x_get_q_flags(bp, fp);
> 
>  	/* Set general SETUP parameters */
>  	bnx2x_pf_q_prep_general(bp, fp, &setup_params->gen_params,
> @@ -7293,7 +7290,7 @@ int bnx2x_setup_queue(struct bnx2x *bp, struct
> bnx2x_fastpath *fp,
> 
>  		/* prepare and send tx-only ramrod*/
>  		rc = bnx2x_setup_tx_only(bp, fp, &q_params,
> -					  tx_only_params, tx_index, leading);
> +					  tx_only_params, tx_index);
>  		if (rc) {
>  			BNX2X_ERR("Queue(%d.%d) TX_ONLY_SETUP failed\n",
>  				  fp->index, tx_index);

NACK

Removing this parameter would decrese the flexability of our code.
For instance we are using this function in our KVM code, which is under the 
development now, where we define a few RSS groups on the same PF and then 
"leading" fp may have an index different from 0.

It's a shame to remove this code now in order to submit it back later...

thanks,
vlad

[PATCH 3/7] bnx2x: decrease indentation in bnx2x_rx_int()

[Michal Schmidt]

For better readability decrease the indentation in bnx2x_rx_int().
'else' is unnecessary when the positive branch ends with a 'goto'.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |  194 +++++++++++------------
 1 files changed, 92 insertions(+), 102 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 448e301..f1fea58 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -624,135 +624,125 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
 		if (unlikely(CQE_TYPE_SLOW(cqe_fp_type))) {
 			bnx2x_sp_event(fp, cqe);
 			goto next_cqe;
+		}
 
 		/* this is an rx packet */
-		} else {
-			rx_buf = &fp->rx_buf_ring[bd_cons];
-			skb = rx_buf->skb;
-			prefetch(skb);
+		rx_buf = &fp->rx_buf_ring[bd_cons];
+		skb = rx_buf->skb;
+		prefetch(skb);
 
-			if (!CQE_TYPE_FAST(cqe_fp_type)) {
+		if (!CQE_TYPE_FAST(cqe_fp_type)) {
 #ifdef BNX2X_STOP_ON_ERROR
-				/* sanity check */
-				if (fp->disable_tpa &&
-				    (CQE_TYPE_START(cqe_fp_type) ||
-				     CQE_TYPE_STOP(cqe_fp_type)))
-					BNX2X_ERR("START/STOP packet while "
-						  "disable_tpa type %x\n",
-						  CQE_TYPE(cqe_fp_type));
+			/* sanity check */
+			if (fp->disable_tpa &&
+			    (CQE_TYPE_START(cqe_fp_type) ||
+			     CQE_TYPE_STOP(cqe_fp_type)))
+				BNX2X_ERR("START/STOP packet while "
+					  "disable_tpa type %x\n",
+					  CQE_TYPE(cqe_fp_type));
 #endif
 
-				if (CQE_TYPE_START(cqe_fp_type)) {
-					u16 queue = cqe_fp->queue_index;
-					DP(NETIF_MSG_RX_STATUS,
-					   "calling tpa_start on queue %d\n",
-					   queue);
+			if (CQE_TYPE_START(cqe_fp_type)) {
+				u16 queue = cqe_fp->queue_index;
+				DP(NETIF_MSG_RX_STATUS,
+				   "calling tpa_start on queue %d\n", queue);
 
-					bnx2x_tpa_start(fp, queue, skb,
-							bd_cons, bd_prod,
-							cqe_fp);
+				bnx2x_tpa_start(fp, queue, skb,
+						bd_cons, bd_prod, cqe_fp);
 
-					/* Set Toeplitz hash for LRO skb */
-					bnx2x_set_skb_rxhash(bp, cqe, skb);
+				/* Set Toeplitz hash for LRO skb */
+				bnx2x_set_skb_rxhash(bp, cqe, skb);
 
-					goto next_rx;
+				goto next_rx;
 
-				} else {
-					u16 queue =
-						cqe->end_agg_cqe.queue_index;
-					DP(NETIF_MSG_RX_STATUS,
-					   "calling tpa_stop on queue %d\n",
-					   queue);
+			} else {
+				u16 queue =
+					cqe->end_agg_cqe.queue_index;
+				DP(NETIF_MSG_RX_STATUS,
+				   "calling tpa_stop on queue %d\n", queue);
 
-					bnx2x_tpa_stop(bp, fp, queue,
-						       &cqe->end_agg_cqe,
-						       comp_ring_cons);
+				bnx2x_tpa_stop(bp, fp, queue, &cqe->end_agg_cqe,
+					       comp_ring_cons);
 #ifdef BNX2X_STOP_ON_ERROR
-					if (bp->panic)
-						return 0;
+				if (bp->panic)
+					return 0;
 #endif
 
-					bnx2x_update_sge_prod(fp, cqe_fp);
-					goto next_cqe;
-				}
+				bnx2x_update_sge_prod(fp, cqe_fp);
+				goto next_cqe;
 			}
-			/* non TPA */
-			len = le16_to_cpu(cqe_fp->pkt_len);
-			pad = cqe_fp->placement_offset;
-			dma_sync_single_for_cpu(&bp->pdev->dev,
+		}
+		/* non TPA */
+		len = le16_to_cpu(cqe_fp->pkt_len);
+		pad = cqe_fp->placement_offset;
+		dma_sync_single_for_cpu(&bp->pdev->dev,
 					dma_unmap_addr(rx_buf, mapping),
-						       pad + RX_COPY_THRESH,
-						       DMA_FROM_DEVICE);
-			prefetch(((char *)(skb)) + L1_CACHE_BYTES);
+					pad + RX_COPY_THRESH, DMA_FROM_DEVICE);
+		prefetch(((char *)(skb)) + L1_CACHE_BYTES);
 
-			/* is this an error packet? */
-			if (unlikely(cqe_fp_flags & ETH_RX_ERROR_FALGS)) {
+		/* is this an error packet? */
+		if (unlikely(cqe_fp_flags & ETH_RX_ERROR_FALGS)) {
+			DP(NETIF_MSG_RX_ERR, "ERROR  flags %x  rx packet %u\n",
+			   cqe_fp_flags, sw_comp_cons);
+			fp->eth_q_stats.rx_err_discard_pkt++;
+			goto reuse_rx;
+		}
+
+		/*
+		 * Since we don't have a jumbo ring,
+		 * copy small packets if mtu > 1500
+		 */
+		if ((bp->dev->mtu > ETH_MAX_PACKET_SIZE) &&
+		    (len <= RX_COPY_THRESH)) {
+			struct sk_buff *new_skb;
+
+			new_skb = netdev_alloc_skb(bp->dev, len + pad);
+			if (new_skb == NULL) {
 				DP(NETIF_MSG_RX_ERR,
-				   "ERROR  flags %x  rx packet %u\n",
-				   cqe_fp_flags, sw_comp_cons);
-				fp->eth_q_stats.rx_err_discard_pkt++;
+				   "ERROR  packet dropped "
+				   "because of alloc failure\n");
+				fp->eth_q_stats.rx_skb_alloc_failed++;
 				goto reuse_rx;
 			}
 
-			/* Since we don't have a jumbo ring
-			 * copy small packets if mtu > 1500
-			 */
-			if ((bp->dev->mtu > ETH_MAX_PACKET_SIZE) &&
-			    (len <= RX_COPY_THRESH)) {
-				struct sk_buff *new_skb;
-
-				new_skb = netdev_alloc_skb(bp->dev, len + pad);
-				if (new_skb == NULL) {
-					DP(NETIF_MSG_RX_ERR,
-					   "ERROR  packet dropped "
-					   "because of alloc failure\n");
-					fp->eth_q_stats.rx_skb_alloc_failed++;
-					goto reuse_rx;
-				}
-
-				/* aligned copy */
-				skb_copy_from_linear_data_offset(skb, pad,
-						    new_skb->data + pad, len);
-				skb_reserve(new_skb, pad);
-				skb_put(new_skb, len);
+			/* aligned copy */
+			skb_copy_from_linear_data_offset(skb, pad,
+						new_skb->data + pad, len);
+			skb_reserve(new_skb, pad);
+			skb_put(new_skb, len);
 
-				bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
+			bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
 
-				skb = new_skb;
+			skb = new_skb;
 
-			} else
-			if (likely(bnx2x_alloc_rx_skb(bp, fp, bd_prod) == 0)) {
-				dma_unmap_single(&bp->pdev->dev,
-					dma_unmap_addr(rx_buf, mapping),
-						 fp->rx_buf_size,
-						 DMA_FROM_DEVICE);
-				skb_reserve(skb, pad);
-				skb_put(skb, len);
+		} else if (likely(bnx2x_alloc_rx_skb(bp, fp, bd_prod) == 0)) {
+			dma_unmap_single(&bp->pdev->dev,
+					 dma_unmap_addr(rx_buf, mapping),
+					 fp->rx_buf_size, DMA_FROM_DEVICE);
+			skb_reserve(skb, pad);
+			skb_put(skb, len);
 
-			} else {
-				DP(NETIF_MSG_RX_ERR,
-				   "ERROR  packet dropped because "
-				   "of alloc failure\n");
-				fp->eth_q_stats.rx_skb_alloc_failed++;
+		} else {
+			DP(NETIF_MSG_RX_ERR,
+			   "ERROR  packet dropped because of alloc failure\n");
+			fp->eth_q_stats.rx_skb_alloc_failed++;
 reuse_rx:
-				bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
-				goto next_rx;
-			}
-
-			skb->protocol = eth_type_trans(skb, bp->dev);
+			bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
+			goto next_rx;
+		}
 
-			/* Set Toeplitz hash for a none-LRO skb */
-			bnx2x_set_skb_rxhash(bp, cqe, skb);
+		skb->protocol = eth_type_trans(skb, bp->dev);
 
-			skb_checksum_none_assert(skb);
+		/* Set Toeplitz hash for a none-LRO skb */
+		bnx2x_set_skb_rxhash(bp, cqe, skb);
 
-			if (bp->dev->features & NETIF_F_RXCSUM) {
+		skb_checksum_none_assert(skb);
 
-				if (likely(BNX2X_RX_CSUM_OK(cqe)))
-					skb->ip_summed = CHECKSUM_UNNECESSARY;
-				else
-					fp->eth_q_stats.hw_csum_err++;
-			}
+		if (bp->dev->features & NETIF_F_RXCSUM) {
+			if (likely(BNX2X_RX_CSUM_OK(cqe)))
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+			else
+				fp->eth_q_stats.hw_csum_err++;
 		}
 
 		skb_record_rx_queue(skb, fp->index);
-- 
1.7.6

Re: [PATCH 3/7] bnx2x: decrease indentation in bnx2x_rx_int()

[Vlad Zolotarov]

On Tuesday 30 August 2011 17:30:42 Michal Schmidt wrote:
> For better readability decrease the indentation in bnx2x_rx_int().
> 'else' is unnecessary when the positive branch ends with a 'goto'.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Acked-by: Dmitry Kravkov <dmitry@broadcom.com>

> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |  194
> +++++++++++------------ 1 files changed, 92 insertions(+), 102
> deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index 448e301..f1fea58
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -624,135 +624,125 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int
> budget) if (unlikely(CQE_TYPE_SLOW(cqe_fp_type))) {
>  			bnx2x_sp_event(fp, cqe);
>  			goto next_cqe;
> +		}
> 
>  		/* this is an rx packet */
> -		} else {
> -			rx_buf = &fp->rx_buf_ring[bd_cons];
> -			skb = rx_buf->skb;
> -			prefetch(skb);
> +		rx_buf = &fp->rx_buf_ring[bd_cons];
> +		skb = rx_buf->skb;
> +		prefetch(skb);
> 
> -			if (!CQE_TYPE_FAST(cqe_fp_type)) {
> +		if (!CQE_TYPE_FAST(cqe_fp_type)) {
>  #ifdef BNX2X_STOP_ON_ERROR
> -				/* sanity check */
> -				if (fp->disable_tpa &&
> -				    (CQE_TYPE_START(cqe_fp_type) ||
> -				     CQE_TYPE_STOP(cqe_fp_type)))
> -					BNX2X_ERR("START/STOP packet while "
> -						  "disable_tpa type %x\n",
> -						  CQE_TYPE(cqe_fp_type));
> +			/* sanity check */
> +			if (fp->disable_tpa &&
> +			    (CQE_TYPE_START(cqe_fp_type) ||
> +			     CQE_TYPE_STOP(cqe_fp_type)))
> +				BNX2X_ERR("START/STOP packet while "
> +					  "disable_tpa type %x\n",
> +					  CQE_TYPE(cqe_fp_type));
>  #endif
> 
> -				if (CQE_TYPE_START(cqe_fp_type)) {
> -					u16 queue = cqe_fp->queue_index;
> -					DP(NETIF_MSG_RX_STATUS,
> -					   "calling tpa_start on queue %d\n",
> -					   queue);
> +			if (CQE_TYPE_START(cqe_fp_type)) {
> +				u16 queue = cqe_fp->queue_index;
> +				DP(NETIF_MSG_RX_STATUS,
> +				   "calling tpa_start on queue %d\n", queue);
> 
> -					bnx2x_tpa_start(fp, queue, skb,
> -							bd_cons, bd_prod,
> -							cqe_fp);
> +				bnx2x_tpa_start(fp, queue, skb,
> +						bd_cons, bd_prod, cqe_fp);
> 
> -					/* Set Toeplitz hash for LRO skb */
> -					bnx2x_set_skb_rxhash(bp, cqe, skb);
> +				/* Set Toeplitz hash for LRO skb */
> +				bnx2x_set_skb_rxhash(bp, cqe, skb);
> 
> -					goto next_rx;
> +				goto next_rx;
> 
> -				} else {
> -					u16 queue =
> -						cqe->end_agg_cqe.queue_index;
> -					DP(NETIF_MSG_RX_STATUS,
> -					   "calling tpa_stop on queue %d\n",
> -					   queue);
> +			} else {
> +				u16 queue =
> +					cqe->end_agg_cqe.queue_index;
> +				DP(NETIF_MSG_RX_STATUS,
> +				   "calling tpa_stop on queue %d\n", queue);
> 
> -					bnx2x_tpa_stop(bp, fp, queue,
> -						       &cqe->end_agg_cqe,
> -						       comp_ring_cons);
> +				bnx2x_tpa_stop(bp, fp, queue, &cqe-
>end_agg_cqe,
> +					       comp_ring_cons);
>  #ifdef BNX2X_STOP_ON_ERROR
> -					if (bp->panic)
> -						return 0;
> +				if (bp->panic)
> +					return 0;
>  #endif
> 
> -					bnx2x_update_sge_prod(fp, cqe_fp);
> -					goto next_cqe;
> -				}
> +				bnx2x_update_sge_prod(fp, cqe_fp);
> +				goto next_cqe;
>  			}
> -			/* non TPA */
> -			len = le16_to_cpu(cqe_fp->pkt_len);
> -			pad = cqe_fp->placement_offset;
> -			dma_sync_single_for_cpu(&bp->pdev->dev,
> +		}
> +		/* non TPA */
> +		len = le16_to_cpu(cqe_fp->pkt_len);
> +		pad = cqe_fp->placement_offset;
> +		dma_sync_single_for_cpu(&bp->pdev->dev,
>  					dma_unmap_addr(rx_buf, mapping),
> -						       pad + RX_COPY_THRESH,
> -						       DMA_FROM_DEVICE);
> -			prefetch(((char *)(skb)) + L1_CACHE_BYTES);
> +					pad + RX_COPY_THRESH, 
DMA_FROM_DEVICE);
> +		prefetch(((char *)(skb)) + L1_CACHE_BYTES);
> 
> -			/* is this an error packet? */
> -			if (unlikely(cqe_fp_flags & ETH_RX_ERROR_FALGS)) {
> +		/* is this an error packet? */
> +		if (unlikely(cqe_fp_flags & ETH_RX_ERROR_FALGS)) {
> +			DP(NETIF_MSG_RX_ERR, "ERROR  flags %x  rx packet 
%u\n",
> +			   cqe_fp_flags, sw_comp_cons);
> +			fp->eth_q_stats.rx_err_discard_pkt++;
> +			goto reuse_rx;
> +		}
> +
> +		/*
> +		 * Since we don't have a jumbo ring,
> +		 * copy small packets if mtu > 1500
> +		 */
> +		if ((bp->dev->mtu > ETH_MAX_PACKET_SIZE) &&
> +		    (len <= RX_COPY_THRESH)) {
> +			struct sk_buff *new_skb;
> +
> +			new_skb = netdev_alloc_skb(bp->dev, len + pad);
> +			if (new_skb == NULL) {
>  				DP(NETIF_MSG_RX_ERR,
> -				   "ERROR  flags %x  rx packet %u\n",
> -				   cqe_fp_flags, sw_comp_cons);
> -				fp->eth_q_stats.rx_err_discard_pkt++;
> +				   "ERROR  packet dropped "
> +				   "because of alloc failure\n");
> +				fp->eth_q_stats.rx_skb_alloc_failed++;
>  				goto reuse_rx;
>  			}
> 
> -			/* Since we don't have a jumbo ring
> -			 * copy small packets if mtu > 1500
> -			 */
> -			if ((bp->dev->mtu > ETH_MAX_PACKET_SIZE) &&
> -			    (len <= RX_COPY_THRESH)) {
> -				struct sk_buff *new_skb;
> -
> -				new_skb = netdev_alloc_skb(bp->dev, len + 
pad);
> -				if (new_skb == NULL) {
> -					DP(NETIF_MSG_RX_ERR,
> -					   "ERROR  packet dropped "
> -					   "because of alloc failure\n");
> -					fp->eth_q_stats.rx_skb_alloc_failed++;
> -					goto reuse_rx;
> -				}
> -
> -				/* aligned copy */
> -				skb_copy_from_linear_data_offset(skb, pad,
> -						    new_skb->data + pad, len);
> -				skb_reserve(new_skb, pad);
> -				skb_put(new_skb, len);
> +			/* aligned copy */
> +			skb_copy_from_linear_data_offset(skb, pad,
> +						new_skb->data + pad, len);
> +			skb_reserve(new_skb, pad);
> +			skb_put(new_skb, len);
> 
> -				bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
> +			bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
> 
> -				skb = new_skb;
> +			skb = new_skb;
> 
> -			} else
> -			if (likely(bnx2x_alloc_rx_skb(bp, fp, bd_prod) == 0)) 
{
> -				dma_unmap_single(&bp->pdev->dev,
> -					dma_unmap_addr(rx_buf, mapping),
> -						 fp->rx_buf_size,
> -						 DMA_FROM_DEVICE);
> -				skb_reserve(skb, pad);
> -				skb_put(skb, len);
> +		} else if (likely(bnx2x_alloc_rx_skb(bp, fp, bd_prod) == 0)) {
> +			dma_unmap_single(&bp->pdev->dev,
> +					 dma_unmap_addr(rx_buf, mapping),
> +					 fp->rx_buf_size, DMA_FROM_DEVICE);
> +			skb_reserve(skb, pad);
> +			skb_put(skb, len);
> 
> -			} else {
> -				DP(NETIF_MSG_RX_ERR,
> -				   "ERROR  packet dropped because "
> -				   "of alloc failure\n");
> -				fp->eth_q_stats.rx_skb_alloc_failed++;
> +		} else {
> +			DP(NETIF_MSG_RX_ERR,
> +			   "ERROR  packet dropped because of alloc 
failure\n");
> +			fp->eth_q_stats.rx_skb_alloc_failed++;
>  reuse_rx:
> -				bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
> -				goto next_rx;
> -			}
> -
> -			skb->protocol = eth_type_trans(skb, bp->dev);
> +			bnx2x_reuse_rx_skb(fp, bd_cons, bd_prod);
> +			goto next_rx;
> +		}
> 
> -			/* Set Toeplitz hash for a none-LRO skb */
> -			bnx2x_set_skb_rxhash(bp, cqe, skb);
> +		skb->protocol = eth_type_trans(skb, bp->dev);
> 
> -			skb_checksum_none_assert(skb);
> +		/* Set Toeplitz hash for a none-LRO skb */
> +		bnx2x_set_skb_rxhash(bp, cqe, skb);
> 
> -			if (bp->dev->features & NETIF_F_RXCSUM) {
> +		skb_checksum_none_assert(skb);
> 
> -				if (likely(BNX2X_RX_CSUM_OK(cqe)))
> -					skb->ip_summed = CHECKSUM_UNNECESSARY;
> -				else
> -					fp->eth_q_stats.hw_csum_err++;
> -			}
> +		if (bp->dev->features & NETIF_F_RXCSUM) {
> +			if (likely(BNX2X_RX_CSUM_OK(cqe)))
> +				skb->ip_summed = CHECKSUM_UNNECESSARY;
> +			else
> +				fp->eth_q_stats.hw_csum_err++;
>  		}
> 
>  		skb_record_rx_queue(skb, fp->index);

[PATCH 4/7] bnx2x: simplify TPA sanity check

[Michal Schmidt]

In the TPA branch we already know the CQE type is either START or STOP.
No need to test for that. Even if the type were to differ, we wouldn't
want to suppress the error message.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index f1fea58..fe5be0c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -634,9 +634,7 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
 		if (!CQE_TYPE_FAST(cqe_fp_type)) {
 #ifdef BNX2X_STOP_ON_ERROR
 			/* sanity check */
-			if (fp->disable_tpa &&
-			    (CQE_TYPE_START(cqe_fp_type) ||
-			     CQE_TYPE_STOP(cqe_fp_type)))
+			if (fp->disable_tpa)
 				BNX2X_ERR("START/STOP packet while "
 					  "disable_tpa type %x\n",
 					  CQE_TYPE(cqe_fp_type));
-- 
1.7.6

Re: [PATCH 4/7] bnx2x: simplify TPA sanity check

[Vlad Zolotarov]

On Tuesday 30 August 2011 17:30:43 Michal Schmidt wrote:
> In the TPA branch we already know the CQE type is either START or STOP.
> No need to test for that. Even if the type were to differ, we wouldn't
> want to suppress the error message.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index f1fea58..fe5be0c
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -634,9 +634,7 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
>  		if (!CQE_TYPE_FAST(cqe_fp_type)) {
>  #ifdef BNX2X_STOP_ON_ERROR
>  			/* sanity check */
> -			if (fp->disable_tpa &&
> -			    (CQE_TYPE_START(cqe_fp_type) ||
> -			     CQE_TYPE_STOP(cqe_fp_type)))
> +			if (fp->disable_tpa)
>  				BNX2X_ERR("START/STOP packet while "
>  					  "disable_tpa type %x\n",
>  					  CQE_TYPE(cqe_fp_type));

Acked-by: Vladislav Zolotarov <vladz@broadcom.com>

[PATCH 5/7] bnx2x: do not set TPA flags and features in bnx2x_init_bp

[Michal Schmidt]

The .ndo_{set,fix}_features callbacks are sufficient.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 64314f7..617a072 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -9752,15 +9752,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 					"must load devices in order!\n");
 
 	bp->multi_mode = multi_mode;
-
-	/* Set TPA flags */
-	if (disable_tpa) {
-		bp->flags &= ~TPA_ENABLE_FLAG;
-		bp->dev->features &= ~NETIF_F_LRO;
-	} else {
-		bp->flags |= TPA_ENABLE_FLAG;
-		bp->dev->features |= NETIF_F_LRO;
-	}
 	bp->disable_tpa = disable_tpa;
 
 	if (CHIP_IS_E1(bp))
-- 
1.7.6

Re: [PATCH 5/7] bnx2x: do not set TPA flags and features in bnx2x_init_bp

[Vlad Zolotarov]

On Tuesday 30 August 2011 17:30:44 Michal Schmidt wrote:
> The .ndo_{set,fix}_features callbacks are sufficient.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    9 ---------
>  1 files changed, 0 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index 64314f7..617a072
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -9752,15 +9752,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>  					"must load devices in order!\n");
> 
>  	bp->multi_mode = multi_mode;
> -
> -	/* Set TPA flags */
> -	if (disable_tpa) {
> -		bp->flags &= ~TPA_ENABLE_FLAG;
> -		bp->dev->features &= ~NETIF_F_LRO;
> -	} else {
> -		bp->flags |= TPA_ENABLE_FLAG;
> -		bp->dev->features |= NETIF_F_LRO;
> -	}
>  	bp->disable_tpa = disable_tpa;
> 
>  	if (CHIP_IS_E1(bp))

NACK

This patch will cause the bnx2x to initialize HW with LRO disabled on the 
first ifup because our code considers the TPA_ENABLE_FLAG when desiding on 
whether LRO is enabled or not. ethtool would still report the LRO on though!

thanks,
vlad

Re: [PATCH 5/7] bnx2x: do not set TPA flags and features in bnx2x_init_bp

[Michal Schmidt]

On 08/30/2011 06:21 PM, Vlad Zolotarov wrote:
> NACK
>
> This patch will cause the bnx2x to initialize HW with LRO disabled on the
> first ifup because our code considers the TPA_ENABLE_FLAG when desiding on
> whether LRO is enabled or not. ethtool would still report the LRO on though!

I see. I thought register_netdevice() would always call 
bnx2x_set_features(), but this is not the case.

Patch 7/7 has a similar problem with RX_VLAN_STRIP_FLAG then.

Michal

[PATCH 6/7] bnx2x: move fp->disable_tpa to ->flags

[Michal Schmidt]

Store the boolean fp->disable_tpa in a more general 'flags' field.
Later more flags will be added.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    3 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |   31 ++++++++++------------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h  |    2 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    4 +-
 4 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index c0d2d9c..02fa7a7 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -477,6 +477,8 @@ struct bnx2x_fastpath {
 	u8			cl_qzone_id;
 	u8			fw_sb_id;	/* status block number in FW */
 	u8			igu_sb_id;	/* status block number in HW */
+	u8			flags;
+#define FP_TPA			(1 << 0)	/* TPA enabled */
 
 	u16			rx_bd_prod;
 	u16			rx_bd_cons;
@@ -491,7 +493,6 @@ struct bnx2x_fastpath {
 
 	/* TPA related */
 	struct bnx2x_agg_info	tpa_info[ETH_MAX_AGGREGATION_QUEUES_E1H_E2];
-	u8			disable_tpa;
 #ifdef BNX2X_STOP_ON_ERROR
 	u64			tpa_queue_used;
 #endif
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index fe5be0c..d45aaa5 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -60,15 +60,12 @@ static inline void bnx2x_bz_fp(struct bnx2x *bp, int index)
 
 	/*
 	 * set the tpa flag for each queue. The tpa flag determines the queue
-	 * minimal size so it must be set prior to queue memory allocation
+	 * minimal size so it must be set prior to queue memory allocation.
+	 *
+	 * We don't want TPA on an FCoE L2 ring.
 	 */
-	fp->disable_tpa = ((bp->flags & TPA_ENABLE_FLAG) == 0);
-
-#ifdef BCM_CNIC
-	/* We don't want TPA on an FCoE L2 ring */
-	if (IS_FCOE_FP(fp))
-		fp->disable_tpa = 1;
-#endif
+	if ((bp->flags & TPA_ENABLE_FLAG) && !IS_FCOE_FP(fp))
+		fp->flags = FP_TPA;
 }
 
 /**
@@ -634,9 +631,9 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
 		if (!CQE_TYPE_FAST(cqe_fp_type)) {
 #ifdef BNX2X_STOP_ON_ERROR
 			/* sanity check */
-			if (fp->disable_tpa)
+			if (!(fp->flags & FP_TPA))
 				BNX2X_ERR("START/STOP packet while "
-					  "disable_tpa type %x\n",
+					  "TPA disabled, type %x\n",
 					  CQE_TYPE(cqe_fp_type));
 #endif
 
@@ -993,7 +990,7 @@ void bnx2x_init_rx_rings(struct bnx2x *bp)
 		DP(NETIF_MSG_IFUP,
 		   "mtu %d  rx_buf_size %d\n", bp->dev->mtu, fp->rx_buf_size);
 
-		if (!fp->disable_tpa) {
+		if (fp->flags & FP_TPA) {
 			/* Fill the per-aggregtion pool */
 			for (i = 0; i < max_agg_queues; i++) {
 				struct bnx2x_agg_info *tpa_info =
@@ -1009,7 +1006,7 @@ void bnx2x_init_rx_rings(struct bnx2x *bp)
 						  "disabling TPA on this "
 						  "queue!\n", j);
 					bnx2x_free_tpa_pool(bp, fp, i);
-					fp->disable_tpa = 1;
+					fp->flags &= ~FP_TPA;
 					break;
 				}
 				dma_unmap_addr_set(first_buf, mapping, 0);
@@ -1036,7 +1033,7 @@ void bnx2x_init_rx_rings(struct bnx2x *bp)
 								ring_prod);
 					bnx2x_free_tpa_pool(bp, fp,
 							    max_agg_queues);
-					fp->disable_tpa = 1;
+					fp->flags &= ~FP_TPA;
 					ring_prod = 0;
 					break;
 				}
@@ -1130,7 +1127,7 @@ static void bnx2x_free_rx_skbs(struct bnx2x *bp)
 
 		bnx2x_free_rx_bds(fp);
 
-		if (!fp->disable_tpa)
+		if (fp->flags & FP_TPA)
 			bnx2x_free_tpa_pool(bp, fp, CHIP_IS_E1(bp) ?
 					    ETH_MAX_AGGREGATION_QUEUES_E1 :
 					    ETH_MAX_AGGREGATION_QUEUES_E1H_E2);
@@ -1724,7 +1721,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
 	/*
 	 * Zero fastpath structures preserving invariants like napi, which are
 	 * allocated only once, fp index, max_cos, bp pointer.
-	 * Also set fp->disable_tpa.
+	 * Also set fp->flags.
 	 */
 	for_each_queue(bp, i)
 		bnx2x_bz_fp(bp, i);
@@ -3182,8 +3179,8 @@ alloc_mem_err:
 	 * In these cases we disable the queue
 	 * Min size is different for OOO, TPA and non-TPA queues
 	 */
-	if (ring_size < (fp->disable_tpa ?
-				MIN_RX_SIZE_NONTPA : MIN_RX_SIZE_TPA)) {
+	if (ring_size < ((fp->flags & FP_TPA) ? MIN_RX_SIZE_TPA :
+						MIN_RX_SIZE_NONTPA)) {
 			/* release memory allocated for this queue */
 			bnx2x_free_fp_mem_at(bp, index);
 			return -ENOMEM;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
index 54d50b7..14b3658 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
@@ -1013,7 +1013,7 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp,
 {
 	int i;
 
-	if (fp->disable_tpa)
+	if (!(fp->flags & FP_TPA))
 		return;
 
 	for (i = 0; i < last; i++)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 617a072..7bc6944 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -2709,7 +2709,7 @@ static unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
 	if (IS_FCOE_FP(fp))
 		__set_bit(BNX2X_Q_FLG_FCOE, &flags);
 
-	if (!fp->disable_tpa) {
+	if (fp->flags & FP_TPA) {
 		__set_bit(BNX2X_Q_FLG_TPA, &flags);
 		__set_bit(BNX2X_Q_FLG_TPA_IPV6, &flags);
 	}
@@ -2750,7 +2750,7 @@ static void bnx2x_pf_rx_q_prep(struct bnx2x *bp,
 	u16 sge_sz = 0;
 	u16 tpa_agg_size = 0;
 
-	if (!fp->disable_tpa) {
+	if (fp->flags & FP_TPA) {
 		pause->sge_th_hi = 250;
 		pause->sge_th_lo = 150;
 		tpa_agg_size = min_t(u32,
-- 
1.7.6

[PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michal Schmidt]

Allow disabling of HW RX VLAN stripping with ethtool.

[v2: Store the flag in the fp to ensure that pending packets are
     handled correctly during a switch.]

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    2 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |   29 ++++++++++++++++------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   10 ++++----
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 02fa7a7..e70a208 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -479,6 +479,7 @@ struct bnx2x_fastpath {
 	u8			igu_sb_id;	/* status block number in HW */
 	u8			flags;
 #define FP_TPA			(1 << 0)	/* TPA enabled */
+#define FP_VLAN_STRIP		(1 << 1)	/* RX VLAN headers stripping */
 
 	u16			rx_bd_prod;
 	u16			rx_bd_cons;
@@ -1180,6 +1181,7 @@ struct bnx2x {
 #define NO_MCP_FLAG			(1 << 9)
 
 #define BP_NOMCP(bp)			(bp->flags & NO_MCP_FLAG)
+#define RX_VLAN_STRIP_FLAG		(1 << 10)
 #define MF_FUNC_DIS			(1 << 11)
 #define OWN_CNIC_IRQ			(1 << 12)
 #define NO_ISCSI_OOO_FLAG		(1 << 13)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index d45aaa5..e39ea23 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -66,6 +66,9 @@ static inline void bnx2x_bz_fp(struct bnx2x *bp, int index)
 	 */
 	if ((bp->flags & TPA_ENABLE_FLAG) && !IS_FCOE_FP(fp))
 		fp->flags = FP_TPA;
+
+	if (bp->flags & RX_VLAN_STRIP_FLAG)
+		fp->flags |= FP_VLAN_STRIP;
 }
 
 /**
@@ -359,7 +362,7 @@ static void bnx2x_tpa_start(struct bnx2x_fastpath *fp, u16 queue,
 /**
  * bnx2x_set_lro_mss - calculate the approximate value of the MSS
  *
- * @bp:			driver handle
+ * @fp:			fastpath handle
  * @parsing_flags:	parsing flags from the START CQE
  * @len_on_bd:		total length of the first packet for the
  *			aggregation.
@@ -367,8 +370,8 @@ static void bnx2x_tpa_start(struct bnx2x_fastpath *fp, u16 queue,
  * Approximate value of the MSS for this aggregation calculated using
  * the first packet of it.
  */
-static inline u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
-				    u16 len_on_bd)
+static u16 bnx2x_set_lro_mss(struct bnx2x_fastpath *fp, u16 parsing_flags,
+			     u16 len_on_bd)
 {
 	/*
 	 * TPA arrgregation won't have either IP options or TCP options
@@ -382,6 +385,10 @@ static inline u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
 	else /* IPv4 */
 		hdrs_len += sizeof(struct iphdr);
 
+	/* VLAN header present and not stripped by HW */
+	if ((parsing_flags & PARSING_FLAGS_VLAN) &&
+	    !(fp->flags & FP_VLAN_STRIP))
+		hdrs_len += VLAN_HLEN;
 
 	/* Check if there was a TCP timestamp, if there is it's will
 	 * always be 12 bytes length: nop nop kind length echo val.
@@ -409,9 +416,9 @@ static int bnx2x_fill_frag_skb(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 	frag_size = le16_to_cpu(cqe->pkt_len) - len_on_bd;
 	pages = SGE_PAGE_ALIGN(frag_size) >> SGE_PAGE_SHIFT;
 
-	/* This is needed in order to enable forwarding support */
+	/* Doing LRO, let TCP know the receive MSS */
 	if (frag_size)
-		skb_shinfo(skb)->gso_size = bnx2x_set_lro_mss(bp,
+		skb_shinfo(skb)->gso_size = bnx2x_set_lro_mss(fp,
 					tpa_info->parsing_flags, len_on_bd);
 
 #ifdef BNX2X_STOP_ON_ERROR
@@ -511,7 +518,8 @@ static void bnx2x_tpa_stop(struct bnx2x *bp, struct bnx2x_fastpath *fp,
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 		if (!bnx2x_fill_frag_skb(bp, fp, queue, skb, cqe, cqe_idx)) {
-			if (tpa_info->parsing_flags & PARSING_FLAGS_VLAN)
+			if ((tpa_info->parsing_flags & PARSING_FLAGS_VLAN) &&
+			    (fp->flags & FP_VLAN_STRIP))
 				__vlan_hwaccel_put_tag(skb, tpa_info->vlan_tag);
 			napi_gro_receive(&fp->napi, skb);
 		} else {
@@ -742,8 +750,8 @@ reuse_rx:
 
 		skb_record_rx_queue(skb, fp->index);
 
-		if (le16_to_cpu(cqe_fp->pars_flags.flags) &
-		    PARSING_FLAGS_VLAN)
+		if ((le16_to_cpu(cqe_fp->pars_flags.flags) &
+		    PARSING_FLAGS_VLAN) && (fp->flags & FP_VLAN_STRIP))
 			__vlan_hwaccel_put_tag(skb,
 					       le16_to_cpu(cqe_fp->vlan_tag));
 		napi_gro_receive(&fp->napi, skb);
@@ -3415,6 +3423,11 @@ int bnx2x_set_features(struct net_device *dev, u32 features)
 	else
 		flags &= ~TPA_ENABLE_FLAG;
 
+	if (features & NETIF_F_HW_VLAN_RX)
+		flags |= RX_VLAN_STRIP_FLAG;
+	else
+		flags &= ~RX_VLAN_STRIP_FLAG;
+
 	if (features & NETIF_F_LOOPBACK) {
 		if (bp->link_params.loopback_mode != LOOPBACK_BMAC) {
 			bp->link_params.loopback_mode = LOOPBACK_BMAC;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 7bc6944..15624bc 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -2719,9 +2719,8 @@ static unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
 		__set_bit(BNX2X_Q_FLG_MCAST, &flags);
 	}
 
-	/* Always set HW VLAN stripping */
-	__set_bit(BNX2X_Q_FLG_VLAN, &flags);
-
+	if (fp->flags & FP_VLAN_STRIP)
+		__set_bit(BNX2X_Q_FLG_VLAN, &flags);
 
 	return flags | bnx2x_get_common_flags(bp, fp, true);
 }
@@ -10262,12 +10261,13 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 
 	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_LRO |
-		NETIF_F_RXCSUM | NETIF_F_RXHASH | NETIF_F_HW_VLAN_TX;
+		NETIF_F_RXCSUM | NETIF_F_RXHASH |
+		NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
 
 	dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
 
-	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
+	dev->features |= dev->hw_features;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
 
-- 
1.7.6

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michał Mirosław]

2011/8/30 Michal Schmidt <mschmidt@redhat.com>:
> Allow disabling of HW RX VLAN stripping with ethtool.
>
> [v2: Store the flag in the fp to ensure that pending packets are
>     handled correctly during a switch.]
[...]
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -66,6 +66,9 @@ static inline void bnx2x_bz_fp(struct bnx2x *bp, int index)
>         */
>        if ((bp->flags & TPA_ENABLE_FLAG) && !IS_FCOE_FP(fp))
>                fp->flags = FP_TPA;
> +
> +       if (bp->flags & RX_VLAN_STRIP_FLAG)
> +               fp->flags |= FP_VLAN_STRIP;
>  }
[...]
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -2719,9 +2719,8 @@ static unsigned long bnx2x_get_q_flags(struct bnx2x *bp,
>                __set_bit(BNX2X_Q_FLG_MCAST, &flags);
>        }
>
> -       /* Always set HW VLAN stripping */
> -       __set_bit(BNX2X_Q_FLG_VLAN, &flags);
> -
> +       if (fp->flags & FP_VLAN_STRIP)
> +               __set_bit(BNX2X_Q_FLG_VLAN, &flags);
>
>        return flags | bnx2x_get_common_flags(bp, fp, true);
>  }


It seems rather convoluted and unnecessary that you mirror
NETIF_F_HW_VLAN_RX in bp->flags and then also in fp->flags. Are the
fp->flags strictly mirroring hardware state (as in: there is no way
the states can differ in any point in time where the flags are
tested)? For this to be true, the two functions above need to be
called only without releasing a lock between them that is also taken
by receive handler. Isn't there a flag in the rx descriptor of a
packet that says if VLAN was stripped? (All that flag keeping would be
unnecessary then.)

Best Regards,
Michał Mirosław

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michal Schmidt]

On 08/30/2011 08:27 PM, Michał Mirosław wrote:
> It seems rather convoluted and unnecessary that you mirror
> NETIF_F_HW_VLAN_RX in bp->flags

This mirroring is for the benefit of functions called indirectly from 
bnx2x_set_features(). They cannot look at dev->features for 
NETIF_F_HW_VLAN_RX because it's not set before ->ndo_set_features() returns.

> and then also in fp->flags.  Are the
> fp->flags strictly mirroring hardware state (as in: there is no way
> the states can differ in any point in time where the flags are
> tested)?

Yes. This is the purpose of the second mirroring of the flag.

> For this to be true, the two functions above need to be
> called only without releasing a lock between them that is also taken
> by receive handler.

The flag propagates from bp->flags to fp->flags between unloading and 
reloading the NIC. The receive handler cannot run at the time.

> Isn't there a flag in the rx descriptor of a
> packet that says if VLAN was stripped? (All that flag keeping would be
> unnecessary then.)

There is no such flag, AFAIK.

Michal

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michał Mirosław]

W dniu 30 sierpnia 2011 21:30 użytkownik Michal Schmidt
<mschmidt@redhat.com> napisał:
> On 08/30/2011 08:27 PM, Michał Mirosław wrote:
>> It seems rather convoluted and unnecessary that you mirror
>> NETIF_F_HW_VLAN_RX in bp->flags
> This mirroring is for the benefit of functions called indirectly from
> bnx2x_set_features(). They cannot look at dev->features for
> NETIF_F_HW_VLAN_RX because it's not set before ->ndo_set_features() returns.

You can cheat in ndo_set_features in this case. Just set/clear
NETIF_F_HW_VLAN_RX bit in dev->features before you call those
functions (but please leave a comment there explaining why!). This is
allowed because if ndo_set_features fails it needs to fix
dev->features itself, and if it succeeds dev->features will be
replaced with exactly what was passed to ndo_set_features. So, unless
you need to check whether the VLAN stripping state changed in those
functions, then you can get rid of this copy.

>> and then also in fp->flags.  Are the
>> fp->flags strictly mirroring hardware state (as in: there is no way
>> the states can differ in any point in time where the flags are
>> tested)?
> Yes. This is the purpose of the second mirroring of the flag.
>> For this to be true, the two functions above need to be
>> called only without releasing a lock between them that is also taken
>> by receive handler.
> The flag propagates from bp->flags to fp->flags between unloading and
> reloading the NIC. The receive handler cannot run at the time.
>> Isn't there a flag in the rx descriptor of a
>> packet that says if VLAN was stripped? (All that flag keeping would be
>> unnecessary then.)
> There is no such flag, AFAIK.

Thanks for explanation!

Best Regards,
Michał Mirosław

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Vlad Zolotarov]

On Tuesday 30 August 2011 22:30:11 Michal Schmidt wrote:

> > and then also in fp->flags.  Are the
> > fp->flags strictly mirroring hardware state (as in: there is no way
> > the states can differ in any point in time where the flags are
> > tested)?
> 
> Yes. This is the purpose of the second mirroring of the flag.

Michal,
although the above is true i'd say it's a bit of an overkill:
RX VLAN stripping is configured in a function level, so keeping it in a per 
queue level is not needed. 

The problem u were trying to resolve (and u resolved it) was to separate the 
RX_VLAN_ENABLED flag semantics into two: requested feature and HW configured 
feature in order to further check the second in the fast path, while setting 
the first one in the set_features(). Then the second lag is updated according 
to the first one during the loading of the function (bnx2x_nic_load()).

Therefore it would be enough to just add those two flags in the function (bp) 
level keeping the rest of your patch as it is. This would also cancel the need 
for a patch 6.

Thanks,
vlad

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michal Schmidt]

On Wed, 31 Aug 2011 15:01:39 +0300 Vlad Zolotarov wrote:
> On Tuesday 30 August 2011 22:30:11 Michal Schmidt wrote:
> 
> > > and then also in fp->flags.  Are the
> > > fp->flags strictly mirroring hardware state (as in: there is no
> > > way the states can differ in any point in time where the flags are
> > > tested)?
> > 
> > Yes. This is the purpose of the second mirroring of the flag.
> 
> Michal,
> although the above is true i'd say it's a bit of an overkill:
> RX VLAN stripping is configured in a function level, so keeping it in
> a per queue level is not needed. 
> 
> The problem u were trying to resolve (and u resolved it) was to
> separate the RX_VLAN_ENABLED flag semantics into two: requested
> feature and HW configured feature in order to further check the
> second in the fast path, while setting the first one in the
> set_features(). Then the second lag is updated according to the first
> one during the loading of the function (bnx2x_nic_load()).
> 
> Therefore it would be enough to just add those two flags in the
> function (bp) level keeping the rest of your patch as it is. This
> would also cancel the need for a patch 6.

Alright, I will remove the per-fp flag and move it to bp.

I will also apply the cheat suggested by Michał, so that:
 - dev->features & NETIF_F_HW_VLAN_RX is the requested state
 - bp->flags & RX_VLAN_STRIP_FLAG is the HW configured state
=> Only one new bp flag needed, not two.

BTW, Michał's cheat should apply to TPA_ENABLE_FLAG as well - it only
mirrors NETIF_F_LRO. But for TPA the HW configured state is really
per-queue (fp->disable_tpa). I will remove TPA_ENABLE_FLAG unless you
object to Michał's "cheat".

Thanks,
Michal

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Vlad Zolotarov]

On Wednesday 31 August 2011 16:53:24 Michal Schmidt wrote:
> On Wed, 31 Aug 2011 15:01:39 +0300 Vlad Zolotarov wrote:
> > On Tuesday 30 August 2011 22:30:11 Michal Schmidt wrote:
> > > > and then also in fp->flags.  Are the
> > > > fp->flags strictly mirroring hardware state (as in: there is no
> > > > way the states can differ in any point in time where the flags are
> > > > tested)?
> > > 
> > > Yes. This is the purpose of the second mirroring of the flag.
> > 
> > Michal,
> > although the above is true i'd say it's a bit of an overkill:
> > RX VLAN stripping is configured in a function level, so keeping it in
> > a per queue level is not needed.
> > 
> > The problem u were trying to resolve (and u resolved it) was to
> > separate the RX_VLAN_ENABLED flag semantics into two: requested
> > feature and HW configured feature in order to further check the
> > second in the fast path, while setting the first one in the
> > set_features(). Then the second lag is updated according to the first
> > one during the loading of the function (bnx2x_nic_load()).
> > 
> > Therefore it would be enough to just add those two flags in the
> > function (bp) level keeping the rest of your patch as it is. This
> > would also cancel the need for a patch 6.
> 
> Alright, I will remove the per-fp flag and move it to bp.
> 
> I will also apply the cheat suggested by Michał, so that:
>  - dev->features & NETIF_F_HW_VLAN_RX is the requested state
>  - bp->flags & RX_VLAN_STRIP_FLAG is the HW configured state
> => Only one new bp flag needed, not two.
> 
> BTW, Michał's cheat should apply to TPA_ENABLE_FLAG as well - it only
> mirrors NETIF_F_LRO. But for TPA the HW configured state is really
> per-queue (fp->disable_tpa). I will remove TPA_ENABLE_FLAG unless you
> object to Michał's "cheat".

I agree the TPA_ENABLE_FLAG may and should be removed.
However I didn’t like the idea of relying on the current implementation 
of the calling function (__netdev_update_features()). If u want to change 
the implementation the way we rely on the dev->features in the 
ndo_set_features() flow, which is a semantics change, u'd rather change 
the __netdev_update_features() so that it sets the dev->features before 
ndo_set_features() call and restores it in case of a failure. Something like this:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0a7f619..a5f6d3e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -943,8 +943,7 @@ struct net_device_ops {
                                                 struct net_device *slave_dev);
        u32                     (*ndo_fix_features)(struct net_device *dev,
                                                    u32 features);
-       int                     (*ndo_set_features)(struct net_device *dev,
-                                                   u32 features);
+       int                     (*ndo_set_features)(struct net_device *dev);
 };
 
 /*
diff --git a/net/core/dev.c b/net/core/dev.c
index b2e262e..474e539 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5321,7 +5321,7 @@ static u32 netdev_fix_features(struct net_device *dev, u32 features)
 
 int __netdev_update_features(struct net_device *dev)
 {
-       u32 features;
+       u32 features, old_features;
        int err = 0;
 
        ASSERT_RTNL();
@@ -5340,19 +5340,23 @@ int __netdev_update_features(struct net_device *dev)
        netdev_dbg(dev, "Features changed: 0x%08x -> 0x%08x\n",
                dev->features, features);
 
+       /* Remember the original features and set the new ones */
+       old_features = dev->features;
+       dev->features = features;
+
        if (dev->netdev_ops->ndo_set_features)
-               err = dev->netdev_ops->ndo_set_features(dev, features);
+               err = dev->netdev_ops->ndo_set_features(dev);
 
        if (unlikely(err < 0)) {
+               /* Restore the original features */
+               dev->features = old_features;
+
                netdev_err(dev,
                        "set_features() failed (%d); wanted 0x%08x, left 0x%08x\n",
                        err, features, dev->features);
                return -1;
        }
 
-       if (!err)
-               dev->features = features;
-
        return 1;
 }

However, this would involve updating all other L2 drivers that implement ndo_set_features().

Pls., comment.

thanks,
vlad

> 
> Thanks,
> Michal

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michal Schmidt]

On Wed, 31 Aug 2011 18:07:53 +0300 Vlad Zolotarov wrote:
> If u want to change the implementation the way we rely on the
> dev->features in the ndo_set_features() flow, which is a semantics
> change, u'd rather change the __netdev_update_features() so that it
> sets the dev->features before ndo_set_features() call and restores it
> in case of a failure. Something like this:

...
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b2e262e..474e539 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5321,7 +5321,7 @@ static u32 netdev_fix_features(struct
> net_device *dev, u32 features) 
>  int __netdev_update_features(struct net_device *dev)
>  {
> -       u32 features;
> +       u32 features, old_features;
>         int err = 0;
>  
>         ASSERT_RTNL();
> @@ -5340,19 +5340,23 @@ int __netdev_update_features(struct
> net_device *dev) netdev_dbg(dev, "Features changed: 0x%08x ->
> 0x%08x\n", dev->features, features);
>  
> +       /* Remember the original features and set the new ones */
> +       old_features = dev->features;
> +       dev->features = features;
> +
>         if (dev->netdev_ops->ndo_set_features)
> -               err = dev->netdev_ops->ndo_set_features(dev,
> features);
> +               err = dev->netdev_ops->ndo_set_features(dev);

Drivers want to know which features changed. They compare
the features argument with dev->features.
Perhaps we could pass old_features as the argument. Then we'd better
change the name of the callback to avoid confusion.

I think it's not worth changing. I could restore dev->features before
returning if bnx2x_reload_if_running() fails.

Michal

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michal Schmidt]

On Wed, 31 Aug 2011 17:37:49 +0200 Michal Schmidt wrote:
> I could restore dev->features before
> returning if bnx2x_reload_if_running() fails.

Or even safer - restore them always:
	...
	u32 orig_features = dev->features;
	dev->features = features;
        ret = bnx2x_reload_if_running(dev);
        dev->features = orig_features;
        return ret;
	...
This way we don't have to assume anything about
__netdev_update_features().

Michal

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Vlad Zolotarov]

On Wednesday 31 August 2011 18:51:20 Michal Schmidt wrote:
> On Wed, 31 Aug 2011 17:37:49 +0200 Michal Schmidt wrote:
> > I could restore dev->features before
> > returning if bnx2x_reload_if_running() fails.
> 
> Or even safer - restore them always:
> 	...
> 	u32 orig_features = dev->features;
> 	dev->features = features;
>         ret = bnx2x_reload_if_running(dev);
>         dev->features = orig_features;
>         return ret;
> 	...
> This way we don't have to assume anything about
> __netdev_update_features().

I agree - it's the best choice if we go for a bnx2x-only solution.

thanks,
vlad

> 
> Michal
> --
> 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

Re: [PATCH 7/7] bnx2x: expose HW RX VLAN stripping toggle

[Michał Mirosław]

2011/8/31 Michal Schmidt <mschmidt@redhat.com>:
> On Wed, 31 Aug 2011 17:37:49 +0200 Michal Schmidt wrote:
>> I could restore dev->features before
>> returning if bnx2x_reload_if_running() fails.
>
> Or even safer - restore them always:
>        ...
>        u32 orig_features = dev->features;
>        dev->features = features;
>        ret = bnx2x_reload_if_running(dev);
>        dev->features = orig_features;
>        return ret;
>        ...
> This way we don't have to assume anything about
> __netdev_update_features().

The assignment in __netdev_update_features() is just to save copying
that assignment all over the drivers' code. This won't change unless
someone wants to break^Wchange the design.

Best Regards,
Michał Mirosław