| Message ID | 20110812143324.5740.45824.stgit@dhcp-29-224.brq.redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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;
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