netdev - Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

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: <690e556f-a486-41e3-99ef-c29cb0a26d83@lunn.ch>
Date: Tue, 3 Dec 2024 00:52:53 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Zhiyuan Wan <kmlinuxm@...il.com>
Cc: kuba@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	willy.liu@...ltek.com, Yuki Lee <febrieac@...look.com>
Subject: Re: [PATCH 1/2] net: phy: realtek: add combo mode support for
 RTL8211FS

> +static int rtl8211f_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	struct rtl821x_priv *priv = phydev->priv;
> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	linkmode_copy(phydev->advertising, phydev->supported);

This is all very unusual for config_aneg(). genphy_read_abilities()
will have been done very early on during phy_probe(). So why do it
now? And why overwrite how the user might of configured what is to be
advertised?

> +static int rtl8211f_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +	struct rtl821x_priv *priv = phydev->priv;
> +	bool changed = false;
> +
> +	if (rtl8211f_mode(phydev) != priv->lastmode) {
> +		changed = true;
> +		ret = rtl8211f_config_aneg(phydev);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = genphy_restart_aneg(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return genphy_c37_read_status(phydev, &changed);
> +}

So you are assuming read_status() is called once per second? But what
about when interrupts are used?

You might want to look at how the marvell driver does this. It is not
great, but better than this.

    Andrew

---
pw-bot: cr