| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <zrrrivvodf7ovikm4lb7gcmkkff3umujjcrjfdlk5aglfnc6nf@vi7k5b4qjsv4>
Date: Fri, 12 Apr 2024 21:32:34 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>, Andrew Lunn <andrew@...n.ch>,
Russell King <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>
Cc: hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
chenhuacai@...nel.org, linux@...linux.org.uk, guyinggang@...ngson.cn,
netdev@...r.kernel.org, chris.chenfeiyang@...il.com, siyanteng01@...il.com
Subject: Re: [PATCH net-next v11 1/6] net: stmmac: Move all PHYLINK MAC
capabilities initializations to MAC-specific setup methods
Hi Yanteng
On Fri, Apr 12, 2024 at 07:28:06PM +0800, Yanteng Si wrote:
> From: Serge Semin <fancer.lancer@...il.com>
>
> Seeing the Tx-queues-based constraint is DW QoS Eth-specific there is
> such reason. It might be better to move the selective Half-duplex
> mode disabling to the MAC-specific callback.
>
> But there are a better option to implement the MAC capabilities
> detection procedure. Let's see what MAC-capabilities can be currently
> specified based on the DW MAC IP-core versions:
>
> DW MAC100: MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> MAC_10 | MAC_100
>
> DW GMAC: MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> MAC_10 | MAC_100 | MAC_1000
>
> DW QoS Eth: MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD
> but if the amount of the active Tx queues is > 1, then:
> MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> MAC_10FD | MAC_100FD | MAC_1000FD | MAC_2500FD
>
> DW XGMAC: MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> MAC_1000FD | MAC_2500FD | MAC_5000FD |
> MAC_10000FD | MAC_25000FD |
> MAC_40000FD | MAC_50000FD |
> MAC_100000FD
>
> As you can see there are only two common capabilities:
> MAC_ASYM_PAUSE | MAC_SYM_PAUSE. Seeing the flow-control is implemented
> as a callback for each MAC IP-core (see dwmac100_flow_ctrl(),
> dwmac1000_flow_ctrl(), sun8i_dwmac_flow_ctrl(), etc) we can freely
> move all the PHYLINK MAC capabilities initializations to the
> MAC-specific setup methods.
>
> After that the only IP-core which requires the capabilities update will
> be DW QoS Eth. So the Tx-queue-based capabilities update can be moved
> there and the rest of the xgmac_phylink_get_caps() callback can be
> dropped.
>
> We can go further. Instead of calling the
> stmmac_set_half_duplex()/stmmac_set_mac_capabilties() methods on the
> device init and queues reinit stages, we can move their bodies into
> the phylink:mac_get_caps() callback.
>
> Others see:
> <https://lore.kernel.org/netdev/cover.1706601050.git.siyanteng@loong
> son.cn/T/#m7d724d33faee34fed696e4458d9f6b09b0572e77>
Just submitted the series with this patch being properly split up and
described:
https://lore.kernel.org/netdev/20240412180340.7965-1-fancer.lancer@gmail.com/
You can drop this patch, copy my patchset into your repo and rebase
your series onto it. Thus for the time being, until my series is
reviewed and merged in, you'll be able to continue with your patchset
developments/reviews, but submitting only your portion of the patches.
Alternatively my series could be just merged into yours as a set of
the preparation patches, for instance, after it's fully reviewed.
Not sure which solution is better. Andrew? Russell? David? Eric?
Jakub? Paolo?
-Serge(y)
>
> Signed-off-by: Serge Semin <fancer.lancer@...il.com>
> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> ---
> drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
> .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 2 +
> .../ethernet/stmicro/stmmac/dwmac1000_core.c | 2 +
> .../ethernet/stmicro/stmmac/dwmac100_core.c | 2 +
> .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 8 +++-
> .../ethernet/stmicro/stmmac/dwxgmac2_core.c | 15 +++----
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 43 ++++++++-----------
> 7 files changed, 36 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index f55cf09f0783..9cd62b2110a1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -553,6 +553,7 @@ extern const struct stmmac_hwtimestamp stmmac_ptp;
> extern const struct stmmac_mode_ops dwmac4_ring_mode_ops;
>
> struct mac_link {
> + u32 caps;
> u32 speed_mask;
> u32 speed10;
> u32 speed100;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index b21d99faa2d0..e1b761dcfa1d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -1096,6 +1096,8 @@ static struct mac_device_info *sun8i_dwmac_setup(void *ppriv)
>
> priv->dev->priv_flags |= IFF_UNICAST_FLT;
>
> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000;
> /* The loopback bit seems to be re-set when link change
> * Simply mask it each time
> * Speed 10/100/1000 are set in BIT(2)/BIT(3)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> index 3927609abc44..8555299443f4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> @@ -539,6 +539,8 @@ int dwmac1000_setup(struct stmmac_priv *priv)
> if (mac->multicast_filter_bins)
> mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins);
>
> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000;
> mac->link.duplex = GMAC_CONTROL_DM;
> mac->link.speed10 = GMAC_CONTROL_PS;
> mac->link.speed100 = GMAC_CONTROL_PS | GMAC_CONTROL_FES;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c
> index a6e8d7bd9588..7667d103cd0e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c
> @@ -175,6 +175,8 @@ int dwmac100_setup(struct stmmac_priv *priv)
> dev_info(priv->device, "\tDWMAC100\n");
>
> mac->pcsr = priv->ioaddr;
> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10 | MAC_100;
> mac->link.duplex = MAC_CONTROL_F;
> mac->link.speed10 = 0;
> mac->link.speed100 = 0;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> index cef25efbdff9..70a4ac16d3c8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> @@ -70,7 +70,11 @@ static void dwmac4_core_init(struct mac_device_info *hw,
>
> static void dwmac4_phylink_get_caps(struct stmmac_priv *priv)
> {
> - priv->phylink_config.mac_capabilities |= MAC_2500FD;
> + /* Half-Duplex can only work with single tx queue */
> + if (priv->plat->tx_queues_to_use > 1)
> + priv->hw->link.caps &= ~(MAC_10HD | MAC_100HD | MAC_1000HD);
> + else
> + priv->hw->link.caps |= (MAC_10HD | MAC_100HD | MAC_1000HD);
> }
>
> static void dwmac4_rx_queue_enable(struct mac_device_info *hw,
> @@ -1378,6 +1382,8 @@ int dwmac4_setup(struct stmmac_priv *priv)
> if (mac->multicast_filter_bins)
> mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins);
>
> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000 | MAC_2500FD;
> mac->link.duplex = GMAC_CONFIG_DM;
> mac->link.speed10 = GMAC_CONFIG_PS;
> mac->link.speed100 = GMAC_CONFIG_FES | GMAC_CONFIG_PS;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index e841e312077e..759b9b7a2f3f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -47,14 +47,6 @@ static void dwxgmac2_core_init(struct mac_device_info *hw,
> writel(XGMAC_INT_DEFAULT_EN, ioaddr + XGMAC_INT_EN);
> }
>
> -static void xgmac_phylink_get_caps(struct stmmac_priv *priv)
> -{
> - priv->phylink_config.mac_capabilities |= MAC_2500FD | MAC_5000FD |
> - MAC_10000FD | MAC_25000FD |
> - MAC_40000FD | MAC_50000FD |
> - MAC_100000FD;
> -}
> -
> static void dwxgmac2_set_mac(void __iomem *ioaddr, bool enable)
> {
> u32 tx = readl(ioaddr + XGMAC_TX_CONFIG);
> @@ -1540,7 +1532,6 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>
> const struct stmmac_ops dwxgmac210_ops = {
> .core_init = dwxgmac2_core_init,
> - .phylink_get_caps = xgmac_phylink_get_caps,
> .set_mac = dwxgmac2_set_mac,
> .rx_ipc = dwxgmac2_rx_ipc,
> .rx_queue_enable = dwxgmac2_rx_queue_enable,
> @@ -1601,7 +1592,6 @@ static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
>
> const struct stmmac_ops dwxlgmac2_ops = {
> .core_init = dwxgmac2_core_init,
> - .phylink_get_caps = xgmac_phylink_get_caps,
> .set_mac = dwxgmac2_set_mac,
> .rx_ipc = dwxgmac2_rx_ipc,
> .rx_queue_enable = dwxlgmac2_rx_queue_enable,
> @@ -1698,6 +1688,11 @@ int dwxlgmac2_setup(struct stmmac_priv *priv)
> if (mac->multicast_filter_bins)
> mac->mcast_bits_log2 = ilog2(mac->multicast_filter_bins);
>
> + mac->link.caps = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_1000FD | MAC_2500FD | MAC_5000FD |
> + MAC_10000FD | MAC_25000FD |
> + MAC_40000FD | MAC_50000FD |
> + MAC_100000FD;
> mac->link.duplex = 0;
> mac->link.speed1000 = XLGMAC_CONFIG_SS_1000;
> mac->link.speed2500 = XLGMAC_CONFIG_SS_2500;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index fe3498e86de9..af16efeedf4a 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)
> {
> @@ -1102,6 +1118,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,
> @@ -1195,24 +1212,12 @@ static int stmmac_init_phy(struct net_device *dev)
> return ret;
> }
>
> -static void stmmac_set_half_duplex(struct stmmac_priv *priv)
> -{
> - /* Half-Duplex can only work with single tx queue */
> - if (priv->plat->tx_queues_to_use > 1)
> - priv->phylink_config.mac_capabilities &=
> - ~(MAC_10HD | MAC_100HD | MAC_1000HD);
> - else
> - priv->phylink_config.mac_capabilities |=
> - (MAC_10HD | MAC_100HD | MAC_1000HD);
> -}
> -
> static int stmmac_phy_setup(struct stmmac_priv *priv)
> {
> struct stmmac_mdio_bus_data *mdio_bus_data;
> 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;
> @@ -1236,19 +1241,6 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
> xpcs_get_interfaces(priv->hw->xpcs,
> priv->phylink_config.supported_interfaces);
>
> - priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> - MAC_10FD | MAC_100FD |
> - MAC_1000FD;
> -
> - stmmac_set_half_duplex(priv);
> -
> - /* Get the MAC specific capabilities */
> - stmmac_mac_phylink_get_caps(priv);
> -
> - 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);
> @@ -7357,7 +7349,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_set_half_duplex(priv);
> stmmac_napi_add(dev);
>
> if (netif_running(dev))
> --
> 2.31.4
>
Powered by blists - more mailing lists