[PATCH 0/3 net-next] bnx2x: VLAN stripping toggle and a small cleanup

[PATCH 0/3 net-next] bnx2x: VLAN stripping toggle and a small cleanup

[Michal Schmidt]

Hello,
a couple of cleanups and the addition of the VLAN stripping toggle, updated
according to Vlad's and Michał's feedback.

Michal Schmidt (3):
  bnx2x: remove TPA_ENABLE_FLAG
  bnx2x: do not set NETIF_F_LRO in bnx2x_init_bp
  bnx2x: expose HW RX VLAN stripping toggle

 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    2 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |   42 +++++++++++++--------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   19 +++-------
 3 files changed, 32 insertions(+), 31 deletions(-)

-- 
1.7.6

[PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Michal Schmidt]

TPA_ENABLE_FLAG is unnecessary, because it mirrors NETIF_F_LRO.

The "cheat" in bnx2x_set_features() was suggested by Michał Mirosław.

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

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 735e491..5d5f323 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1174,7 +1174,7 @@ struct bnx2x {
 #define USING_MSIX_FLAG			(1 << 5)
 #define USING_MSI_FLAG			(1 << 6)
 #define DISABLE_MSI_FLAG		(1 << 7)
-#define TPA_ENABLE_FLAG			(1 << 8)
+
 #define NO_MCP_FLAG			(1 << 9)
 
 #define BP_NOMCP(bp)			(bp->flags & NO_MCP_FLAG)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index a08b101..c660317 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -62,7 +62,7 @@ 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
 	 */
-	fp->disable_tpa = ((bp->flags & TPA_ENABLE_FLAG) == 0);
+	fp->disable_tpa = !(bp->dev->features & NETIF_F_LRO);
 
 #ifdef BCM_CNIC
 	/* We don't want TPA on an FCoE L2 ring */
@@ -3410,13 +3410,10 @@ u32 bnx2x_fix_features(struct net_device *dev, u32 features)
 int bnx2x_set_features(struct net_device *dev, u32 features)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	u32 flags = bp->flags;
 	bool bnx2x_reload = false;
 
-	if (features & NETIF_F_LRO)
-		flags |= TPA_ENABLE_FLAG;
-	else
-		flags &= ~TPA_ENABLE_FLAG;
+	if ((features ^ dev->features) & NETIF_F_LRO)
+		bnx2x_reload = true;
 
 	if (features & NETIF_F_LOOPBACK) {
 		if (bp->link_params.loopback_mode != LOOPBACK_BMAC) {
@@ -3430,14 +3427,17 @@ int bnx2x_set_features(struct net_device *dev, u32 features)
 		}
 	}
 
-	if (flags ^ bp->flags) {
-		bp->flags = flags;
-		bnx2x_reload = true;
-	}
-
 	if (bnx2x_reload) {
-		if (bp->recovery_state == BNX2X_RECOVERY_DONE)
+		if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
+			/*
+			 * Cheat! Normally dev->features will be set after we
+			 * return, but that's too late. We need to know how to
+			 * configure the NIC when reloading it, so update
+			 * the features early.
+			 */
+			dev->features = features;
 			return bnx2x_reload_if_running(dev);
+		}
 		/* else: bnx2x_nic_load() will be called at end of recovery */
 	}
 
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index e7b584b..fd32c04 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -9757,13 +9757,10 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 	bp->multi_mode = multi_mode;
 
 	/* Set TPA flags */
-	if (disable_tpa) {
-		bp->flags &= ~TPA_ENABLE_FLAG;
+	if (disable_tpa)
 		bp->dev->features &= ~NETIF_F_LRO;
-	} else {
-		bp->flags |= TPA_ENABLE_FLAG;
+	else
 		bp->dev->features |= NETIF_F_LRO;
-	}
 	bp->disable_tpa = disable_tpa;
 
 	if (CHIP_IS_E1(bp))
-- 
1.7.6

[PATCH 2/3] bnx2x: do not set NETIF_F_LRO in bnx2x_init_bp

[Michal Schmidt]

bnx2x_fix_features() will take care of clearing NETIF_F_LRO if
'disable_tpa' is used.

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

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index fd32c04..c1285db 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -9755,12 +9755,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->dev->features &= ~NETIF_F_LRO;
-	else
-		bp->dev->features |= NETIF_F_LRO;
 	bp->disable_tpa = disable_tpa;
 
 	if (CHIP_IS_E1(bp))
-- 
1.7.6

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

[Michal Schmidt]

Allow disabling of HW RX VLAN stripping with ethtool.

[v3: per-queue flag was overkill, store the HW config in bp.
     Suggestions by Vlad Zolotarov and Michał Mirosław.]

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

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 5d5f323..b85017e 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1174,7 +1174,7 @@ struct bnx2x {
 #define USING_MSIX_FLAG			(1 << 5)
 #define USING_MSI_FLAG			(1 << 6)
 #define DISABLE_MSI_FLAG		(1 << 7)
-
+#define RX_VLAN_STRIP_FLAG		(1 << 8)
 #define NO_MCP_FLAG			(1 << 9)
 
 #define BP_NOMCP(bp)			(bp->flags & NO_MCP_FLAG)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index c660317..6e1b4b4 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -385,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) &&
+	    !(bp->flags & RX_VLAN_STRIP_FLAG))
+		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.
@@ -412,7 +416,7 @@ 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,
 					tpa_info->parsing_flags, len_on_bd);
@@ -514,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) &&
+			    (bp->flags & RX_VLAN_STRIP_FLAG))
 				__vlan_hwaccel_put_tag(skb, tpa_info->vlan_tag);
 			napi_gro_receive(&fp->napi, skb);
 		} else {
@@ -745,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) && (bp->flags & RX_VLAN_STRIP_FLAG))
 			__vlan_hwaccel_put_tag(skb,
 					       le16_to_cpu(cqe_fp->vlan_tag));
 		napi_gro_receive(&fp->napi, skb);
@@ -1711,6 +1716,11 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
 
 	bp->state = BNX2X_STATE_OPENING_WAIT4_LOAD;
 
+	if (bp->dev->features & NETIF_F_HW_VLAN_RX)
+		bp->flags |= RX_VLAN_STRIP_FLAG;
+	else
+		bp->flags &= ~RX_VLAN_STRIP_FLAG;
+
 	/* Set the initial link reported state to link down */
 	bnx2x_acquire_phy_lock(bp);
 	memset(&bp->last_reported_link, 0, sizeof(bp->last_reported_link));
@@ -3412,7 +3422,7 @@ int bnx2x_set_features(struct net_device *dev, u32 features)
 	struct bnx2x *bp = netdev_priv(dev);
 	bool bnx2x_reload = false;
 
-	if ((features ^ dev->features) & NETIF_F_LRO)
+	if ((features ^ dev->features) & (NETIF_F_LRO | NETIF_F_HW_VLAN_RX))
 		bnx2x_reload = true;
 
 	if (features & NETIF_F_LOOPBACK) {
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index c1285db..e61be4e 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -2720,9 +2720,8 @@ static inline 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 (bp->flags & RX_VLAN_STRIP_FLAG)
+		__set_bit(BNX2X_Q_FLG_VLAN, &flags);
 
 	return flags | bnx2x_get_common_flags(bp, fp, true);
 }
@@ -10265,12 +10264,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 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Vlad Zolotarov]

