| 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: <daccdb60-1503-4fcc-87dc-754fb8bf9109@lunn.ch>
Date: Tue, 16 Sep 2025 03:32:34 +0200
From: Andrew Lunn <andrew@...n.ch>
To: David Yang <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, 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>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v9 3/3] net: dsa: yt921x: Add support for
Motorcomm YT921x
> +static int
> +yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
> + phy_interface_t interface, int speed, int duplex,
> + bool tx_pause, bool rx_pause)
> +{
> + struct device *dev = to_device(priv);
> + u32 mask;
> + u32 ctrl;
> + int res;
> +
> + switch (speed) {
> + case SPEED_10:
> + ctrl = YT921X_PORT_SPEED_10;
> + break;
> + case SPEED_100:
> + ctrl = YT921X_PORT_SPEED_100;
> + break;
> + case SPEED_1000:
> + ctrl = YT921X_PORT_SPEED_1000;
> + break;
> + case SPEED_10000:
> + ctrl = YT921X_PORT_SPEED_10000;
> + break;
> + case SPEED_2500:
> + ctrl = YT921X_PORT_SPEED_2500;
> + break;
> + default:
> + dev_err(dev, "Unsupported speed %d\n", speed);
> + /* compile complains about uninitialized variable */
> + ctrl = 0;
> + break;
If this should not happen, it is better to return -EINVAL.
I would also suggest sorting these numerically. 10G after 2.5G.
> + }
> + if (duplex == DUPLEX_FULL)
> + ctrl |= YT921X_PORT_DUPLEX_FULL;
> + if (tx_pause)
> + ctrl |= YT921X_PORT_TX_PAUSE;
> + if (rx_pause)
> + ctrl |= YT921X_PORT_RX_PAUSE;
> + ctrl |= YT921X_PORT_RX_MAC_EN | YT921X_PORT_TX_MAC_EN;
> + res = yt921x_reg_write(priv, YT921X_PORTn_CTRL(port), ctrl);
> + if (res)
> + return res;
> +
> + if (yt921x_port_is_external(port)) {
> + mask = YT921X_SGMII_SPEED_M;
> + switch (speed) {
> + case SPEED_10:
> + ctrl = YT921X_SGMII_SPEED_10;
> + break;
> + case SPEED_100:
> + ctrl = YT921X_SGMII_SPEED_100;
> + break;
> + case SPEED_1000:
> + ctrl = YT921X_SGMII_SPEED_1000;
> + break;
> + case SPEED_10000:
> + ctrl = YT921X_SGMII_SPEED_10000;
> + break;
> + case SPEED_2500:
> + ctrl = YT921X_SGMII_SPEED_2500;
> + break;
> + default:
> + ctrl = 0;
> + break;
Same here.
> + }
> + mask |= YT921X_SGMII_DUPLEX_FULL;
> + if (duplex == DUPLEX_FULL)
> + ctrl |= YT921X_SGMII_DUPLEX_FULL;
> + mask |= YT921X_SGMII_TX_PAUSE;
> + if (tx_pause)
> + ctrl |= YT921X_SGMII_TX_PAUSE;
> + mask |= YT921X_SGMII_RX_PAUSE;
> + if (rx_pause)
> + ctrl |= YT921X_SGMII_RX_PAUSE;
> + mask |= YT921X_SGMII_LINK;
> + ctrl |= YT921X_SGMII_LINK;
> + res = yt921x_reg_update_bits(priv, YT921X_SGMIIn(port),
> + mask, ctrl);
> + if (res)
> + return res;
> +
> + mask = YT921X_XMII_LINK;
> + res = yt921x_reg_set_bits(priv, YT921X_XMIIn(port), mask);
> + if (res)
> + return res;
> +
> + switch (speed) {
> + case SPEED_10:
> + ctrl = YT921X_MDIO_POLLING_SPEED_10;
> + break;
> + case SPEED_100:
> + ctrl = YT921X_MDIO_POLLING_SPEED_100;
> + break;
> + case SPEED_1000:
> + ctrl = YT921X_MDIO_POLLING_SPEED_1000;
> + break;
> + case SPEED_10000:
> + ctrl = YT921X_MDIO_POLLING_SPEED_10000;
> + break;
> + case SPEED_2500:
> + ctrl = YT921X_MDIO_POLLING_SPEED_2500;
> + break;
> + default:
> + ctrl = 0;
> + break;
and again.
> +static int
> +yt921x_port_config(struct yt921x_priv *priv, int port, unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct device *dev = to_device(priv);
> + u32 mask;
> + u32 ctrl;
> + int res;
> +
> + if (!yt921x_port_is_external(port)) {
> + if (interface != PHY_INTERFACE_MODE_INTERNAL) {
> + dev_err(dev, "Wrong mode %d on port %d\n",
> + interface, port);
> + return -EINVAL;
> + }
> + return 0;
> + }
> +
> + switch (interface) {
> + /* SGMII */
> + case PHY_INTERFACE_MODE_SGMII:
> + case PHY_INTERFACE_MODE_100BASEX:
> + case PHY_INTERFACE_MODE_1000BASEX:
> + case PHY_INTERFACE_MODE_2500BASEX:
This comment is wrong. Only the first is SGMII.
> +static void
> +yt921x_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct dsa_port *dp = dsa_phylink_to_port(config);
> + struct dsa_switch *ds = dp->ds;
> + struct yt921x_priv *priv = to_yt921x_priv(ds);
> + struct device *dev = to_device(priv);
> + int port = dp->index;
> + int res;
> +
> + cancel_delayed_work(&priv->ports[port].mib_read);
Should that be cancel_delayed_work_sync() ? Does it matter if the work
is running on a different CPU?
> +static void
> +yt921x_dsa_phylink_get_caps(struct dsa_switch *ds, int port,
> + struct phylink_config *config)
> +{
> + struct yt921x_priv *priv = to_yt921x_priv(ds);
> + const struct yt921x_info *info = priv->info;
> +
> + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000;
> +
> + if ((info->internal_mask & BIT(port)) != 0) {
> + /* Port 10 for MCU should probably go here too. But since that
> + * is untested yet, turn it down for the moment by letting it
> + * fall to the default branch.
> + */
> + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> + config->supported_interfaces);
> + } else if ((info->external_mask & BIT(port)) != 0) {
> + /* TODO: external ports may support SGMII only, XMII only, or
> + * SGMII + XMII depending on the chip. However, we can't get
> + * the accurate config table due to lack of document, thus
> + * we simply declare SGMII + XMII and rely on the correctness
> + * of devicetree for now.
> + */
> +
> + /* SGMII */
> + __set_bit(PHY_INTERFACE_MODE_SGMII,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_100BASEX,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> + config->supported_interfaces);
> + config->mac_capabilities |= MAC_2500FD;
And again
> +static int yt921x_chip_detect(struct yt921x_priv *priv)
> +{
> + struct device *dev = to_device(priv);
> + const struct yt921x_info *info;
> + u8 extmode;
> + u32 chipid;
> + u32 major;
> + u32 mode;
> + int res;
> +
> + res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
> + if (res)
> + return res;
> +
> + major = FIELD_GET(YT921X_CHIP_ID_MAJOR, chipid);
> +
> + for (info = yt921x_infos; info->name; info++)
> + if (info->major == major)
> + goto found_major;
> +
> + dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
> + return -ENODEV;
> +
> +found_major:
> + res = yt921x_reg_read(priv, YT921X_CHIP_MODE, &mode);
> + if (res)
> + return res;
> + res = yt921x_edata_read(priv, YT921X_EDATA_EXTMODE, &extmode);
> + if (res)
> + return res;
> +
> + for (; info->name; info++)
> + if (info->major == major && info->mode == mode &&
> + info->extmode == extmode)
> + goto found_chip;
> +
> + dev_err(dev, "Unsupported chipid 0x%x with chipmode 0x%x 0x%x\n",
> + chipid, mode, extmode);
> + return -ENODEV;
> +
> +found_chip:
> + /* Print chipid here since we are interested in lower 16 bits */
> + dev_info(dev,
> + "Motorcomm %s ethernet switch, chipid: 0x%x, "
> + "chipmode: 0x%x 0x%x\n",
> + info->name, chipid, mode, extmode);
> +
> + priv->info = info;
> + return 0;
The use of gotos here is backwards to normal. They are pretty much
only used in Linux to jump to the end to do cleanup on error. I don't
know of any other driver which uses goto on success. Please change
this.
> + /* Register the internal mdio bus. Nodes for internal ports should have
> + * proper phy-handle pointing to their PHYs. Not enabling the internal
> + * bus is possible, though pretty wired, if internal ports are not used.
weird, not wired.
Andrew
---
pw-bot: cr
Powered by blists - more mailing lists