| 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
| ||
|
Date: Thu, 22 Jun 2023 12:55:52 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 1/6] net: dsa: vsc73xx: convert to PHYLINK
On Wed, Jun 21, 2023 at 09:12:56PM +0200, Pawel Dembicki wrote:
> + /* This driver does not make use of the speed, duplex, pause or the
> + * advertisement in its mac_config, so it is safe to mark this driver
> + * as non-legacy.
> + */
> + config->legacy_pre_march2020 = false;
Great stuff, thanks!
> +static void vsc73xx_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct vsc73xx *vsc = ds->priv;
Nit: normally have a blank line between function variable declarations
and the rest of the function body.
> /* Special handling of the CPU-facing port */
> if (port == CPU_PORT) {
> /* Other ports are already initialized but not this one */
> @@ -775,104 +803,92 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
> VSC73XX_ADVPORTM_ENA_GTX |
> VSC73XX_ADVPORTM_DDR_MODE);
> }
> +}
>
> - /* This is the MAC confiuration that always need to happen
> - * after a PHY or the CPU port comes up or down.
> - */
> - if (!phydev->link) {
> - int maxloop = 10;
> +static void vsc73xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u32 val;
>
> - dev_dbg(vsc->dev, "port %d: went down\n",
> - port);
> + int maxloop = VSC73XX_TABLE_ATTEMPTS;
Reverse christmas-tree for variable declarations, and there shouldn't
be a blank line between them. In any case, I don't think you need
"maxloop" if you adopt my suggestion below.
>
> - /* Disable RX on this port */
> - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> - VSC73XX_MAC_CFG,
> - VSC73XX_MAC_CFG_RX_EN, 0);
> + dev_dbg(vsc->dev, "port %d: went down\n",
> + port);
>
> - /* Discard packets */
> - vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> - VSC73XX_ARBDISC, BIT(port), BIT(port));
> + /* Disable RX on this port */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_MAC_CFG,
> + VSC73XX_MAC_CFG_RX_EN, 0);
> +
> + /* Discard packets */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ARBITER, 0,
> + VSC73XX_ARBDISC, BIT(port), BIT(port));
>
> - /* Wait until queue is empty */
> + /* Wait until queue is empty */
> + vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> + VSC73XX_ARBEMPTY, &val);
> + while (!(val & BIT(port))) {
> + msleep(1);
> vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> VSC73XX_ARBEMPTY, &val);
> - while (!(val & BIT(port))) {
> - msleep(1);
> - vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
> - VSC73XX_ARBEMPTY, &val);
> - if (--maxloop == 0) {
> - dev_err(vsc->dev,
> - "timeout waiting for block arbiter\n");
> - /* Continue anyway */
> - break;
> - }
> + if (--maxloop == 0) {
> + dev_err(vsc->dev,
> + "timeout waiting for block arbiter\n");
> + /* Continue anyway */
> + break;
> }
> + }
I know you're only moving this code, but I think it would be good to
_first_ have a patch that fixes this polling function:
int ret, err;
...
ret = read_poll_timeout(vsc73xx_read, err,
err < 0 || (val & BIT(port)),
1000, 10000, false,
vsc, VSC73XX_BLOCK_ARBITER, 0,
VSC73XX_ARBEMPTY, &val);
if (ret != 0)
dev_err(vsc->dev,
"timeout waiting for block arbiter\n");
else if (err < 0)
dev_err(vsc->dev,
"error reading arbiter\n");
This avoids the issue that on the last iteration, the code reads the
register, test it, find the condition that's being waiting for is
false, _then_ waits and end up printing the error message - that last
wait is rather useless, and as the arbiter state isn't checked after
waiting, it could be that we had success during the last wait.
> +static void vsc73xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev,
> + int speed, int duplex,
> + bool tx_pause, bool rx_pause)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u32 val;
>
> + switch (speed) {
> + case SPEED_1000:
> /* Set up default for internal port or external RGMII */
> - if (phydev->interface == PHY_INTERFACE_MODE_RGMII)
> + if (interface == PHY_INTERFACE_MODE_RGMII)
> val = VSC73XX_MAC_CFG_1000M_F_RGMII;
> else
> val = VSC73XX_MAC_CFG_1000M_F_PHY;
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else if (phydev->speed == SPEED_100) {
> - if (phydev->duplex == DUPLEX_FULL) {
> - val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 100 Mbit full duplex mode\n",
> - port);
> - } else {
> - val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 100 Mbit half duplex mode\n",
> - port);
> - }
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else if (phydev->speed == SPEED_10) {
> - if (phydev->duplex == DUPLEX_FULL) {
> + break;
> + case SPEED_100:
> + case SPEED_10:
> + if (duplex == DUPLEX_FULL)
> val = VSC73XX_MAC_CFG_100_10M_F_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 10 Mbit full duplex mode\n",
> - port);
> - } else {
> + else
> val = VSC73XX_MAC_CFG_100_10M_H_PHY;
> - dev_dbg(vsc->dev,
> - "port %d: 10 Mbit half duplex mode\n",
> - port);
> - }
> - vsc73xx_adjust_enable_port(vsc, port, phydev, val);
> - } else {
> - dev_err(vsc->dev,
> - "could not adjust link: unknown speed\n");
> + break;
> }
Do the dev_dbg() add anything useful over what phylink prints when the
link comes up?
I don't think moving to a switch() statement for this is a good idea.
Given that "val" may be uninitialised, I suspect the following may be
a better solution:
if (speed == SPEED_1000 || speed == SPEED_100 || speed == SPEED_10) {
if (speed == SPEED_1000) {
...
} else {
...
}
... set VSC73XX_BLOCK_ANALYZER and call
vsc73xx_adjust_enable_port ...
}
However, looking at the definitions of the various macros, it seems we
can do a little better by not using the VSC73XX_MAC_CFG_*M_[FH]_*
definitions:
if (speed == SPEED_1000) {
val = VSC73XX_MAC_CFG_GIGA_MODE |
VSC73XX_MAC_CFG_TX_IPG_1000M;
if (interface == PHY_INTERFACE_MODE_RGMII)
val |= VSC73XX_MAC_CFG_CLK_SEL_1000M;
else
val |= VSC73XX_MAC_CFG_CLK_SEL_EXT;
} else {
val = VSC73XX_MAC_CFG_TX_IPG_100_10M |
VSC73XX_MAC_CFG_CLK_SEL_EXT;
}
if (duplex == DUPLEX_FULL)
val |= VSC73XX_MAC_CFG_FDX;
Now, this reveals a question: when operating in RGMII mode, why do we
need VSC73XX_MAC_CFG_CLK_SEL_1000M for 1G, and
VSC73XX_MAC_CFG_CLK_SEL_EXT for 10M and 100M, whereas "PHY" mode always
uses CLK_SEL_EXT ?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists