| Message ID | 20230418111459.811553-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 59be75db5966f920bf5cccfd2a34b3bf222660d7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool mm API consolidation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 21 this patch: 21 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 21 this patch: 21 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 17 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Apr 18, 2023 at 02:14:51PM +0300, Vladimir Oltean wrote: > Current enetc_set_mm() is designed to set the priv->active_offloads bit > ENETC_F_QBU for enetc_mm_link_state_update() to act on, but if the link > is already up, it modifies the ENETC_MMCSR_ME ("Merge Enable") bit > directly. > > The problem is that it only *sets* ENETC_MMCSR_ME if the link is up, it > doesn't *clear* it if needed. So subsequent enetc_get_mm() calls still > see tx-enabled as true, up until a link down event, which is when > enetc_mm_link_state_update() will get called. > > This is not a functional issue as far as I can assess. It has only come > up because I'd like to uphold a simple API rule in core ethtool code: > the pMAC cannot be disabled if TX is going to be enabled. Currently, > the fact that TX remains enabled for longer than expected (after the > enetc_set_mm() call that disables it) is going to violate that rule, > which is how it was caught. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > v1->v2: none > > drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > index 838750a03cf6..ee1ea71fe79e 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > @@ -1041,10 +1041,13 @@ static int enetc_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg, > else > priv->active_offloads &= ~ENETC_F_QBU; > > - /* If link is up, enable MAC Merge right away */ > - if (!!(priv->active_offloads & ENETC_F_QBU) && > - !(val & ENETC_MMCSR_LINK_FAIL)) > - val |= ENETC_MMCSR_ME; > + /* If link is up, enable/disable MAC Merge right away */ > + if (!(val & ENETC_MMCSR_LINK_FAIL)) { > + if (!!(priv->active_offloads & ENETC_F_QBU)) nit: The !!() seems unnecessary, I wonder if it can be written in a simpler way as: if (priv->active_offloads & ENETC_F_QBU) > + val |= ENETC_MMCSR_ME; > + else > + val &= ~ENETC_MMCSR_ME; > + } > > val &= ~ENETC_MMCSR_VT_MASK; > val |= ENETC_MMCSR_VT(cfg->verify_time); > -- > 2.34.1 >
On Thu, Apr 20, 2023 at 04:22:04PM +0200, Simon Horman wrote: > > - /* If link is up, enable MAC Merge right away */ > > - if (!!(priv->active_offloads & ENETC_F_QBU) && > > - !(val & ENETC_MMCSR_LINK_FAIL)) > > - val |= ENETC_MMCSR_ME; > > + /* If link is up, enable/disable MAC Merge right away */ > > + if (!(val & ENETC_MMCSR_LINK_FAIL)) { > > + if (!!(priv->active_offloads & ENETC_F_QBU)) > > nit: The !!() seems unnecessary, > I wonder if it can be written in a simpler way as: > > if (priv->active_offloads & ENETC_F_QBU) I agree. Normally I omit the double negation in simple statements like this. Here I didn't, because the expression was split into 2 "if" conditions, and I kept the individual terms as-is for some reason. Since the generated object code is absolutely the same either way, I would not resend just for minor style comments such as this one, if you don't mind. However, I do appreciate the review and I'll pay more attention to this detail in the future.
On Thu, Apr 20, 2023 at 08:03:54PM +0300, Vladimir Oltean wrote: > On Thu, Apr 20, 2023 at 04:22:04PM +0200, Simon Horman wrote: > > > - /* If link is up, enable MAC Merge right away */ > > > - if (!!(priv->active_offloads & ENETC_F_QBU) && > > > - !(val & ENETC_MMCSR_LINK_FAIL)) > > > - val |= ENETC_MMCSR_ME; > > > + /* If link is up, enable/disable MAC Merge right away */ > > > + if (!(val & ENETC_MMCSR_LINK_FAIL)) { > > > + if (!!(priv->active_offloads & ENETC_F_QBU)) > > > > nit: The !!() seems unnecessary, > > I wonder if it can be written in a simpler way as: > > > > if (priv->active_offloads & ENETC_F_QBU) > > I agree. Normally I omit the double negation in simple statements like this. > Here I didn't, because the expression was split into 2 "if" conditions, > and I kept the individual terms as-is for some reason. > > Since the generated object code is absolutely the same either way, I would not > resend just for minor style comments such as this one, if you don't mind. > However, I do appreciate the review and I'll pay more attention to this > detail in the future. Thanks. I agree the result should be same. No need to resend because of this.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c index 838750a03cf6..ee1ea71fe79e 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c @@ -1041,10 +1041,13 @@ static int enetc_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg, else priv->active_offloads &= ~ENETC_F_QBU; - /* If link is up, enable MAC Merge right away */ - if (!!(priv->active_offloads & ENETC_F_QBU) && - !(val & ENETC_MMCSR_LINK_FAIL)) - val |= ENETC_MMCSR_ME; + /* If link is up, enable/disable MAC Merge right away */ + if (!(val & ENETC_MMCSR_LINK_FAIL)) { + if (!!(priv->active_offloads & ENETC_F_QBU)) + val |= ENETC_MMCSR_ME; + else + val &= ~ENETC_MMCSR_ME; + } val &= ~ENETC_MMCSR_VT_MASK; val |= ENETC_MMCSR_VT(cfg->verify_time);
Current enetc_set_mm() is designed to set the priv->active_offloads bit ENETC_F_QBU for enetc_mm_link_state_update() to act on, but if the link is already up, it modifies the ENETC_MMCSR_ME ("Merge Enable") bit directly. The problem is that it only *sets* ENETC_MMCSR_ME if the link is up, it doesn't *clear* it if needed. So subsequent enetc_get_mm() calls still see tx-enabled as true, up until a link down event, which is when enetc_mm_link_state_update() will get called. This is not a functional issue as far as I can assess. It has only come up because I'd like to uphold a simple API rule in core ethtool code: the pMAC cannot be disabled if TX is going to be enabled. Currently, the fact that TX remains enabled for longer than expected (after the enetc_set_mm() call that disables it) is going to violate that rule, which is how it was caught. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: none drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)