| 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: <aa5e48e4b03eb9fd8eb6dacb439ef8e600245774.camel@collabora.com>
Date: Tue, 07 Dec 2021 12:47:35 +0000
From: Martyn Welch <martyn.welch@...labora.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC net] net: dsa: mv88e6xxx: allow use of PHYs on CPU
and DSA ports
Sorry Russell, but unfortunately this patch doesn't seem to be
resolving the issue for me.
I've dumped in a bit of debug around this change to see if I could
determine what was going on here, it seems that in my case the function
is being exited before this at:
/* FIXME: is this the correct test? If we're in fixed mode on an
* internal port, why should we process this any different from
* PHY mode? On the other hand, the port may be automedia between
* an internal PHY and the serdes...
*/
if ((mode == MLO_AN_PHY) && mv88e6xxx_phy_is_internal(ds, port))
return;
Martyn
On Tue, 2021-12-07 at 10:59 +0000, Russell King (Oracle) wrote:
> Martyn Welch reports that his CPU port is unable to link where it has
> been necessary to use one of the switch ports with an internal PHY for
> the CPU port. The reason behind this is the port control register is
> left forcing the link down, preventing traffic flow.
>
> This occurs because during initialisation, phylink expects the link to
> be down, and DSA forces the link down by synthesising a call to the
> DSA drivers phylink_mac_link_down() method, but we don't touch the
> forced-link state when we later reconfigure the port.
>
> Resolve this by also unforcing the link state when we are operating in
> PHY mode and the PPU is set to poll the PHY to retrieve link status
> information.
>
> Reported-by: Martyn Welch <martyn.welch@...labora.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> ---
> This patch requires:
> https://lore.kernel.org/r/E1muXm7-00EwJB-7n@rmk-PC.armlinux.org.uk
>
> If we convert Marvell DSA filly to split PCS (we have 6185 and 6352
> complete, 6390 and 6393 remain), the "legacy_march2020" patches, and
> the phylink get_caps patches, we can then make Marvell DSA a non-
> legacy phylink driver. This gives much cleaner semantics concerning
> when the phylink_mac_config() method is called. Specifically, being
> non-legacy means phylink_mac_config() will no longer be called just
> before every phylink_mac_link_up() method call, and will not be called
> for in-band advertisement changes.
>
> This means that phylink_mac_config() will only be called when the port
> needs to be reconfigured, which allows drivers to force the link down
> for safe reconfiguration without potentially causing an infinite loop
> of link-down link-up events that can occur with the legacy behaviour.
>
> Thus, being non-legacy and allowing the forcing removes the need for
> DSA to synthesise a call to phylink_mac_link_down() at initialisation
> time for this driver... but we need all DSA drivers to be non-legacy
> before we can safely remove that.
>
> drivers/net/dsa/mv88e6xxx/chip.c | 56 +++++++++++++++++++-------------
> 1 file changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> b/drivers/net/dsa/mv88e6xxx/chip.c
> index 9f675464efc3..b033e653d3f4 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -698,7 +698,7 @@ static void mv88e6xxx_mac_config(struct dsa_switch
> *ds, int port,
> {
> struct mv88e6xxx_chip *chip = ds->priv;
> struct mv88e6xxx_port *p;
> - int err;
> + int err = 0;
>
> p = &chip->ports[port];
>
> @@ -711,31 +711,43 @@ static void mv88e6xxx_mac_config(struct
> dsa_switch *ds, int port,
> return;
>
> mv88e6xxx_reg_lock(chip);
> - /* In inband mode, the link may come up at any time while the
> link
> - * is not forced down. Force the link down while we reconfigure
> the
> - * interface mode.
> - */
> - if (mode == MLO_AN_INBAND && p->interface != state->interface
> &&
> - chip->info->ops->port_set_link)
> - chip->info->ops->port_set_link(chip, port,
> LINK_FORCED_DOWN);
>
> - err = mv88e6xxx_port_config_interface(chip, port, state-
> >interface);
> - if (err && err != -EOPNOTSUPP)
> - goto err_unlock;
> -
> - err = mv88e6xxx_serdes_pcs_config(chip, port, mode, state-
> >interface,
> - state->advertising);
> - /* FIXME: we should restart negotiation if something changed -
> which
> - * is something we get if we convert to using phylinks PCS
> operations.
> - */
> - if (err > 0)
> - err = 0;
> + if (mode != MLO_AN_PHY || !mv88e6xxx_phy_is_internal(ds, port))
> {
> + /* In inband mode, the link may come up at any time
> while the
> + * link is not forced down. Force the link down while
> we
> + * reconfigure the interface mode.
> + */
> + if (mode == MLO_AN_INBAND &&
> + p->interface != state->interface &&
> + chip->info->ops->port_set_link)
> + chip->info->ops->port_set_link(chip, port,
> +
> LINK_FORCED_DOWN);
> +
> + err = mv88e6xxx_port_config_interface(chip, port,
> + state-
> >interface);
> + if (err && err != -EOPNOTSUPP)
> + goto err_unlock;
> +
> + err = mv88e6xxx_serdes_pcs_config(chip, port, mode,
> + state->interface,
> + state->advertising);
> + /* FIXME: we should restart negotiation if something
> changed -
> + * which is something we get if we convert to using
> phylinks
> + * PCS operations.
> + */
> + if (err > 0)
> + err = 0;
> + }
>
> /* Undo the forced down state above after completing
> configuration
> - * irrespective of its state on entry, which allows the link to
> come up.
> + * irrespective of its state on entry, which allows the link to
> come
> + * up in the in-band case where there is no separate SERDES.
> Also
> + * ensure that the link can come up if the PPU is in use and we
> are
> + * in PHY mode (we treat the PPU as an effective in-band
> mechanism.)
> */
> - if (mode == MLO_AN_INBAND && p->interface != state->interface
> &&
> - chip->info->ops->port_set_link)
> + if (chip->info->ops->port_set_link &&
> + ((mode == MLO_AN_INBAND && p->interface != state-
> >interface) ||
> + (mode == MLO_AN_PHY && mv88e6xxx_port_ppu_updates(chip,
> port))))
> chip->info->ops->port_set_link(chip, port,
> LINK_UNFORCED);
>
> p->interface = state->interface;
Powered by blists - more mailing lists