Patchwork bnx2x: suppress repeated error messages about Max BW

login
register
mail settings
Submitter Michal Schmidt
Date Aug. 12, 2011, 2:33 p.m.
Message ID <20110812143324.5740.45824.stgit@dhcp-29-224.brq.redhat.com>
Download mbox | patch
Permalink /patch/109845/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Michal Schmidt - Aug. 12, 2011, 2:33 p.m.
When a VN is configured with invalid Max BW, the error does not have to
be logged repeatedly and fill the logs.
Warn only once when the bad configuration is detected on boot, or when
the configuration changes dynamically from good to bad.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---

 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |    5 ++---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h  |   21 +++++++++++----------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    2 +-
 4 files changed, 15 insertions(+), 14 deletions(-)


--
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
Eilon Greenstein - Aug. 15, 2011, 10:54 a.m.
On Fri, 2011-08-12 at 07:33 -0700, Michal Schmidt wrote:
> When a VN is configured with invalid Max BW, the error does not have to
> be logged repeatedly and fill the logs.
> Warn only once when the bad configuration is detected on boot, or when
> the configuration changes dynamically from good to bad.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> 
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |    1 +
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  |    5 ++---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h  |   21 +++++++++++----------
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    2 +-
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index c423504..648e165 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -1220,6 +1220,7 @@ struct bnx2x {
>  	struct cmng_struct_per_port cmng;
>  	u32			vn_weight_sum;
>  	u32			mf_config[E1HVN_MAX];
> +	bool			prev_max_cfg_invalid[E1HVN_MAX];
The warning is always for the current VN, so if you insist on showing a
warning only once on a board with invalid configuration, you can use a
single boolean.

Thanks,
Eilon


--
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
Michal Schmidt - Aug. 15, 2011, 11:59 a.m.
On 08/15/2011 12:54 PM, Eilon Greenstein wrote:
> On Fri, 2011-08-12 at 07:33 -0700, Michal Schmidt wrote:
>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> index c423504..648e165 100644
>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> @@ -1220,6 +1220,7 @@ struct bnx2x {
>>   	struct cmng_struct_per_port cmng;
>>   	u32			vn_weight_sum;
>>   	u32			mf_config[E1HVN_MAX];
>> +	bool			prev_max_cfg_invalid[E1HVN_MAX];
> The warning is always for the current VN, so if you insist on showing a
> warning only once on a board with invalid configuration, you can use a
> single boolean.

bnx2x_cmng_fns_init() iterates over VNs:

      for (vn = VN_0; vn < E1HVN_MAX; vn++)
              bnx2x_init_vn_minmax(bp, vn);

and bnx2x_init_vn_minmax() calls bnx2x_extract_max_cfg() on the given 
VN, so it seems that the warning can be produced for a non-current VN.

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
Eilon Greenstein - Aug. 15, 2011, 12:33 p.m.
On Mon, 2011-08-15 at 04:59 -0700, Michal Schmidt wrote:
> On 08/15/2011 12:54 PM, Eilon Greenstein wrote:
> > On Fri, 2011-08-12 at 07:33 -0700, Michal Schmidt wrote:
> >> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> >> index c423504..648e165 100644
> >> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> >> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> >> @@ -1220,6 +1220,7 @@ struct bnx2x {
> >>   	struct cmng_struct_per_port cmng;
> >>   	u32			vn_weight_sum;
> >>   	u32			mf_config[E1HVN_MAX];
> >> +	bool			prev_max_cfg_invalid[E1HVN_MAX];
> > The warning is always for the current VN, so if you insist on showing a
> > warning only once on a board with invalid configuration, you can use a
> > single boolean.
> 
> bnx2x_cmng_fns_init() iterates over VNs:
> 
>       for (vn = VN_0; vn < E1HVN_MAX; vn++)
>               bnx2x_init_vn_minmax(bp, vn);
> 
> and bnx2x_init_vn_minmax() calls bnx2x_extract_max_cfg() on the given 
> VN, so it seems that the warning can be produced for a non-current VN.

You are right, only one function (the PMF) will call this code for all
functions. But I suspect that if you have zero values, you will have
them for all VNs - is that the case? If so, you can still warn only
once. Adding 4 boolean variables to the bnx2x structure just to overcome
a bad configuration seems excessive to me.




--
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
Michal Schmidt - Aug. 15, 2011, 3:13 p.m.
On 08/15/2011 02:33 PM, Eilon Greenstein wrote:
> On Mon, 2011-08-15 at 04:59 -0700, Michal Schmidt wrote:
>> and bnx2x_init_vn_minmax() calls bnx2x_extract_max_cfg() on the given
>> VN, so it seems that the warning can be produced for a non-current VN.
>
> You are right, only one function (the PMF) will call this code for all
> functions. But I suspect that if you have zero values, you will have
> them for all VNs - is that the case?

A tester reported getting only these 4 messages with the patch applied:

[bnx2x_extract_max_cfg:1074(eth4)]Illegal configuration detected for Max 
BW on vn 2 - using 100 instead
[bnx2x_extract_max_cfg:1074(eth5)]Illegal configuration detected for Max 
BW on vn 2 - using 100 instead
[bnx2x_extract_max_cfg:1074(eth6)]Illegal configuration detected for Max 
BW on vn 3 - using 100 instead
[bnx2x_extract_max_cfg:1074(eth7)]Illegal configuration detected for Max 
BW on vn 3 - using 100 instead

This suggests that VNs 0 and 1 had non-zero Max BW configuration.

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
Eilon Greenstein - Aug. 15, 2011, 6:47 p.m.
On Mon, 2011-08-15 at 08:13 -0700, Michal Schmidt wrote:
> On 08/15/2011 02:33 PM, Eilon Greenstein wrote:
> > On Mon, 2011-08-15 at 04:59 -0700, Michal Schmidt wrote:
> >> and bnx2x_init_vn_minmax() calls bnx2x_extract_max_cfg() on the given
> >> VN, so it seems that the warning can be produced for a non-current VN.
> >
> > You are right, only one function (the PMF) will call this code for all
> > functions. But I suspect that if you have zero values, you will have
> > them for all VNs - is that the case?
> 
> A tester reported getting only these 4 messages with the patch applied:
> 
> [bnx2x_extract_max_cfg:1074(eth4)]Illegal configuration detected for Max 
> BW on vn 2 - using 100 instead
> [bnx2x_extract_max_cfg:1074(eth5)]Illegal configuration detected for Max 
> BW on vn 2 - using 100 instead
> [bnx2x_extract_max_cfg:1074(eth6)]Illegal configuration detected for Max 
> BW on vn 3 - using 100 instead
> [bnx2x_extract_max_cfg:1074(eth7)]Illegal configuration detected for Max 
> BW on vn 3 - using 100 instead
> 
> This suggests that VNs 0 and 1 had non-zero Max BW configuration.

Michal - this is a great point of data! It helped me finding a bug in
that code - the code is not suitable for 4 port devices, it always
assumes 4 VN per PCI function, while in 4 port devices there are only 2
VN per PCI function. I assume that you are seeing this problem on a
57800 with 2x10G + 2x1G - and the 1G devices are in single function mode
and therefore you are seeing this error message. I will send a patch to
fix the problem on 4 port devices soon (after testing it for a while) -
please confirm that you are seeing this issue on 2x10G+2x1G 57800
device.

Now it all makes sense to me - it’s not just misconfigured board
workaround, this is a real issue :)

Thanks for helping in identifying it!
Eilon



--
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
Michal Schmidt - Aug. 16, 2011, 11:38 a.m.
On 08/15/2011 08:47 PM, Eilon Greenstein wrote:
> On Mon, 2011-08-15 at 08:13 -0700, Michal Schmidt wrote:
>> A tester reported getting only these 4 messages with the patch applied:
>>
>> [bnx2x_extract_max_cfg:1074(eth4)]Illegal configuration detected for Max
>> BW on vn 2 - using 100 instead
>> [bnx2x_extract_max_cfg:1074(eth5)]Illegal configuration detected for Max
>> BW on vn 2 - using 100 instead
>> [bnx2x_extract_max_cfg:1074(eth6)]Illegal configuration detected for Max
>> BW on vn 3 - using 100 instead
>> [bnx2x_extract_max_cfg:1074(eth7)]Illegal configuration detected for Max
>> BW on vn 3 - using 100 instead
>>
>> This suggests that VNs 0 and 1 had non-zero Max BW configuration.
>
> Michal - this is a great point of data! It helped me finding a bug in
> that code - the code is not suitable for 4 port devices, it always
> assumes 4 VN per PCI function, while in 4 port devices there are only 2
> VN per PCI function. I assume that you are seeing this problem on a
> 57800 with 2x10G + 2x1G - and the 1G devices are in single function mode
> and therefore you are seeing this error message. I will send a patch to
> fix the problem on 4 port devices soon (after testing it for a while) -
> please confirm that you are seeing this issue on 2x10G+2x1G 57800
> device.

Eilon,
the tester is seeing this with BCM57711E. It's a HP-Blade bl460c-g6 with 
HP VirtualConnect. Quote from him:

   hp-agents reports 4 dual port nic's, Linux kernel reports 8 identical
   nic's but it's actual a blade with 2 LOM's (lan on motherboard) with
   each one port. Via VC we present max 4 FlexNic's per port, but for
   this server we present 2 FlexNic's per port.

   The fun with Linux is that it always sees all FlexNic's devices even
   if we configure 2 FlexNics via a VC profile on a port like on this
   server.

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
Eilon Greenstein - Aug. 16, 2011, 12:45 p.m.
On Tue, 2011-08-16 at 04:38 -0700, Michal Schmidt wrote:
> On 08/15/2011 08:47 PM, Eilon Greenstein wrote:
> > On Mon, 2011-08-15 at 08:13 -0700, Michal Schmidt wrote:
> >> A tester reported getting only these 4 messages with the patch applied:
> >>
> >> [bnx2x_extract_max_cfg:1074(eth4)]Illegal configuration detected for Max
> >> BW on vn 2 - using 100 instead
> >> [bnx2x_extract_max_cfg:1074(eth5)]Illegal configuration detected for Max
> >> BW on vn 2 - using 100 instead
> >> [bnx2x_extract_max_cfg:1074(eth6)]Illegal configuration detected for Max
> >> BW on vn 3 - using 100 instead
> >> [bnx2x_extract_max_cfg:1074(eth7)]Illegal configuration detected for Max
> >> BW on vn 3 - using 100 instead
> >>
> >> This suggests that VNs 0 and 1 had non-zero Max BW configuration.
> >
> > Michal - this is a great point of data! It helped me finding a bug in
> > that code - the code is not suitable for 4 port devices, it always
> > assumes 4 VN per PCI function, while in 4 port devices there are only 2
> > VN per PCI function. I assume that you are seeing this problem on a
> > 57800 with 2x10G + 2x1G - and the 1G devices are in single function mode
> > and therefore you are seeing this error message. I will send a patch to
> > fix the problem on 4 port devices soon (after testing it for a while) -
> > please confirm that you are seeing this issue on 2x10G+2x1G 57800
> > device.
> 
> Eilon,
> the tester is seeing this with BCM57711E. It's a HP-Blade bl460c-g6 with 
> HP VirtualConnect. Quote from him:
> 
>    hp-agents reports 4 dual port nic's, Linux kernel reports 8 identical
>    nic's but it's actual a blade with 2 LOM's (lan on motherboard) with
>    each one port. Via VC we present max 4 FlexNic's per port, but for
>    this server we present 2 FlexNic's per port.
> 
>    The fun with Linux is that it always sees all FlexNic's devices even
>    if we configure 2 FlexNics via a VC profile on a port like on this
>    server.
> 
> Michal

I see. This seems to be a valid VC configuration and therefore the
message should be reduced to debug level all together. At least we
resolved a 4 port configuration problem...

Eilon




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

Patch

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index c423504..648e165 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1220,6 +1220,7 @@  struct bnx2x {
 	struct cmng_struct_per_port cmng;
 	u32			vn_weight_sum;
 	u32			mf_config[E1HVN_MAX];
+	bool			prev_max_cfg_invalid[E1HVN_MAX];
 	u32			mf2_config[E2_FUNC_MAX];
 	u32			path_has_ovlan; /* E3 */
 	u16			mf_ov;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index d724a18..a5216a9 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -841,8 +841,7 @@  u16 bnx2x_get_mf_speed(struct bnx2x *bp)
 {
 	u16 line_speed = bp->link_vars.line_speed;
 	if (IS_MF(bp)) {
-		u16 maxCfg = bnx2x_extract_max_cfg(bp,
-						   bp->mf_config[BP_VN(bp)]);
+		u16 maxCfg = bnx2x_extract_max_cfg(bp, BP_VN(bp));
 
 		/* Calculate the current MAX line speed limit for the MF
 		 * devices
@@ -1153,7 +1152,7 @@  void bnx2x_update_max_mf_config(struct bnx2x *bp, u32 value)
 	/* load old values */
 	u32 mf_cfg = bp->mf_config[BP_VN(bp)];
 
-	if (value != bnx2x_extract_max_cfg(bp, mf_cfg)) {
+	if (value != bnx2x_extract_max_cfg(bp, BP_VN(bp))) {
 		/* leave all but MAX value */
 		mf_cfg &= ~FUNC_MF_CFG_MAX_BW_MASK;
 
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
index 223bfee..6e75c42 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
@@ -1473,19 +1473,20 @@  void bnx2x_release_phy_lock(struct bnx2x *bp);
  * bnx2x_extract_max_cfg - extract MAX BW part from MF configuration.
  *
  * @bp:		driver handle
- * @mf_cfg:	MF configuration
+ * @vn:		vnic index
  *
  */
-static inline u16 bnx2x_extract_max_cfg(struct bnx2x *bp, u32 mf_cfg)
+static inline u16 bnx2x_extract_max_cfg(struct bnx2x *bp, int vn)
 {
-	u16 max_cfg = (mf_cfg & FUNC_MF_CFG_MAX_BW_MASK) >>
-			      FUNC_MF_CFG_MAX_BW_SHIFT;
-	if (!max_cfg) {
-		BNX2X_ERR("Illegal configuration detected for Max BW - "
-			  "using 100 instead\n");
-		max_cfg = 100;
-	}
-	return max_cfg;
+	u16 max_cfg = (bp->mf_config[vn] & FUNC_MF_CFG_MAX_BW_MASK) >>
+					 FUNC_MF_CFG_MAX_BW_SHIFT;
+
+	if (!max_cfg && !bp->prev_max_cfg_invalid[vn])
+		BNX2X_ERR("Illegal configuration detected for Max BW "
+			  "on vn %d - using 100 instead\n", vn);
+	bp->prev_max_cfg_invalid[vn] = !max_cfg;
+
+	return max_cfg ?: 100;
 }
 
 #endif /* BNX2X_CMN_H */
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 1507091..a952f84 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -2335,7 +2335,7 @@  static void bnx2x_init_vn_minmax(struct bnx2x *bp, int vn)
 		vn_max_rate = 0;
 
 	} else {
-		u32 maxCfg = bnx2x_extract_max_cfg(bp, vn_cfg);
+		u32 maxCfg = bnx2x_extract_max_cfg(bp, vn);
 
 		vn_min_rate = ((vn_cfg & FUNC_MF_CFG_MIN_BW_MASK) >>
 				FUNC_MF_CFG_MIN_BW_SHIFT) * 100;