On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:
>  	if (bnx2x_reload) {
> -		if (bp->recovery_state == BNX2X_RECOVERY_DONE)
> +		if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
> +			/*
> +			 * Cheat! Normally dev->features will be set after we
> +			 * return, but that's too late. We need to know how to
> +			 * configure the NIC when reloading it, so update
> +			 * the features early.
> +			 */
> +			dev->features = features;
>  			return bnx2x_reload_if_running(dev);

NACK 

This is bogus - what if bnx2x_reload_if_running(dev) (bnx2x_nic_load()) 
failes? The original dev->features would be lost... Pls., see my latest 
response on your previouse patch series thread.

thanks,
vlad

> +		}
>  		/* else: bnx2x_nic_load() will be called at end of recovery */
>  	}
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index e7b584b..fd32c04
> 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -9757,13 +9757,10 @@ static int __devinit bnx2x_init_bp(struct bnx2x
> *bp) bp->multi_mode = multi_mode;
> 
>  	/* Set TPA flags */
> -	if (disable_tpa) {
> -		bp->flags &= ~TPA_ENABLE_FLAG;
> +	if (disable_tpa)
>  		bp->dev->features &= ~NETIF_F_LRO;
> -	} else {
> -		bp->flags |= TPA_ENABLE_FLAG;
> +	else
>  		bp->dev->features |= NETIF_F_LRO;
> -	}
>  	bp->disable_tpa = disable_tpa;
> 
>  	if (CHIP_IS_E1(bp))

Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Michal Schmidt]

On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:
> On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:
> >  	if (bnx2x_reload) {
> > -		if (bp->recovery_state == BNX2X_RECOVERY_DONE)
> > +		if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
> > +			/*
> > +			 * Cheat! Normally dev->features will be
> > set after we
> > +			 * return, but that's too late. We need to
> > know how to
> > +			 * configure the NIC when reloading it, so
> > update
> > +			 * the features early.
> > +			 */
> > +			dev->features = features;
> >  			return bnx2x_reload_if_running(dev);
> 
> NACK 
> 
> This is bogus - what if bnx2x_reload_if_running(dev)
> (bnx2x_nic_load()) failes? The original dev->features would be
> lost... 

Well, yes, but since the NIC would be now not working, do we really
care about its features? :-)

Michal

RE: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Vladislav Zolotarov]

> -----Original Message-----
> From: Michal Schmidt [mailto:mschmidt@redhat.com]
> Sent: Wednesday, August 31, 2011 6:59 PM
> To: Vladislav Zolotarov
> Cc: netdev@vger.kernel.org; Dmitry Kravkov; Eilon Greenstein;
> mirqus@gmail.com
> Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG
> 
> On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:
> > On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:
> > >  	if (bnx2x_reload) {
> > > -		if (bp->recovery_state == BNX2X_RECOVERY_DONE)
> > > +		if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
> > > +			/*
> > > +			 * Cheat! Normally dev->features will be
> > > set after we
> > > +			 * return, but that's too late. We need to
> > > know how to
> > > +			 * configure the NIC when reloading it, so
> > > update
> > > +			 * the features early.
> > > +			 */
> > > +			dev->features = features;
> > >  			return bnx2x_reload_if_running(dev);
> >
> > NACK
> >
> > This is bogus - what if bnx2x_reload_if_running(dev)
> > (bnx2x_nic_load()) failes? The original dev->features would be
> > lost...
> 
> Well, yes, but since the NIC would be now not working, do we really
> care about its features? :-)

U r kidding, right? ;)
We care about the consistency in the netdev features state - if we failed 
to configure the requested feature and returned an error on e.g. "ethtool -K ethX lro on"
call, it's expected that a subsequent ethtool -k ethX call won't report the requested
feature (LRO) as set.

Thanks,
vlad

> 
> Michal

Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Michal Schmidt]

On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:
> This is bogus - what if bnx2x_reload_if_running(dev)
> (bnx2x_nic_load()) failes? The original dev->features would be
> lost... Pls., see my latest response on your previouse patch series
> thread.

OK, so the conclusion in the other thread was to restore the features
before returning. Here's an updated patch.


Subject: [PATCH] bnx2x: remove TPA_ENABLE_FLAG

TPA_ENABLE_FLAG is unnecessary, because it mirrors NETIF_F_LRO.

