| 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: <YiHa/himI3WJVOhy@shell.armlinux.org.uk>
Date: Fri, 4 Mar 2022 09:25:18 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, kishon@...com, vigneshr@...com,
grygorii.strashko@...com
Subject: Re: [RESEND PATCH] net: ethernet: ti: am65-cpsw: Convert to PHYLINK
Hi,
On Fri, Mar 04, 2022 at 01:28:12PM +0530, Siddharth Vadapalli wrote:
> Convert am65-cpsw driver and am65-cpsw ethtool to use Phylink APIs
> as described at Documentation/networking/sfp-phylink.rst. All calls
> to Phy APIs are replaced with their equivalent Phylink APIs.
Okay, that's what you're doing, but please mention what the reason for
the change is.
> @@ -1494,6 +1409,87 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
> .ndo_get_devlink_port = am65_cpsw_ndo_get_devlink_port,
> };
>
> +static void am65_cpsw_nuss_validate(struct phylink_config *config, unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + phylink_generic_validate(config, supported, state);
> +}
If you don't need anything special, please just initialise the member
directly:
.validate = phylink_generic_validate,
> +
> +static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + /* Currently not used */
> +}
> +
> +static void am65_cpsw_nuss_mac_link_down(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct am65_cpsw_slave_data *slave = container_of(config, struct am65_cpsw_slave_data,
> + phylink_config);
> + struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> + struct am65_cpsw_common *common = port->common;
> + struct net_device *ndev = port->ndev;
> + int tmo;
> +
> + /* disable forwarding */
> + cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_DISABLE);
> +
> + cpsw_sl_ctl_set(port->slave.mac_sl, CPSW_SL_CTL_CMD_IDLE);
> +
> + tmo = cpsw_sl_wait_for_idle(port->slave.mac_sl, 100);
> + dev_dbg(common->dev, "down msc_sl %08x tmo %d\n",
> + cpsw_sl_reg_read(port->slave.mac_sl, CPSW_SL_MACSTATUS), tmo);
> +
> + cpsw_sl_ctl_reset(port->slave.mac_sl);
> +
> + am65_cpsw_qos_link_down(ndev);
> + netif_tx_disable(ndev);
You didn't call netif_tx_disable() in your adjust_link afaics, so why
is it added here?
> +}
> +
> +static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> + unsigned int mode, phy_interface_t interface, int speed,
> + int duplex, bool tx_pause, bool rx_pause)
> +{
> + struct am65_cpsw_slave_data *slave = container_of(config, struct am65_cpsw_slave_data,
> + phylink_config);
> + struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> + struct am65_cpsw_common *common = port->common;
> + struct net_device *ndev = port->ndev;
> + u32 mac_control = CPSW_SL_CTL_GMII_EN;
> +
> + if (speed == SPEED_1000)
> + mac_control |= CPSW_SL_CTL_GIG;
> + if (speed == SPEED_10 && interface == PHY_INTERFACE_MODE_RGMII)
> + /* Can be used with in band mode only */
> + mac_control |= CPSW_SL_CTL_EXT_EN;
> + if (speed == SPEED_100 && interface == PHY_INTERFACE_MODE_RMII)
> + mac_control |= CPSW_SL_CTL_IFCTL_A;
> + if (duplex)
> + mac_control |= CPSW_SL_CTL_FULLDUPLEX;
> +
> + /* rx_pause/tx_pause */
> + if (rx_pause)
> + mac_control |= CPSW_SL_CTL_RX_FLOW_EN;
> +
> + if (tx_pause)
> + mac_control |= CPSW_SL_CTL_TX_FLOW_EN;
> +
> + cpsw_sl_ctl_set(port->slave.mac_sl, mac_control);
> +
> + /* enable forwarding */
> + cpsw_ale_control_set(common->ale, port->port_id, ALE_PORT_STATE, ALE_PORT_STATE_FORWARD);
> +
> + am65_cpsw_qos_link_up(ndev, speed);
> + netif_tx_wake_all_queues(ndev);
> +}
> +
> +static const struct phylink_mac_ops am65_cpsw_phylink_mac_ops = {
> + .validate = am65_cpsw_nuss_validate,
> + .mac_config = am65_cpsw_nuss_mac_config,
> + .mac_link_down = am65_cpsw_nuss_mac_link_down,
> + .mac_link_up = am65_cpsw_nuss_mac_link_up,
> +};
> +
> static void am65_cpsw_nuss_slave_disable_unused(struct am65_cpsw_port *port)
> {
> struct am65_cpsw_common *common = port->common;
> @@ -1887,24 +1883,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> of_property_read_bool(port_np, "ti,mac-only");
>
> /* get phy/link info */
> - if (of_phy_is_fixed_link(port_np)) {
> - ret = of_phy_register_fixed_link(port_np);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "failed to register fixed-link phy %pOF\n",
> - port_np);
> - port->slave.phy_node = of_node_get(port_np);
> - } else {
> - port->slave.phy_node =
> - of_parse_phandle(port_np, "phy-handle", 0);
> - }
> -
> - if (!port->slave.phy_node) {
> - dev_err(dev,
> - "slave[%d] no phy found\n", port_id);
> - return -ENODEV;
> - }
> -
> + port->slave.phy_node = port_np;
> ret = of_get_phy_mode(port_np, &port->slave.phy_if);
> if (ret) {
> dev_err(dev, "%pOF read phy-mode err %d\n",
> @@ -1947,6 +1926,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
> struct am65_cpsw_ndev_priv *ndev_priv;
> struct device *dev = common->dev;
> struct am65_cpsw_port *port;
> + struct phylink *phylink;
> int ret;
>
> port = &common->ports[port_idx];
> @@ -1984,6 +1964,26 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
> port->ndev->netdev_ops = &am65_cpsw_nuss_netdev_ops;
> port->ndev->ethtool_ops = &am65_cpsw_ethtool_ops_slave;
>
> + /* Configuring Phylink */
> + port->slave.phylink_config.dev = &port->ndev->dev;
> + port->slave.phylink_config.type = PHYLINK_NETDEV;
> + port->slave.phylink_config.pcs_poll = true;
Does this compile? This member was removed, so you probably get a
compile error today.
> + port->slave.phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 |
> + MAC_100 | MAC_1000FD | MAC_2500FD;
> +
> + phy_interface_set_rgmii(port->slave.phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_SGMII, port->slave.phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_1000BASEX, port->slave.phylink_config.supported_interfaces);
If you support SGMII and 1000BASE-X with inband signalling, I strongly
recommend that you implement phylink_pcs support as well, so you are
able to provide phylink with the inband results.
> +
> + phylink = phylink_create(&port->slave.phylink_config, dev->fwnode, port->slave.phy_if,
> + &am65_cpsw_phylink_mac_ops);
> + if (IS_ERR(phylink)) {
> + phylink_destroy(port->slave.phylink);
This is wrong and will cause a NULL pointer dereference - please remove
the call to phylink_destroy() here.
However, I could not find another call to phylink_destroy() in your
patch which means you will leak memory when the driver is unbound.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists