netdev - Re: [PATCH net-next v13 12/15] net: stmmac: Fixed failure to set network speed to 1000.

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: <23704881-e100-4498-a34c-e206bc32b308@loongson.cn>
Date: Fri, 5 Jul 2024 19:38:40 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Serge Semin <fancer.lancer@...il.com>,
 si.yanteng@...ux.dev, Huacai Chen <chenhuacai@...nel.org>,
 hkallweit1@...il.com, peppe.cavallaro@...com, alexandre.torgue@...s.st.com,
 joabreu@...opsys.com, Jose.Abreu@...opsys.com, guyinggang@...ngson.cn,
 netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v13 12/15] net: stmmac: Fixed failure to set
 network speed to 1000.


在 2024/7/5 19:31, Russell King (Oracle) 写道:
> On Fri, Jul 05, 2024 at 07:17:01PM +0800, Yanteng Si wrote:
>> 在 2024/7/4 04:33, Russell King (Oracle) 写道:
>>> I think we should "lie" to userspace rather than report how the
>>> hardware was actually programmed - again, because that's what would
>>> happen with Marvell Alaska.
>>>
>>>> What about other speeds? Is this limited to 1G? Since we have devices
>>>> without auto-neg for 2500BaseX i assume it is not an issue there.
>>> 1000base-X can have AN disabled - that's not an issue. Yes, there's
>>> the ongoing issues with 2500base-X. 10Gbase-T wording is similar to
>>> 1000base-T, so we probably need to do similar there. Likely also the
>>> case for 2500base-T and 5000base-T as well.
>>>
>>> So I'm thinking of something like this (untested):
>>>
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index 6c6ec9475709..197c4d5ab55b 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -2094,22 +2094,20 @@ EXPORT_SYMBOL(phy_reset_after_clk_enable);
>>>    /**
>>>     * genphy_config_advert - sanitize and advertise auto-negotiation parameters
>>>     * @phydev: target phy_device struct
>>> + * @advert: auto-negotiation parameters to advertise
>>>     *
>>>     * Description: Writes MII_ADVERTISE with the appropriate values,
>>>     *   after sanitizing the values to make sure we only advertise
>>>     *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
>>>     *   hasn't changed, and > 0 if it has changed.
>>>     */
>>> -static int genphy_config_advert(struct phy_device *phydev)
>>> +static int genphy_config_advert(struct phy_device *phydev,
>>> +				const unsigned long *advert)
>>>    {
>>>    	int err, bmsr, changed = 0;
>>>    	u32 adv;
>>> -	/* Only allow advertising what this PHY supports */
>>> -	linkmode_and(phydev->advertising, phydev->advertising,
>>> -		     phydev->supported);
>>> -
>>> -	adv = linkmode_adv_to_mii_adv_t(phydev->advertising);
>>> +	adv = linkmode_adv_to_mii_adv_t(advert);
>>>    	/* Setup standard advertisement */
>>>    	err = phy_modify_changed(phydev, MII_ADVERTISE,
>>> @@ -2132,7 +2130,7 @@ static int genphy_config_advert(struct phy_device *phydev)
>>>    	if (!(bmsr & BMSR_ESTATEN))
>>>    		return changed;
>>> -	adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
>>> +	adv = linkmode_adv_to_mii_ctrl1000_t(advert);
>>>    	err = phy_modify_changed(phydev, MII_CTRL1000,
>>>    				 ADVERTISE_1000FULL | ADVERTISE_1000HALF,
>>> @@ -2356,6 +2354,9 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg);
>>>     */
>>>    int __genphy_config_aneg(struct phy_device *phydev, bool changed)
>>>    {
>>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert);
>>> +	const struct phy_setting *set;
>>> +	unsigned long *advert;
>>>    	int err;
>>>    	err = genphy_c45_an_config_eee_aneg(phydev);
>>> @@ -2370,10 +2371,25 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
>>>    	else if (err)
>>>    		changed = true;
>>> -	if (AUTONEG_ENABLE != phydev->autoneg)
>>> +	if (phydev->autoneg == AUTONEG_ENABLE) {
>>> +		/* Only allow advertising what this PHY supports */
>>> +		linkmode_and(phydev->advertising, phydev->advertising,
>>> +			     phydev->supported);
>>> +		advert = phydev->advertising;
>>> +	} else if (phydev->speed < SPEED_1000) {
>>>    		return genphy_setup_forced(phydev);
>>> +	} else {
>>> +		linkmode_zero(fixed_advert);
>>> +
>>> +		set = phy_lookup_setting(phydev->speed, phydev->duplex,
>>> +					 phydev->supported, true);
>>> +		if (set)
>>> +			linkmode_set(set->bit, fixed_advert);
>>> +
>>> +		advert = fixed_advert;
>>> +	}
>>> -	err = genphy_config_advert(phydev);
>>> +	err = genphy_config_advert(phydev, advert);
>>>    	if (err < 0) /* error */
>>>    		return err;
>>>    	else if (err)
>> It looks great, but I still want to follow Russell's earlier advice and drop
>> this patch
>>
>> from v14, then submit it separately with the above code.
> If the above change is made to phylib, then drivers do not need any
> changes other than removing such workarounds detecting !AN with
> speed = 1G.
>
> The point of the above change is that drivers shouldn't be doing
> anything and the issue should be handled entirely within phylib.
>
OK, I see. I will try this after send v14(Maybe tomorrow, or next Monday).


Thanks,

Yanteng