"Cheating" in bnx2x_set_features() was suggested by Michał Mirosław.

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

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 735e491..5d5f323 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1174,7 +1174,7 @@ struct bnx2x {
 #define USING_MSIX_FLAG			(1 << 5)
 #define USING_MSI_FLAG			(1 << 6)
 #define DISABLE_MSI_FLAG		(1 << 7)
-#define TPA_ENABLE_FLAG			(1 << 8)
+
 #define NO_MCP_FLAG			(1 << 9)
 
 #define BP_NOMCP(bp)			(bp->flags & NO_MCP_FLAG)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index a08b101..399e71c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -62,7 +62,7 @@ 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
 	 */
-	fp->disable_tpa = ((bp->flags & TPA_ENABLE_FLAG) == 0);
+	fp->disable_tpa = !(bp->dev->features & NETIF_F_LRO);
 
 #ifdef BCM_CNIC
 	/* We don't want TPA on an FCoE L2 ring */
@@ -3410,13 +3410,10 @@ u32 bnx2x_fix_features(struct net_device *dev, u32 features)
 int bnx2x_set_features(struct net_device *dev, u32 features)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	u32 flags = bp->flags;
 	bool bnx2x_reload = false;
 
-	if (features & NETIF_F_LRO)
-		flags |= TPA_ENABLE_FLAG;
-	else
-		flags &= ~TPA_ENABLE_FLAG;
+	if ((features ^ dev->features) & NETIF_F_LRO)
+		bnx2x_reload = true;
 
 	if (features & NETIF_F_LOOPBACK) {
 		if (bp->link_params.loopback_mode != LOOPBACK_BMAC) {
@@ -3430,14 +3427,22 @@ int bnx2x_set_features(struct net_device *dev, u32 features)
 		}
 	}
 
-	if (flags ^ bp->flags) {
-		bp->flags = flags;
-		bnx2x_reload = true;
-	}
-
 	if (bnx2x_reload) {
-		if (bp->recovery_state == BNX2X_RECOVERY_DONE)
-			return bnx2x_reload_if_running(dev);
+		if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
+			/*
+			 * Cheat! Normally dev->features will be set after we
+			 * return, but that's too late. We need to know how to
+			 * configure the NIC when reloading it, so copy the
+			 * features beforehand. Restore them after reload to
+			 * avoid any possible confusion of the caller.
+			 */
+			int ret;
+			u32 orig_features = dev->features;
+			dev->features = features;
+			ret = bnx2x_reload_if_running(dev);
+			dev->features = orig_features;
+			return ret;
+		}
 		/* else: bnx2x_nic_load() will be called at end of recovery */
 	}
 
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index e7b584b..fd32c04 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -9757,13 +9757,10 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
 	bp->multi_mode = multi_mode;
 
 	/* Set TPA flags */
-	if (disable_tpa) {
-		bp->flags &= ~TPA_ENABLE_FLAG;
+	if (disable_tpa)
 		bp->dev->features &= ~NETIF_F_LRO;
-	} else {
-		bp->flags |= TPA_ENABLE_FLAG;
+	else
 		bp->dev->features |= NETIF_F_LRO;
-	}
 	bp->disable_tpa = disable_tpa;
 
 	if (CHIP_IS_E1(bp))
-- 
1.7.6

Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Michał Mirosław]

