linux-kernel - Re: [PATCH net-next V5 4/5] net: lan743x: Migrate phylib to phylink

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cad5b31d-7713-4a1d-88e4-58d6878e90c5@lunn.ch>
Date: Thu, 5 Sep 2024 15:11:20 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, linux@...linux.org.uk,
	kuba@...nel.org, hkallweit1@...il.com, richardcochran@...il.com,
	rdunlap@...radead.org, bryan.whitehead@...rochip.com,
	edumazet@...gle.com, pabeni@...hat.com,
	maxime.chevallier@...tlin.com, linux-kernel@...r.kernel.org,
	horms@...nel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next V5 4/5] net: lan743x: Migrate phylib to phylink

> +static struct {
> +	unsigned long mask;
> +	int speed;
> +	int duplex;
> +} lan743x_mac_caps_params[] = {
> +	{ MAC_2500FD,	SPEED_2500,   DUPLEX_FULL },
> +	{ MAC_1000FD,	SPEED_1000,   DUPLEX_FULL },
> +	{ MAC_100FD,	SPEED_100,    DUPLEX_FULL },
> +	{ MAC_10FD,	SPEED_10,     DUPLEX_FULL },
> +	{ MAC_100HD,	SPEED_100,    DUPLEX_HALF },
> +	{ MAC_10HD,	SPEED_10,     DUPLEX_HALF },
> +};
> +
> +static int lan743x_find_max_speed(unsigned long caps, int *speed, int *duplex)
> +{
> +	int i;
> +
> +	*speed = SPEED_UNKNOWN;
> +	*duplex = DUPLEX_UNKNOWN;
> +	for (i = 0; i < ARRAY_SIZE(lan743x_mac_caps_params); i++) {
> +		if (caps & lan743x_mac_caps_params[i].mask) {
> +			*speed = lan743x_mac_caps_params[i].speed;
> +			*duplex = lan743x_mac_caps_params[i].duplex;
> +			break;
> +		}
> +	}
> +
> +	return *speed == SPEED_UNKNOWN ? -EINVAL : 0;
> +}
> +
> +static int lan743x_phylink_create(struct lan743x_adapter *adapter)
> +{
> +	struct net_device *netdev = adapter->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;
> +


> +	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);
> +		adapter->phylink_config.mac_capabilities |= MAC_2500FD;


> +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 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) {
> +			/* attach the mac to the phy */
> +			ret = phylink_connect_phy(adapter->phylink, phydev);
> +		} else if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> +			      ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> +			struct phylink_link_state state;
> +			unsigned long caps;
> +
> +			caps = adapter->phylink_config.mac_capabilities;
> +			ret = lan743x_find_max_speed(caps, &state.speed,
> +						     &state.duplex);

This seems like an overly complicated way to determine the speed. If
MAC_2500FD is set, you know it is 2500. Otherwise it must by 1000. Why
not use a simple if?

I'm not the best at reviewing phylink patches, but since Russell is
off-lime, i'm the best you have. So

Reviewed-by: Andrew Lunn <andrew@...n.ch>

but please do consider simplifying the code above.

    Andrew