| Message ID | 20231006053706.514618-1-will@extrahop.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | da6192ca72d5ad913d109d43dc896290ad05d98f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/mlx5e: Again mutually exclude RX-FCS and RX-port-timestamp | expand |
On 06/10/2023 8:37, Will Mortensen wrote: > Commit 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs > flag change") seems to have accidentally inverted the logic added in > commit 0bc73ad46a76 ("net/mlx5e: Mutually exclude RX-FCS and > RX-port-timestamp"). > > The impact of this is a little unclear since it seems the FCS scattered > with RX-FCS is (usually?) correct regardless. > Thanks for your patch. > Fixes: 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs flag change") > Tested-by: Charlotte Tan <charlotte@extrahop.com> > Reviewed-by: Charlotte Tan <charlotte@extrahop.com> > Cc: Adham Faris <afaris@nvidia.com> > Cc: Aya Levin <ayal@nvidia.com> > Cc: Tariq Toukan <tariqt@nvidia.com> > Cc: Moshe Shemesh <moshe@nvidia.com> > Cc: Saeed Mahameed <saeedm@nvidia.com> > Signed-off-by: Will Mortensen <will@extrahop.com> > --- > For what it's worth, regardless of this change the PCMR register behaves > unexpectedly in our testing on NICs where rx_ts_over_crc_cap is 1 (i.e. > where rx_ts_over_crc is supported), such as ConnectX-7 running firmware > 28.37.1014. For example, fcs_chk is always 0, and rx_ts_over_crc can > never be set to 1 after being set to 0. On ConnectX-5, where > rx_ts_over_crc_cap is 0, fcs_chk behaves as expected. > > We'll probably be opening a support case about that after we test more, > but I mention it here because it makes FCS-related testing confusing. > Please open the case and we'll analyze. > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > index a2ae791538ed..acb40770cf0c 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > @@ -3952,13 +3952,14 @@ static int set_feature_rx_fcs(struct net_device *netdev, bool enable) > struct mlx5e_channels *chs = &priv->channels; > struct mlx5e_params new_params; > int err; > + bool rx_ts_over_crc = !enable; nit: Please maintain the reserved Christmas tree. > > mutex_lock(&priv->state_lock); > > new_params = chs->params; > new_params.scatter_fcs_en = enable; > err = mlx5e_safe_switch_params(priv, &new_params, mlx5e_set_rx_port_ts_wrap, > - &new_params.scatter_fcs_en, true); > + &rx_ts_over_crc, true); > mutex_unlock(&priv->state_lock); > return err; > } Reviewed-by: Tariq Toukan <tariqt@nvidia.com> Regards, Tariq
On Tue, 2023-10-10 at 09:31 +0300, Tariq Toukan wrote: > > On 06/10/2023 8:37, Will Mortensen wrote: > > Commit 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs > > flag change") seems to have accidentally inverted the logic added in > > commit 0bc73ad46a76 ("net/mlx5e: Mutually exclude RX-FCS and > > RX-port-timestamp"). > > > > The impact of this is a little unclear since it seems the FCS scattered > > with RX-FCS is (usually?) correct regardless. > > > > Thanks for your patch. > > > Fixes: 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs flag change") > > Tested-by: Charlotte Tan <charlotte@extrahop.com> > > Reviewed-by: Charlotte Tan <charlotte@extrahop.com> > > Cc: Adham Faris <afaris@nvidia.com> > > Cc: Aya Levin <ayal@nvidia.com> > > Cc: Tariq Toukan <tariqt@nvidia.com> > > Cc: Moshe Shemesh <moshe@nvidia.com> > > Cc: Saeed Mahameed <saeedm@nvidia.com> > > Signed-off-by: Will Mortensen <will@extrahop.com> > > --- > > For what it's worth, regardless of this change the PCMR register behaves > > unexpectedly in our testing on NICs where rx_ts_over_crc_cap is 1 (i.e. > > where rx_ts_over_crc is supported), such as ConnectX-7 running firmware > > 28.37.1014. For example, fcs_chk is always 0, and rx_ts_over_crc can > > never be set to 1 after being set to 0. On ConnectX-5, where > > rx_ts_over_crc_cap is 0, fcs_chk behaves as expected. > > > > We'll probably be opening a support case about that after we test more, > > but I mention it here because it makes FCS-related testing confusing. > > > > Please open the case and we'll analyze. > > > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > index a2ae791538ed..acb40770cf0c 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > @@ -3952,13 +3952,14 @@ static int set_feature_rx_fcs(struct net_device *netdev, bool enable) > > struct mlx5e_channels *chs = &priv->channels; > > struct mlx5e_params new_params; > > int err; > > + bool rx_ts_over_crc = !enable; > > nit: Please maintain the reserved Christmas tree. > > > > > mutex_lock(&priv->state_lock); > > > > new_params = chs->params; > > new_params.scatter_fcs_en = enable; > > err = mlx5e_safe_switch_params(priv, &new_params, mlx5e_set_rx_port_ts_wrap, > > - &new_params.scatter_fcs_en, true); > > + &rx_ts_over_crc, true); > > mutex_unlock(&priv->state_lock); > > return err; > > } > > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> @Tariq: do you prefer we will take this patch directly, or do you prefer send it with a later PR? Thanks, Paolo
On 10/10/2023 11:52, Paolo Abeni wrote: > On Tue, 2023-10-10 at 09:31 +0300, Tariq Toukan wrote: >> >> On 06/10/2023 8:37, Will Mortensen wrote: >>> Commit 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs >>> flag change") seems to have accidentally inverted the logic added in >>> commit 0bc73ad46a76 ("net/mlx5e: Mutually exclude RX-FCS and >>> RX-port-timestamp"). >>> >>> The impact of this is a little unclear since it seems the FCS scattered >>> with RX-FCS is (usually?) correct regardless. >>> >> >> Thanks for your patch. >> >>> Fixes: 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs flag change") >>> Tested-by: Charlotte Tan <charlotte@extrahop.com> >>> Reviewed-by: Charlotte Tan <charlotte@extrahop.com> >>> Cc: Adham Faris <afaris@nvidia.com> >>> Cc: Aya Levin <ayal@nvidia.com> >>> Cc: Tariq Toukan <tariqt@nvidia.com> >>> Cc: Moshe Shemesh <moshe@nvidia.com> >>> Cc: Saeed Mahameed <saeedm@nvidia.com> >>> Signed-off-by: Will Mortensen <will@extrahop.com> >>> --- >>> For what it's worth, regardless of this change the PCMR register behaves >>> unexpectedly in our testing on NICs where rx_ts_over_crc_cap is 1 (i.e. >>> where rx_ts_over_crc is supported), such as ConnectX-7 running firmware >>> 28.37.1014. For example, fcs_chk is always 0, and rx_ts_over_crc can >>> never be set to 1 after being set to 0. On ConnectX-5, where >>> rx_ts_over_crc_cap is 0, fcs_chk behaves as expected. >>> >>> We'll probably be opening a support case about that after we test more, >>> but I mention it here because it makes FCS-related testing confusing. >>> >> >> Please open the case and we'll analyze. >> >>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >>> index a2ae791538ed..acb40770cf0c 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >>> @@ -3952,13 +3952,14 @@ static int set_feature_rx_fcs(struct net_device *netdev, bool enable) >>> struct mlx5e_channels *chs = &priv->channels; >>> struct mlx5e_params new_params; >>> int err; >>> + bool rx_ts_over_crc = !enable; >> >> nit: Please maintain the reserved Christmas tree. >> >>> >>> mutex_lock(&priv->state_lock); >>> >>> new_params = chs->params; >>> new_params.scatter_fcs_en = enable; >>> err = mlx5e_safe_switch_params(priv, &new_params, mlx5e_set_rx_port_ts_wrap, >>> - &new_params.scatter_fcs_en, true); >>> + &rx_ts_over_crc, true); >>> mutex_unlock(&priv->state_lock); >>> return err; >>> } >> >> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > > @Tariq: do you prefer we will take this patch directly, or do you > prefer send it with a later PR? > > Thanks, > > Paolo > Please take it. Thanks, Tariq
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Thu, 5 Oct 2023 22:37:06 -0700 you wrote: > Commit 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs > flag change") seems to have accidentally inverted the logic added in > commit 0bc73ad46a76 ("net/mlx5e: Mutually exclude RX-FCS and > RX-port-timestamp"). > > The impact of this is a little unclear since it seems the FCS scattered > with RX-FCS is (usually?) correct regardless. > > [...] Here is the summary with links: - net/mlx5e: Again mutually exclude RX-FCS and RX-port-timestamp https://git.kernel.org/netdev/net/c/da6192ca72d5 You are awesome, thank you!
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index a2ae791538ed..acb40770cf0c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -3952,13 +3952,14 @@ static int set_feature_rx_fcs(struct net_device *netdev, bool enable) struct mlx5e_channels *chs = &priv->channels; struct mlx5e_params new_params; int err; + bool rx_ts_over_crc = !enable; mutex_lock(&priv->state_lock); new_params = chs->params; new_params.scatter_fcs_en = enable; err = mlx5e_safe_switch_params(priv, &new_params, mlx5e_set_rx_port_ts_wrap, - &new_params.scatter_fcs_en, true); + &rx_ts_over_crc, true); mutex_unlock(&priv->state_lock); return err; }
Commit 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs flag change") seems to have accidentally inverted the logic added in commit 0bc73ad46a76 ("net/mlx5e: Mutually exclude RX-FCS and RX-port-timestamp"). The impact of this is a little unclear since it seems the FCS scattered with RX-FCS is (usually?) correct regardless. Fixes: 1e66220948df8 ("net/mlx5e: Update rx ring hw mtu upon each rx-fcs flag change") Tested-by: Charlotte Tan <charlotte@extrahop.com> Reviewed-by: Charlotte Tan <charlotte@extrahop.com> Cc: Adham Faris <afaris@nvidia.com> Cc: Aya Levin <ayal@nvidia.com> Cc: Tariq Toukan <tariqt@nvidia.com> Cc: Moshe Shemesh <moshe@nvidia.com> Cc: Saeed Mahameed <saeedm@nvidia.com> Signed-off-by: Will Mortensen <will@extrahop.com> --- For what it's worth, regardless of this change the PCMR register behaves unexpectedly in our testing on NICs where rx_ts_over_crc_cap is 1 (i.e. where rx_ts_over_crc is supported), such as ConnectX-7 running firmware 28.37.1014. For example, fcs_chk is always 0, and rx_ts_over_crc can never be set to 1 after being set to 0. On ConnectX-5, where rx_ts_over_crc_cap is 0, fcs_chk behaves as expected. We'll probably be opening a support case about that after we test more, but I mention it here because it makes FCS-related testing confusing. drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)