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 for Android: free password hash cracker in your pocket

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

Message-ID: <b8329de3-150a-4f71-bf53-cc52c513a620@loongson.cn>
Date: Fri, 5 Jul 2024 19:17:01 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Andrew Lunn <andrew@...n.ch>, Serge Semin <fancer.lancer@...il.com>
Cc: 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.

Hi all,


Thanks for your comments!


在 2024/7/4 04:33, Russell King (Oracle) 写道:
> On Wed, Jul 03, 2024 at 09:09:53PM +0200, Andrew Lunn wrote:
>>> Rather than erroring out, I think it may be better to just adopt
>>> the Marvell solution to this problem to give consistent behaviour
>>> across all PHYs.
>> Yes, expand phy_config_aneg() to look for this condition and enable
>> autoneg. But should we disable it when the condition is reverted? The
>> user swaps to 100Mbps forced?
> 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.


Thanks,

Yanteng