2011/8/31 Vladislav Zolotarov <vladz@broadcom.com>:
>> -----Original Message-----
>> From: Michal Schmidt [mailto:mschmidt@redhat.com]
>> Sent: Wednesday, August 31, 2011 6:59 PM
>> To: Vladislav Zolotarov
>> Cc: netdev@vger.kernel.org; Dmitry Kravkov; Eilon Greenstein;
>> mirqus@gmail.com
>> Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG
>>
>> On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:
>> > On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:
>> > >   if (bnx2x_reload) {
>> > > -         if (bp->recovery_state == BNX2X_RECOVERY_DONE)
>> > > +         if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
>> > > +                 /*
>> > > +                  * Cheat! Normally dev->features will be
>> > > set after we
>> > > +                  * return, but that's too late. We need to
>> > > know how to
>> > > +                  * configure the NIC when reloading it, so
>> > > update
>> > > +                  * the features early.
>> > > +                  */
>> > > +                 dev->features = features;
>> > >                   return bnx2x_reload_if_running(dev);
>> >
>> > NACK
>> >
>> > This is bogus - what if bnx2x_reload_if_running(dev)
>> > (bnx2x_nic_load()) failes? The original dev->features would be
>> > lost...
>>
>> Well, yes, but since the NIC would be now not working, do we really
>> care about its features? :-)
>
> U r kidding, right? ;)
> We care about the consistency in the netdev features state - if we failed
> to configure the requested feature and returned an error on e.g. "ethtool -K ethX lro on"
> call, it's expected that a subsequent ethtool -k ethX call won't report the requested
> feature (LRO) as set.

If bnx2x_reload_if_running() failure means that NIC is disabled, then
Michal is right that there's no point in caring about dev->features,
sice 'load' operation (NIC configuration) needs to be done again
anyway.

Best Regards,
Michał Mirosław

RE: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Vladislav Zolotarov]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Michal Miroslaw
> Sent: Wednesday, August 31, 2011 9:05 PM
> To: Vladislav Zolotarov
> Cc: Michal Schmidt; netdev@vger.kernel.org; Dmitry Kravkov; Eilon
> Greenstein
> Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG
> 
> 2011/8/31 Vladislav Zolotarov <vladz@broadcom.com>:
> >> -----Original Message-----
> >> From: Michal Schmidt [mailto:mschmidt@redhat.com]
> >> Sent: Wednesday, August 31, 2011 6:59 PM
> >> To: Vladislav Zolotarov
> >> Cc: netdev@vger.kernel.org; Dmitry Kravkov; Eilon Greenstein;
> >> mirqus@gmail.com
> >> Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG
> >>
> >> On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:
> >> > On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:
> >> > >   if (bnx2x_reload) {
> >> > > -         if (bp->recovery_state == BNX2X_RECOVERY_DONE)
> >> > > +         if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
> >> > > +                 /*
> >> > > +                  * Cheat! Normally dev->features will be
> >> > > set after we
> >> > > +                  * return, but that's too late. We need to
> >> > > know how to
> >> > > +                  * configure the NIC when reloading it, so
> >> > > update
> >> > > +                  * the features early.
> >> > > +                  */
> >> > > +                 dev->features = features;
> >> > >                   return bnx2x_reload_if_running(dev);
> >> >
> >> > NACK
> >> >
> >> > This is bogus - what if bnx2x_reload_if_running(dev)
> >> > (bnx2x_nic_load()) failes? The original dev->features would be
> >> > lost...
> >>
> >> Well, yes, but since the NIC would be now not working, do we really
> >> care about its features? :-)
> >
> > U r kidding, right? ;)
> > We care about the consistency in the netdev features state - if we
> failed
> > to configure the requested feature and returned an error on e.g.
> "ethtool -K ethX lro on"
> > call, it's expected that a subsequent ethtool -k ethX call won't
> report the requested
> > feature (LRO) as set.
> 
> If bnx2x_reload_if_running() failure means that NIC is disabled, then
> Michal is right that there's no point in caring about dev->features,
> sice 'load' operation (NIC configuration) needs to be done again
> anyway.

Michal, it's a matter of a consistent semantics/behavior of the ethtool callbacks just as I described above.
As long as dev->features may be queried both when device is down I'm afraid I can't agree with u.

Thanks,
vlad

> 
> Best Regards,
> Michał Mirosław
> --
> 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 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Michał Mirosław]

