| Message ID | 20240412180340.7965-5-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: Fix MAC-capabilities procedure | expand |
Hi Serge, On Fri, 12 Apr 2024, Serge Semin wrote: > +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, > + phy_interface_t interface) > +{ > + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > + > + /* Get the MAC-specific capabilities */ > + stmmac_mac_phylink_get_caps(priv); This is a bit of a nitpick, but the terminology is quite confusing between stmmac_mac_phylink_get_caps() and stmmac_mac_get_caps(). Ideally, we could just get rid of the whole stmmac_do_void_callback() complexity and just call phylink_get_caps() directly. In the meantime, maybe renaming this to stmmac_mac_core_get_caps() would be acceptable? Please let me know what you think. Thanks,
On Fri, 2024-04-12 at 21:03 +0300, Serge Semin wrote: > After a set of fixes the stmmac_phy_setup() and stmmac_reinit_queues() > method have turned to having some duplicated code. Let's get rid from the > duplication by moving the MAC-capabilities initialization to the PHYLINK > MAC-capabilities getter. The getter is called during each network device > interface open/close cycle. So the MAC-capabilities will be initialized in > normal device functioning and in case of the Tx/Rx queues > re-initialization as the original code semantics implies. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> This is a net-next follow-up for the previous 3 patches targeting 'net', right? If so, you should have posted this one separately after the other would have been merged back into net-next. We can apply the first 3 to 'net', but you will have to repost 4/4 after ~Thu. Thanks, Paolo
On Tue, Apr 16, 2024 at 12:27:29PM +0200, Paolo Abeni wrote: > On Fri, 2024-04-12 at 21:03 +0300, Serge Semin wrote: > > After a set of fixes the stmmac_phy_setup() and stmmac_reinit_queues() > > method have turned to having some duplicated code. Let's get rid from the > > duplication by moving the MAC-capabilities initialization to the PHYLINK > > MAC-capabilities getter. The getter is called during each network device > > interface open/close cycle. So the MAC-capabilities will be initialized in > > normal device functioning and in case of the Tx/Rx queues > > re-initialization as the original code semantics implies. > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > This is a net-next follow-up for the previous 3 patches > targeting 'net', right? Right. The last patch in the series is a cleanup patch which gets rid from the duplicated code by moving it to the PHYLINK MAC-capability getter. > > If so, you should have posted this one separately after the other would > have been merged back into net-next. I thought about that initially. But since this patch content is connected with the rest of patches and the maintainers/reviewers may ask to do things differently than it's provided in the initial patches, I decided to submit the entire series as is but indicating that the last patch is intended for 'net-next'. > > We can apply the first 3 to 'net', but you will have to repost 4/4 > after ~Thu. Agreed. I'll repost 4/4 after the initial 3 patches get to be merged in. Thanks -Serge(y) > > Thanks, > > Paolo >
Hi Romain On Tue, Apr 16, 2024 at 09:56:32AM +0200, Romain Gantois wrote: > Hi Serge, > > On Fri, 12 Apr 2024, Serge Semin wrote: > > > +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, > > + phy_interface_t interface) > > +{ > > + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > > + > > + /* Get the MAC-specific capabilities */ > > + stmmac_mac_phylink_get_caps(priv); > > This is a bit of a nitpick, but the terminology is quite confusing between > stmmac_mac_phylink_get_caps() and stmmac_mac_get_caps(). Right, the naming turns to be ambiguous "a bit".) > Ideally, we could just > get rid of the whole stmmac_do_void_callback() complexity and just call > phylink_get_caps() directly. No, this isn't a good solution. The local coding convention implies using the macro-functions implemented to execute the callbacks. We can't use the macros everywhere but in this place. > In the meantime, maybe renaming this to > stmmac_mac_core_get_caps() would be acceptable? The name was selected to align with the rest of the PHYLINK callbacks: static const struct phylink_mac_ops stmmac_phylink_mac_ops = { .mac_get_caps = stmmac_mac_get_caps, .mac_select_pcs = stmmac_mac_select_pcs, .mac_config = stmmac_mac_config, .mac_link_down = stmmac_mac_link_down, .mac_link_up = stmmac_mac_link_up, }; So I don't think that changing it to something different would be a good alternative. What could be a better option is to rename the stmmac_ops::phylink_get_caps() callback and stmmac_mac_phylink_get_caps() macro-function to something like: stmmac_ops::link_update_caps() stmmac_mac_link_update_caps() especially seeing the callback no longer sets the phylink MAC capabilities directly. What do you think? -Serge(y) > > Please let me know what you think. > > Thanks, > > -- > Romain Gantois, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Tue, Apr 16, 2024 at 09:56:32AM +0200, Romain Gantois wrote: > Hi Serge, > > On Fri, 12 Apr 2024, Serge Semin wrote: > > > +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, > > + phy_interface_t interface) > > +{ > > + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); > > + > > + /* Get the MAC-specific capabilities */ > > + stmmac_mac_phylink_get_caps(priv); > > This is a bit of a nitpick, but the terminology is quite confusing between > stmmac_mac_phylink_get_caps() and stmmac_mac_get_caps(). Ideally, we could just > get rid of the whole stmmac_do_void_callback() complexity and just call > phylink_get_caps() directly. In the meantime, maybe renaming this to > stmmac_mac_core_get_caps() would be acceptable? I'd prefer not to do that. If the method is called mac_get_caps() then I'd much rather have method implementations called foo_mac_get_caps() which makes grep easier. So... stmmac_core_mac_get_caps() would be acceptable to me.
On Tue, 16 Apr 2024, Serge Semin wrote: > So I don't think that changing it to something different would be a > good alternative. What could be a better option is to rename the > stmmac_ops::phylink_get_caps() callback and > stmmac_mac_phylink_get_caps() macro-function to something like: > > stmmac_ops::link_update_caps() > stmmac_mac_link_update_caps() > > especially seeing the callback no longer sets the phylink MAC > capabilities directly. What do you think? This seems like a good solution to me. Thanks,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 7c6fb14b5555..cb72dd93191e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -936,6 +936,22 @@ static void stmmac_mac_flow_ctrl(struct stmmac_priv *priv, u32 duplex) priv->pause, tx_cnt); } +static unsigned long stmmac_mac_get_caps(struct phylink_config *config, + phy_interface_t interface) +{ + struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev)); + + /* Get the MAC-specific capabilities */ + stmmac_mac_phylink_get_caps(priv); + + config->mac_capabilities = priv->hw->link.caps; + + if (priv->plat->max_speed) + phylink_limit_mac_speed(config, priv->plat->max_speed); + + return config->mac_capabilities; +} + static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config, phy_interface_t interface) { @@ -1105,6 +1121,7 @@ static void stmmac_mac_link_up(struct phylink_config *config, } static const struct phylink_mac_ops stmmac_phylink_mac_ops = { + .mac_get_caps = stmmac_mac_get_caps, .mac_select_pcs = stmmac_mac_select_pcs, .mac_config = stmmac_mac_config, .mac_link_down = stmmac_mac_link_down, @@ -1204,7 +1221,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) int mode = priv->plat->phy_interface; struct fwnode_handle *fwnode; struct phylink *phylink; - int max_speed; priv->phylink_config.dev = &priv->dev->dev; priv->phylink_config.type = PHYLINK_NETDEV; @@ -1225,15 +1241,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) xpcs_get_interfaces(priv->hw->xpcs, priv->phylink_config.supported_interfaces); - /* Get the MAC specific capabilities */ - stmmac_mac_phylink_get_caps(priv); - - priv->phylink_config.mac_capabilities = priv->hw->link.caps; - - max_speed = priv->plat->max_speed; - if (max_speed) - phylink_limit_mac_speed(&priv->phylink_config, max_speed); - fwnode = priv->plat->port_node; if (!fwnode) fwnode = dev_fwnode(priv->device); @@ -7327,7 +7334,6 @@ int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt) { struct stmmac_priv *priv = netdev_priv(dev); int ret = 0, i; - int max_speed; if (netif_running(dev)) stmmac_release(dev); @@ -7341,14 +7347,6 @@ int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt) priv->rss.table[i] = ethtool_rxfh_indir_default(i, rx_cnt); - stmmac_mac_phylink_get_caps(priv); - - priv->phylink_config.mac_capabilities = priv->hw->link.caps; - - max_speed = priv->plat->max_speed; - if (max_speed) - phylink_limit_mac_speed(&priv->phylink_config, max_speed); - stmmac_napi_add(dev); if (netif_running(dev))
After a set of fixes the stmmac_phy_setup() and stmmac_reinit_queues() method have turned to having some duplicated code. Let's get rid from the duplication by moving the MAC-capabilities initialization to the PHYLINK MAC-capabilities getter. The getter is called during each network device interface open/close cycle. So the MAC-capabilities will be initialized in normal device functioning and in case of the Tx/Rx queues re-initialization as the original code semantics implies. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-)