| 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: <Zqdd1mfSUDK9EifJ@shell.armlinux.org.uk>
Date: Mon, 29 Jul 2024 10:16:06 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
andrew@...n.ch, horms@...nel.org, hkallweit1@...il.com,
richardcochran@...il.com, rdunlap@...radead.org,
bryan.whitehead@...rochip.com, edumazet@...gle.com,
pabeni@...hat.com, linux-kernel@...r.kernel.org,
UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next V2 3/4] net: lan743x: Migrate phylib to phylink
On Tue, Jul 16, 2024 at 05:03:48PM +0530, Raju Lakkaraju wrote:
> +static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> + struct phy_device *phydev,
> + unsigned int link_an_mode,
> + phy_interface_t interface,
> + int speed, int duplex,
> + bool tx_pause, bool rx_pause)
> +{
> + struct net_device *netdev = to_net_dev(config->dev);
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + int mac_cr;
> + u8 cap;
> +
> + mac_cr = lan743x_csr_read(adapter, MAC_CR);
> + /* Pre-initialize register bits.
> + * Resulting value corresponds to SPEED_10
> + */
> + mac_cr &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
> + if (speed == SPEED_2500)
> + mac_cr |= (MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
> + else if (speed == SPEED_1000)
> + mac_cr |= (MAC_CR_CFG_H_);
> + else if (speed == SPEED_100)
> + mac_cr |= (MAC_CR_CFG_L_);
These parens in each of these if() sub-blocks is not required. |=
operates the same way as = - all such operators are treated the same
in C.
> +
> + lan743x_csr_write(adapter, MAC_CR, mac_cr);
> +
> + lan743x_ptp_update_latency(adapter, speed);
> +
> + /* Flow Control operation */
> + cap = 0;
> + if (tx_pause)
> + cap |= FLOW_CTRL_TX;
> + if (rx_pause)
> + cap |= FLOW_CTRL_RX;
> +
> + lan743x_mac_flow_ctrl_set_enables(adapter,
> + cap & FLOW_CTRL_TX,
> + cap & FLOW_CTRL_RX);
> +
> + netif_tx_wake_all_queues(to_net_dev(config->dev));
You already have "netdev", so there's no need to do the to_net_dev()
dance again here.
> +}
> +
> +static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
> + .mac_config = lan743x_phylink_mac_config,
> + .mac_link_down = lan743x_phylink_mac_link_down,
> + .mac_link_up = lan743x_phylink_mac_link_up,
> +};
I guess as there's no PCS support here, you don't support inband mode
for 1000base-X (which is rather fundamental for it).
> +
> +static int lan743x_phylink_create(struct net_device *netdev)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> + struct phylink *pl;
> +
> + adapter->phylink_config.dev = &netdev->dev;
> + adapter->phylink_config.type = PHYLINK_NETDEV;
> + adapter->phylink_config.mac_managed_pm = false;
> +
> + adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
> + MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
> +
> + lan743x_phy_interface_select(adapter);
> +
> + switch (adapter->phy_interface) {
> + case PHY_INTERFACE_MODE_SGMII:
> + __set_bit(PHY_INTERFACE_MODE_SGMII,
> + adapter->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> + adapter->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> + adapter->phylink_config.supported_interfaces);
> + break;
> + case PHY_INTERFACE_MODE_GMII:
> + __set_bit(PHY_INTERFACE_MODE_GMII,
> + adapter->phylink_config.supported_interfaces);
> + break;
> + case PHY_INTERFACE_MODE_MII:
> + __set_bit(PHY_INTERFACE_MODE_MII,
> + adapter->phylink_config.supported_interfaces);
> + break;
> + default:
> + __set_bit(PHY_INTERFACE_MODE_RGMII,
> + adapter->phylink_config.supported_interfaces);
Do you really only support RGMII and not RGMII_ID/RGMII_TXID/RGMII_RXID
(which are normally implemented by tweaking the delays at the PHY end
of the RGMII link) ?
> +static bool lan743x_phy_handle_exists(struct device_node *dn)
> +{
> + dn = of_parse_phandle(dn, "phy-handle", 0);
> + of_node_put(dn);
> + if (IS_ERR(dn))
> + return false;
> +
> + return true;
This likely doesn't work. Have you checked what the return values for
of_parse_phandle() actually are before creating this, because to me
this looks like you haven't - and thus what you've created is wrong.
of_parse_phandle() doesn't return error-pointers when it fails, it
returns NULL. Therefore, this will always return true.
We have another implementation of something very similar in the macb
driver - see macb_phy_handle_exists(), and this one is implemented
correctly.
> +}
> +
> +static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
> +{
> + struct device_node *dn = adapter->pdev->dev.of_node;
> + struct net_device *dev = adapter->netdev;
> + struct fixed_phy_status fphy_status = {
> + .link = 1,
> + .speed = SPEED_1000,
> + .duplex = DUPLEX_FULL,
> + };
> + struct phy_device *phydev;
> + int ret;
> +
> + if (dn)
> + ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
> +
> + if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> + phydev = phy_find_first(adapter->mdiobus);
> + if (!phydev) {
> + if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> + ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> + phydev = fixed_phy_register(PHY_POLL,
> + &fphy_status,
> + NULL);
> + if (IS_ERR(phydev)) {
> + netdev_err(dev, "No PHY/fixed_PHY found\n");
> + return PTR_ERR(phydev);
> + }
Eww. Given that phylink has its own internal fixed-PHY support, can we
not find some way to avoid the legacy fixed-PHY usage here?
--
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