netdev - Re: [PATCH net-next v2 3/5] net: phy: micrel: Add loopback support

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: <384e051a-3226-4851-8284-13acf4869965@engleder-embedded.com>
Date: Tue, 14 Jan 2025 21:09:48 +0100
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/5] net: phy: micrel: Add loopback support

On 13.01.25 22:55, Andrew Lunn wrote:
>> @@ -1030,6 +1030,33 @@ static int ksz9021_config_init(struct phy_device *phydev)
>>   #define MII_KSZ9031RN_EDPD		0x23
>>   #define MII_KSZ9031RN_EDPD_ENABLE	BIT(0)
>>   
>> +static int ksz9031_set_loopback(struct phy_device *phydev, bool enable,
>> +				int speed)
>> +{
>> +	u16 ctl = BMCR_LOOPBACK;
>> +	int ret, val;
>> +
>> +	if (!enable)
>> +		return genphy_loopback(phydev, enable, 0);
>> +
>> +	if (speed == SPEED_10 || speed == SPEED_100 || speed == SPEED_1000)
>> +		phydev->speed = speed;
>> +	else if (speed)
>> +		return -EINVAL;
>> +	phydev->duplex = DUPLEX_FULL;
>> +
>> +	ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
>> +
>> +	phy_modify(phydev, MII_BMCR, ~0, ctl);
>> +
>> +	ret = phy_read_poll_timeout(phydev, MII_BMSR, val, val & BMSR_LSTATUS,
>> +				    5000, 500000, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
> 
> As far as i can see, the marvell PHY is the only other one you can
> specify the speed? I think it would be better if the marvell
> set_loopback and this one looked similar. Either clearing the loopback
> bit and configuring aneg should be here, or the marvell driver should
> be modified to use genphy_loopback to disable loopback.

You are right, they should look similar. I will modify the marvell
driver to also use genphy_loopback() to disable loopback. This way the
same code is used to disable loopback.

genphy_loopback() does not check return codes of phy_modify() and
phy_config_aneg() if loopback is disabled, but marvell driver does.
I will keep genphy_loopback() logic without checking return codes,
as there is no reasonable error handling if loopback disable fails.

Gerhard