W dniu 1 września 2011 10:37 użytkownik Vladislav Zolotarov
<vladz@broadcom.com> napisał:
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Michal Miroslaw
>> Sent: Wednesday, August 31, 2011 9:05 PM
>> To: Vladislav Zolotarov
>> Cc: Michal Schmidt; netdev@vger.kernel.org; Dmitry Kravkov; Eilon
>> Greenstein
>> Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG
>>
>> 2011/8/31 Vladislav Zolotarov <vladz@broadcom.com>:
>> >> -----Original Message-----
>> >> From: Michal Schmidt [mailto:mschmidt@redhat.com]
>> >> Sent: Wednesday, August 31, 2011 6:59 PM
>> >> To: Vladislav Zolotarov
>> >> Cc: netdev@vger.kernel.org; Dmitry Kravkov; Eilon Greenstein;
>> >> mirqus@gmail.com
>> >> Subject: Re: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG
>> >>
>> >> On Wed, 31 Aug 2011 18:16:30 +0300 Vlad Zolotarov wrote:
>> >> > On Wednesday 31 August 2011 18:00:34 Michal Schmidt wrote:
>> >> > >   if (bnx2x_reload) {
>> >> > > -         if (bp->recovery_state == BNX2X_RECOVERY_DONE)
>> >> > > +         if (bp->recovery_state == BNX2X_RECOVERY_DONE) {
>> >> > > +                 /*
>> >> > > +                  * Cheat! Normally dev->features will be
>> >> > > set after we
>> >> > > +                  * return, but that's too late. We need to
>> >> > > know how to
>> >> > > +                  * configure the NIC when reloading it, so
>> >> > > update
>> >> > > +                  * the features early.
>> >> > > +                  */
>> >> > > +                 dev->features = features;
>> >> > >                   return bnx2x_reload_if_running(dev);
>> >> >
>> >> > NACK
>> >> >
>> >> > This is bogus - what if bnx2x_reload_if_running(dev)
>> >> > (bnx2x_nic_load()) failes? The original dev->features would be
>> >> > lost...
>> >>
>> >> Well, yes, but since the NIC would be now not working, do we really
>> >> care about its features? :-)
>> >
>> > U r kidding, right? ;)
>> > We care about the consistency in the netdev features state - if we
>> failed
>> > to configure the requested feature and returned an error on e.g.
>> "ethtool -K ethX lro on"
>> > call, it's expected that a subsequent ethtool -k ethX call won't
>> report the requested
>> > feature (LRO) as set.
>>
>> If bnx2x_reload_if_running() failure means that NIC is disabled, then
>> Michal is right that there's no point in caring about dev->features,
>> sice 'load' operation (NIC configuration) needs to be done again
>> anyway.
> Michal, it's a matter of a consistent semantics/behavior of the ethtool callbacks just as I described above.
> As long as dev->features may be queried both when device is down I'm afraid I can't agree with u.

If I understand correctly, bnx2x_reload_if_running() failure does not
mean exactly that features change failed. It means that device failed
to initialize, possibly because of other (transient?) conditions. So
unless reverting features change and retrying the initialization is
known to allow the device to be brought back, there's no difference
whether old or new dev->features value is kept and which set is
reported by ethtool.

Best Regards,
Michał Mirosław

RE: [PATCH 1/3] bnx2x: remove TPA_ENABLE_FLAG

[Vladislav Zolotarov]

> If I understand correctly, bnx2x_reload_if_running() failure does not
> mean exactly that features change failed. It means that device failed
> to initialize, possibly because of other (transient?) conditions. So
> unless reverting features change and retrying the initialization is
> known to allow the device to be brought back, 

In a general case u can't be sure which (if at all) features change caused
the failure. So, u should return the configuration to the last known "good" one.

> there's no difference
> whether old or new dev->features value is kept and which set is
> reported by ethtool.

Well, I disagree with u on this one - I think that if the state change operation
(of any object, in our case it's a netdev) fails, the current state should remain unchanged.

Thanks,
vlad

> 
> Best Regards,
> Michał Mirosław