| 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: <20240925131517.s562xmc5ekkslkhp@skbuf>
Date: Wed, 25 Sep 2024 16:15:17 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Florian Fainelli <f.fainelli@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
Jiawen Wu <jiawenwu@...stnetic.com>,
Jose Abreu <joabreu@...opsys.com>,
Jose Abreu <Jose.Abreu@...opsys.com>,
linux-arm-kernel@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static
configuration reload
On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote:
> The static configuration reload saves the port speed in the static
> configuration tables by first converting it from the internal
> respresentation to the SPEED_xxx ethtool representation, and then
> converts it back to restore the setting. This is because
> sja1105_adjust_port_config() takes the speed as SPEED_xxx.
>
> However, this is unnecessarily complex. If we split
> sja1105_adjust_port_config() up, we can simply save and restore the
> mac[port].speed member in the static configuration tables.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> ---
Thanks for the patch, and the idea is good.
> drivers/net/dsa/sja1105/sja1105_main.c | 63 +++++++++++++-------------
> 1 file changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index bc7e50dcb57c..b95c64b7e705 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -1257,29 +1257,11 @@ static int sja1105_parse_dt(struct sja1105_private *priv)
> return rc;
> }
>
> -/* Convert link speed from SJA1105 to ethtool encoding */
> -static int sja1105_port_speed_to_ethtool(struct sja1105_private *priv,
> - u64 speed)
> -{
> - if (speed == priv->info->port_speed[SJA1105_SPEED_10MBPS])
> - return SPEED_10;
> - if (speed == priv->info->port_speed[SJA1105_SPEED_100MBPS])
> - return SPEED_100;
> - if (speed == priv->info->port_speed[SJA1105_SPEED_1000MBPS])
> - return SPEED_1000;
> - if (speed == priv->info->port_speed[SJA1105_SPEED_2500MBPS])
> - return SPEED_2500;
> - return SPEED_UNKNOWN;
> -}
> -
> -/* Set link speed in the MAC configuration for a specific port. */
> -static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
> - int speed_mbps)
> +static int sja1105_set_port_speed(struct sja1105_private *priv, int port,
> + int speed_mbps)
> {
> struct sja1105_mac_config_entry *mac;
> - struct device *dev = priv->ds->dev;
I think if you could keep this line in the new sja1105_set_port_speed()
function..
> u64 speed;
> - int rc;
>
> /* On P/Q/R/S, one can read from the device via the MAC reconfiguration
> * tables. On E/T, MAC reconfig tables are not readable, only writable.
> @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
> speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> break;
> default:
> - dev_err(dev, "Invalid speed %iMbps\n", speed_mbps);
> + dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps);
you could also get rid of this unnecessary line change.
> return -EINVAL;
> }
There are 2 more changes which I believe should be made in sja1105_set_port_speed():
- since it isn't called from mac_config() anymore but from mac_link_up()
(change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN
- we can trust that phylink will not call mac_link_up() with a speed
outside what we provided in mac_capabilities, so we can remove the
-EINVAL "default" speed_mbps case, and make this method return void,
as it can never truly cause an error
But I believe these are incremental changes which should be done after
this patch. I've made a note of them and will create 2 patches on top
when I have the spare time.
>
> @@ -1325,11 +1307,29 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
> * we need to configure the PCS only (if even that).
> */
> if (priv->phy_mode[port] == PHY_INTERFACE_MODE_SGMII)
> - mac[port].speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS];
> + speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS];
> else if (priv->phy_mode[port] == PHY_INTERFACE_MODE_2500BASEX)
> - mac[port].speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> - else
> - mac[port].speed = speed;
> + speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS];
> +
> + mac[port].speed = speed;
> +
> + return 0;
> +}
> +
> +/* Set link speed in the MAC configuration for a specific port. */
Could this comment state this instead? "Write the MAC Configuration
Table entry and, if necessary, the CGU settings, after a link speed
change for this port."
> +static int sja1105_set_port_config(struct sja1105_private *priv, int port)
> +{
> + struct sja1105_mac_config_entry *mac;
> + struct device *dev = priv->ds->dev;
> + int rc;
> +
> + /* On P/Q/R/S, one can read from the device via the MAC reconfiguration
> + * tables. On E/T, MAC reconfig tables are not readable, only writable.
> + * We have to *know* what the MAC looks like. For the sake of keeping
> + * the code common, we'll use the static configuration tables as a
> + * reasonable approximation for both E/T and P/Q/R/S.
> + */
> + mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
>
> /* Write to the dynamic reconfiguration tables */
> rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
> @@ -1390,7 +1390,8 @@ static void sja1105_mac_link_up(struct phylink_config *config,
> struct sja1105_private *priv = dp->ds->priv;
> int port = dp->index;
>
> - sja1105_adjust_port_config(priv, port, speed);
> + if (!sja1105_set_port_speed(priv, port, speed))
> + sja1105_set_port_config(priv, port);
>
> sja1105_inhibit_tx(priv, BIT(port), false);
> }
> @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
> {
> struct ptp_system_timestamp ptp_sts_before;
> struct ptp_system_timestamp ptp_sts_after;
> - int speed_mbps[SJA1105_MAX_NUM_PORTS];
> + u64 mac_speed[SJA1105_MAX_NUM_PORTS];
Could you move this line lower to preserve the ordering by decreasing line length?
> u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};
> struct sja1105_mac_config_entry *mac;
> struct dsa_switch *ds = priv->ds;
> @@ -2307,14 +2308,13 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
>
> mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
>
> - /* Back up the dynamic link speed changed by sja1105_adjust_port_config
> + /* Back up the dynamic link speed changed by sja1105_set_port_speed
Could you please put () after the function name? I know the original
code did not have it, but my coding style has changed in the past 5 years.
> * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the
> * switch wants to see in the static config in order to allow us to
> * change it through the dynamic interface later.
> */
> for (i = 0; i < ds->num_ports; i++) {
> - speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
> - mac[i].speed);
> + mac_speed[i] = mac[i].speed;
> mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO];
>
> if (priv->xpcs[i])
> @@ -2377,7 +2377,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
> struct dw_xpcs *xpcs = priv->xpcs[i];
> unsigned int neg_mode;
>
> - rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
> + mac[i].speed = mac_speed[i];
> + rc = sja1105_set_port_config(priv, i);
> if (rc < 0)
> goto out;
>
> --
> 2.30.2
>
Powered by blists - more mailing lists