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(-)
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))
--
2.43.0On 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 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,
--
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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, 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, -- Romain Gantois, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
© 2016 - 2025 Red Hat, Inc.