netdev - Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

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: <f9ed0d60-4883-4ca7-b692-3eedf65ca4dd@lunn.ch>
Date: Mon, 1 Jul 2024 19:00:29 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

> +static int dp83869_connect_phy(void *upstream, struct phy_device *phy)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +
> +	dp83869 = phydev->priv;
> +
> +	if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
> +		return 0;
> +
> +	if (!phy->drv) {
> +		dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!\n");

more instances which could be phydev_{err|warn|info}().

> +		return 0;
> +	}
> +
> +	phy_support_asym_pause(phy);

That is unusual. This is normally used by a MAC driver to indicate it
supports asym pause. It is the MAC which implements pause, but the PHY
negotiates it. So this tells phylib what to advertise to the link
partner. Why would a PHY do this? What if the MAC does not support
asym pause?

> +	linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces);
> +	phy->interface = PHY_INTERFACE_MODE_SGMII;
> +	phy->port = PORT_TP;
> +
> +	phy->speed = SPEED_UNKNOWN;
> +	phy->duplex = DUPLEX_UNKNOWN;
> +	phy->pause = MLO_PAUSE_NONE;
> +	phy->interrupts = PHY_INTERRUPT_DISABLED;
> +	phy->irq = PHY_POLL;
> +	phy->phy_link_change = &dp83869_sfp_phy_change;
> +	phy->state = PHY_READY;

I don't know of any other PHY which messes with the state machine like
this. This needs some explanation.

> +
> +	dp83869->mod_phy = phy;
> +
> +	return 0;
> +}
> +
> +static void dp83869_disconnect_phy(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +
> +	dp83869 = phydev->priv;
> +	dp83869->mod_phy = NULL;
> +}
> +
> +static int dp83869_module_start(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +	struct phy_device *mod_phy;
> +	int ret;
> +
> +	dp83869 = phydev->priv;
> +	mod_phy = dp83869->mod_phy;
> +	if (!mod_phy)
> +		return 0;
> +
> +	ret = phy_init_hw(mod_phy);
> +	if (ret) {
> +		dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: error %d", ret);
> +		return ret;
> +	}
> +
> +	phy_start(mod_phy);

Something else no other PHY driver does....

	Andrew