Date:   Mon, 20 Feb 2023 14:18:37 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support
Message-ID: <Y/OBPZRGM+viGp+8@shell.armlinux.org.uk>
References: <20230220135605.1136137-1-o.rempel@pengutronix.de>
 <20230220135605.1136137-4-o.rempel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230220135605.1136137-4-o.rempel@pengutronix.de>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

Hi,

A couple of minor points, but not sufficient not to prevent merging
this.

On Mon, Feb 20, 2023 at 02:56:04PM +0100, Oleksij Rempel wrote:
> @@ -865,7 +864,12 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
>   */
>  int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
>  {
> -	return genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};

It would be nice to avoid this initialisation in the case where
eee_enabled is true, as this takes CPU cycles. However, not too
bothered about it as this isn't a fast path.

> +
> +	if (!phydev->eee_enabled)
> +		return genphy_c45_write_eee_adv(phydev, adv);
> +
> +	return genphy_c45_write_eee_adv(phydev, phydev->advertising_eee);
>  }
>  
>  /**
> @@ -1431,17 +1435,17 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
>  int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
>  			       struct ethtool_eee *data)
>  {
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
>  	int ret;
>  
>  	if (data->eee_enabled) {
> +		phydev->eee_enabled = true;
>  		if (data->advertised)
> -			adv[0] = data->advertised;
> -		else
> -			linkmode_copy(adv, phydev->supported_eee);
> +			phydev->advertising_eee[0] = data->advertised;

Is there a reason not to use ethtool_convert_legacy_u32_to_link_mode()?
I'm guessing this will be more efficient.

Other than that, looks good.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!